Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-43348

html <nav> element on the wrong content in bootstrapbase

    Details

    • Testing Instructions:
      Hide
      1. Apply the patch.
      2. Select the Clean theme.
      3. Go to Site Administration > Notifications and confirm that the page displays correctly (columns2.php test).
      4. Log out and confirm that the login page displays correctly (columns1.php test).
      5. Log back in.
      6. Create a course and confirm that the course displays correctly (column3.php test).
      7. Go to Site Administration -> Reports -> Live Logs and click on 'Live logs from the past hour', confirm that the popup window displays correctly (popup.php test).
      Show
      Apply the patch. Select the Clean theme. Go to Site Administration > Notifications and confirm that the page displays correctly (columns2.php test). Log out and confirm that the login page displays correctly (columns1.php test). Log back in. Create a course and confirm that the course displays correctly (column3.php test). Go to Site Administration -> Reports -> Live Logs and click on 'Live logs from the past hour', confirm that the popup window displays correctly (popup.php test).
    • Affected Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-43348_master

      Description

      Current code in layout files:

      <div class="breadcrumb-nav"><?php echo $OUTPUT->navbar(); ?></div>
      <nav class="breadcrumb-button"><?php echo $OUTPUT->page_heading_button(); ?></nav>

      oops - thats a bit wrong isn't it?
      I think the breadcrumb is navigation, the button is not navigation.

      More information:
      The current mark-up breaks the HTML5 standard as stated in:
      http://www.w3.org/TR/2010/WD-html5-20100624/sections.html#the-nav-element

      Please see attached image 2013-12-12 10_31_14-MooGJB_ Administration_ Appearance_ Themes_ Theme selector.png.

        Gliffy Diagrams

          Activity

          Hide
          stuartlamour Stuart Lamour added a comment -

          probably going to be the same for all those layout files in clean as well.

          Show
          stuartlamour Stuart Lamour added a comment - probably going to be the same for all those layout files in clean as well.
          Hide
          gb2048 Gareth J Barnard added a comment -

          Will be as they are almost the same as bootstrapbase but have an extra lines for the settings.

          Show
          gb2048 Gareth J Barnard added a comment - Will be as they are almost the same as bootstrapbase but have an extra lines for the settings.
          Hide
          gb2048 Gareth J Barnard added a comment - - edited

          With permission from Stuart Lamour I have assigned to me with the intent of working on it this week. Will be for M2.5+.

          Note: Need to double check any specific CSS that targets the existing mark-up as a 'div'.

          Thoughts Mary Evans please.

          Show
          gb2048 Gareth J Barnard added a comment - - edited With permission from Stuart Lamour I have assigned to me with the intent of working on it this week. Will be for M2.5+. Note: Need to double check any specific CSS that targets the existing mark-up as a 'div'. Thoughts Mary Evans please.
          Show
          gb2048 Gareth J Barnard added a comment - Original diff link from Stuart Lamour = https://github.com/stuartlamour/moodle/compare/moodle:master...master
          Hide
          lazydaisy Mary Evans added a comment -

          Have you tested it? Does it work ok without needing css fixes?

          If nothing untoward happens then, by all means go for it.

          You will also need to fix theme/clean/layouts too don't forget?

          Show
          lazydaisy Mary Evans added a comment - Have you tested it? Does it work ok without needing css fixes? If nothing untoward happens then, by all means go for it. You will also need to fix theme/clean/layouts too don't forget?
          Hide
          lazydaisy Mary Evans added a comment -

          With regards working on 2.5. If it is a bug you can fix it, if it isn't and just a matter of a different layout then sorry you cannot change 2.5 which is a stable branch.

          Show
          lazydaisy Mary Evans added a comment - With regards working on 2.5. If it is a bug you can fix it, if it isn't and just a matter of a different layout then sorry you cannot change 2.5 which is a stable branch.
          Hide
          lazydaisy Mary Evans added a comment -

          Sorry for the noise but could someone please write a little more in the description for this as it isn't that clear why you want to change the div to nav and visa versa for the breadcrumb and nav button.

          Show
          lazydaisy Mary Evans added a comment - Sorry for the noise but could someone please write a little more in the description for this as it isn't that clear why you want to change the div to nav and visa versa for the breadcrumb and nav button.
          Hide
          gb2048 Gareth J Barnard added a comment - - edited

          Hi Mary,

          The current mark-up breaks the HTML5 standard as stated on: http://www.w3.org/TR/2010/WD-html5-20100624/sections.html#the-nav-element - please see attached image 2013-12-12 10_31_14-MooGJB_ Administration_ Appearance_ Themes_ Theme selector.png.

          Cheers,

          Gareth

          Show
          gb2048 Gareth J Barnard added a comment - - edited Hi Mary, The current mark-up breaks the HTML5 standard as stated on: http://www.w3.org/TR/2010/WD-html5-20100624/sections.html#the-nav-element - please see attached image 2013-12-12 10_31_14-MooGJB_ Administration_ Appearance_ Themes_ Theme selector.png. Cheers, Gareth
          Hide
          gb2048 Gareth J Barnard added a comment -

          Hi,

          Requesting peer review with master branch only at the moment as other branches are a duplication. If acceptable as a bug will back-port and provide the other branches.

          Cheers,

          Gareth

          Show
          gb2048 Gareth J Barnard added a comment - Hi, Requesting peer review with master branch only at the moment as other branches are a duplication. If acceptable as a bug will back-port and provide the other branches. Cheers, Gareth
          Hide
          lazydaisy Mary Evans added a comment - - edited

          I've checked through your submitted changes, as well as the corresponding CSS and can find nothing wrong, so submitting this for Integration Review.

          Thanks Gareth for fixing this in Moodle bootstrapbase and clean themes.

          Also a big thanks to Stuart for spotting the error in the first place.

          Cheers
          Mary

          Show
          lazydaisy Mary Evans added a comment - - edited I've checked through your submitted changes, as well as the corresponding CSS and can find nothing wrong, so submitting this for Integration Review. Thanks Gareth for fixing this in Moodle bootstrapbase and clean themes. Also a big thanks to Stuart for spotting the error in the first place. Cheers Mary
          Hide
          lazydaisy Mary Evans added a comment -

          Hi Gareth, you will also need to create a branch based on MOODLE_26_STABLE. Also during integration the Integrator may also allow this to be back-ported to 2.5 as it is a bug. So may be in your own interest to have that branch ready too?
          Thanks

          Show
          lazydaisy Mary Evans added a comment - Hi Gareth, you will also need to create a branch based on MOODLE_26_STABLE. Also during integration the Integrator may also allow this to be back-ported to 2.5 as it is a bug. So may be in your own interest to have that branch ready too? Thanks
          Hide
          gb2048 Gareth J Barnard added a comment -

          Thanks Mary, I'll create some as soon as I can. Please see my previous comment.

          Show
          gb2048 Gareth J Barnard added a comment - Thanks Mary, I'll create some as soon as I can. Please see my previous comment.
          Hide
          lazydaisy Mary Evans added a comment -

          Sorry, I was called away while checkout out why only popup.php was changed in Bootstrapbase and not in Clean theme, and noticed that Clean theme does not have breadcrumb navigation. Checking this against Bootstrapbase/config.php I see that popup layout does not require navbar (which in Moodle speak is the breadcrumb navbar) so bootstrapbase/layout/popup.php is wrong so you can remove the breadcrumb from that file if you would, please?
           

              // Pages that appear in pop-up windows - no navigation, no blocks, no header.
              'popup' => array(
                  'file' => 'popup.php',
                  'regions' => array(),
                  'options' => array('nofooter'=>true, 'nonavbar'=>true),
              ),

          Also since the updates are due any time now, it may be better to wait until they have been done before you embark on the new branches then you will be less likely to have rebase problems.

          Show
          lazydaisy Mary Evans added a comment - Sorry, I was called away while checkout out why only popup.php was changed in Bootstrapbase and not in Clean theme, and noticed that Clean theme does not have breadcrumb navigation. Checking this against Bootstrapbase/config.php I see that popup layout does not require navbar (which in Moodle speak is the breadcrumb navbar) so bootstrapbase/layout/popup.php is wrong so you can remove the breadcrumb from that file if you would, please?   // Pages that appear in pop-up windows - no navigation, no blocks, no header. 'popup' => array( 'file' => 'popup.php', 'regions' => array(), 'options' => array('nofooter'=>true, 'nonavbar'=>true), ), Also since the updates are due any time now, it may be better to wait until they have been done before you embark on the new branches then you will be less likely to have rebase problems.
          Hide
          gb2048 Gareth J Barnard added a comment - - edited

          I've just done all the other two branches. Did not see the comment about 'popup.php' but did think about it at the time when doing 'master' but thought that the code would be required in the layout if a user removed "'nonavbar'=>true" from the theme configuration file. And that it is a separate issue to this one and therefore the Integrators take a dim view of shoehorning other changes in with other fixes in the same tracker, so not done.

          Show
          gb2048 Gareth J Barnard added a comment - - edited I've just done all the other two branches. Did not see the comment about 'popup.php' but did think about it at the time when doing 'master' but thought that the code would be required in the layout if a user removed "'nonavbar'=>true" from the theme configuration file. And that it is a separate issue to this one and therefore the Integrators take a dim view of shoehorning other changes in with other fixes in the same tracker, so not done.
          Hide
          lazydaisy Mary Evans added a comment -

          That's OK.

          Show
          lazydaisy Mary Evans added a comment - That's OK.
          Hide
          gb2048 Gareth J Barnard added a comment -

          Hi Mary Evans,

          Thanks. Please could you submit it for integration review.

          Cheers,

          Gareth

          Show
          gb2048 Gareth J Barnard added a comment - Hi Mary Evans , Thanks. Please could you submit it for integration review. Cheers, Gareth
          Hide
          lazydaisy Mary Evans added a comment - - edited

          Works OK too so setting for Integration Review

          Show
          lazydaisy Mary Evans added a comment - - edited Works OK too so setting for Integration Review
          Hide
          gb2048 Gareth J Barnard added a comment -

          Dear Mary Evans,

          Thank you for sending to integration.

          Cheers,

          Gareth

          Show
          gb2048 Gareth J Barnard added a comment - Dear Mary Evans , Thank you for sending to integration. Cheers, Gareth
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Thanks Gareth - this has been integrated now.

          Show
          samhemelryk Sam Hemelryk added a comment - Thanks Gareth - this has been integrated now.
          Hide
          phalacee Jason Fowler added a comment -

          All good gareth, thanks

          Show
          phalacee Jason Fowler added a comment - All good gareth, thanks
          Hide
          damyon Damyon Wiese added a comment -

          Twas the week before Christmas,
          And all though HQ
          Devs were scrambling to finish peer review.
          They sent all their issues,
          and rushed out the door -
          "To the beach!" someone heard them roar!

          This issue has been released upstream. Thanks!

          Show
          damyon Damyon Wiese added a comment - Twas the week before Christmas, And all though HQ Devs were scrambling to finish peer review. They sent all their issues, and rushed out the door - "To the beach!" someone heard them roar! This issue has been released upstream. Thanks!
          Hide
          markpearson Mark Pearson added a comment - - edited

          Christmas in Australia is Christmas in paradise,
          Christmas in Australia is basically bloody nice,
          Bruce is going steady with Sheila,
          and Sheila's going steady with Bruce,
          and if you haven't got a Christmas suntan,
          you're a Pommie and you ain't no use

          http://www.youtube.com/watch?v=TMkRpu8a0MY

          Enjoy your hols lads!

          Show
          markpearson Mark Pearson added a comment - - edited Christmas in Australia is Christmas in paradise, Christmas in Australia is basically bloody nice, Bruce is going steady with Sheila, and Sheila's going steady with Bruce, and if you haven't got a Christmas suntan, you're a Pommie and you ain't no use http://www.youtube.com/watch?v=TMkRpu8a0MY Enjoy your hols lads!

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                13/Jan/14