Moodle
  1. Moodle
  2. MDL-31311

Fix duplicate title attributes for iframed file and URL resources

    Details

    • Testing Instructions:
      Hide

      1. Make sure that iframes are selected as an option under Site Administration --> Plugins --> Activity Modules --> URL and Site Administration --> Plugins --> Activity Modules --> File
      2. Navigate to a course, turn editing on, add a new File or URL resource.
      3. Fill in data and select "In Frame" from the Display drop down
      4. Save the resource and click to view it.
      5. View the source and see that the title attribute for both iframe elements is identical and only reflects the module name ('File' or 'URL')

      Show
      1. Make sure that iframes are selected as an option under Site Administration --> Plugins --> Activity Modules --> URL and Site Administration --> Plugins --> Activity Modules --> File 2. Navigate to a course, turn editing on, add a new File or URL resource. 3. Fill in data and select "In Frame" from the Display drop down 4. Save the resource and click to view it. 5. View the source and see that the title attribute for both iframe elements is identical and only reflects the module name ('File' or 'URL')
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-31311-Fix_iframe_titles

      Description

      The title attribute for the navigation frame and the content frames on File and URL resources are not helpful in their current state. They are identical and only reflect the type of resource that is being viewed:

      <frameset rows="$framesize,*">
      <frame src="$navurl" title="$modulename"/>
      <frame src="$exteurl" title="$modulename"/>
      </frameset>

      This is confusing to those relying on screen readers.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Rajesh Taneja added a comment -

            Thanks for spotting the issue and providing the patch, Glenn.
            I think, it will be nice to leave the resource title unchanged and change the title for resource content.
            i.e.

            $contentframetitle = format_string($resource->name);
            ...
            ...
             
            <frame src="$navurl" title="$modulename" />
            <frame src="$fileurl" title="$contentframetitle" />
            

            Show
            Rajesh Taneja added a comment - Thanks for spotting the issue and providing the patch, Glenn. I think, it will be nice to leave the resource title unchanged and change the title for resource content. i.e. $contentframetitle = format_string($resource->name); ... ...   <frame src="$navurl" title="$modulename" /> <frame src="$fileurl" title="$contentframetitle" />
            Hide
            Glenn Ansley added a comment -

            Hi Rajesh. Thanks for your feedback. I have provided made the changes. Same diff URL / Branch

            Show
            Glenn Ansley added a comment - Hi Rajesh. Thanks for your feedback. I have provided made the changes. Same diff URL / Branch
            Hide
            Rajesh Taneja added a comment -

            Thanks for fixing this Glenn,
            Patch looks spot-on. Please feel free to push it for integration review, or let me know and I will do the needful.

            Show
            Rajesh Taneja added a comment - Thanks for fixing this Glenn, Patch looks spot-on. Please feel free to push it for integration review, or let me know and I will do the needful.
            Hide
            Glenn Ansley added a comment -

            Hi Rajesh,
            Looks like I'm either missing the ability or the knowhow to submit it for integration review. Looks like you'll have to do it.

            Show
            Glenn Ansley added a comment - Hi Rajesh, Looks like I'm either missing the ability or the knowhow to submit it for integration review. Looks like you'll have to do it.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            Glenn Ansley added a comment -

            Rebased

            Show
            Glenn Ansley added a comment - Rebased
            Hide
            Aparup Banerjee added a comment -

            just adding to the repository field here.

            Show
            Aparup Banerjee added a comment - just adding to the repository field here.
            Hide
            Aparup Banerjee added a comment -

            thanks, this improvement is in integration now for testing.

            Show
            Aparup Banerjee added a comment - thanks, this improvement is in integration now for testing.
            Hide
            Adrian Greeve added a comment -

            Checked out the solution pre and post patch. The title reflects the resource better.
            Thanks.

            Show
            Adrian Greeve added a comment - Checked out the solution pre and post patch. The title reflects the resource better. Thanks.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Many thanks for your collaboration, this code has been integrated upstream and it's available in all the repositories.

            Closing, ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Many thanks for your collaboration, this code has been integrated upstream and it's available in all the repositories. Closing, ciao
            Hide
            Marina Glancy added a comment -

            Noting here that this issue caused a regression MDL-47995

            Show
            Marina Glancy added a comment - Noting here that this issue caused a regression MDL-47995

              People

              • Votes:
                0 Vote for this issue
                Watchers:
                1 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: