Moodle
  1. Moodle
  2. MDL-35816 Accessibility Review issues (Deque)
  3. MDL-35875

Re-write user information table in view user's profile to use semantic HTML

    Details

    • Testing Instructions:
      Hide

      Master:

      1. Check that the user profile page does not look broken (It doesn't have to look 100% identical to the old version).
      2. Change the language to hebrew - check the page again.

      Stables:

      1. Login
      2. Click the user name in the top right to view the profile page
      3. Inspect the code of the table, ensure the summary attribute is no longer there
      4. Click a link under the course profiles section of the user profile
      5. Inspect the code of the table, ensure the summary attribute is no longer there
      Show
      Master: Check that the user profile page does not look broken (It doesn't have to look 100% identical to the old version). Change the language to hebrew - check the page again. Stables: Login Click the user name in the top right to view the profile page Inspect the code of the table, ensure the summary attribute is no longer there Click a link under the course profiles section of the user profile Inspect the code of the table, ensure the summary attribute is no longer there
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
      wip-MDL-35875-stable24
    • Pull Master Branch:
      wip-MDL-35875-master-alt
    • Rank:
      44640

      Description

      Issue
      Table structure - The presentation table used to layout user information on the view user's profile page has an empty summary attribute - it shouldn't have the summary attribute at all. It would be best to use CSS layout instead of a table to present this data.

      Standard Level
      WCAG 2 1.3.1 (A) http://www.w3.org/WAI/WCAG20/quickref/#qr-content-structure-separation-programmatic

      Impact
      Serious

      Example Link
      http://demo.moodle.net/user/profile.php?id=0&sesskey=ZdAicfllpV&bui_moveid=293

      Test Steps

      1. Login as a student
      2. Click on the student
      3. Tab to the user's profile information table and allow a screen reader to read it.
      4. View the source of the page to view the summary attribute on the table
      1. 0001-MDL-35816-User-Modify-the-profile-page-to-use-semant.patch
        16 kB
        Jason Fowler
      1. 35875.png
        104 kB
      2. MDL-35875.png
        42 kB
      3. MDL-35875.png
        152 kB
      4. ProfilePage_new.png
        146 kB
      5. ProfilePage_old.png
        143 kB

        Issue Links

          Activity

          Hide
          Jason Fowler added a comment -

          Hi Jason,

          The description and the title kind of conflict - are you recommending removing the table in favour of DIVs or a descriptive list, or are you recommending the summary be removed and the table left as is?

          Show
          Jason Fowler added a comment - Hi Jason, The description and the title kind of conflict - are you recommending removing the table in favour of DIVs or a descriptive list, or are you recommending the summary be removed and the table left as is?
          Hide
          Jason Fowler added a comment -

          Personally, I would like to see the table removed in favour of something more friendly

          Show
          Jason Fowler added a comment - Personally, I would like to see the table removed in favour of something more friendly
          Hide
          Jason Hardin added a comment -

          Yes I would recommend divs over a table for the user profile presentation. None of the information on the page matches up with the use of a table and it can all be printed in a title: value format with a return line between each title value pair.

          Show
          Jason Hardin added a comment - Yes I would recommend divs over a table for the user profile presentation. None of the information on the page matches up with the use of a table and it can all be printed in a title: value format with a return line between each title value pair.
          Hide
          Jason Fowler added a comment -

          I was thinking more

          <div><span>Name:</span> Jason Fowler</div>
          

          would be fine ..

          Show
          Jason Fowler added a comment - I was thinking more <div><span>Name:</span> Jason Fowler</div> would be fine ..
          Hide
          Adrian Greeve added a comment -

          [Y] Syntax
          [N] Output
          [Y] Whitespace
          [-] Language
          [-] Databases
          [N] Testing
          [-] Security
          [-] Documentation
          [Y] Git
          [*] Sanity check

          Hi Jason,

          I just have a few things to mention about your patch:

          • At least user/view.php would also have to be changed (also has an empty summary element).
          • I think that perhaps a different file needs to be altered to change the style. (theme/base/style/user.css)
          • You've introduced a new variable ($table="true") into the funciton profile_display_fields() and then never used it. Perhaps you are missing an if statement somewhere.
          • You might also want to add a doc block to profile_display_fields and explain your new variable.
          • You should probably use moodle_url and html_writer::link to show urls where possible e.g.
            $aimurl = new moodle_url('aim:goim', array('screenname' => urldecode($user->aim)));
            echo html_writer::link($aimurl, s($user->aim));
            
          • No testing instructions.
          • I'm not really a huge fan of the information wrapping around the user profile picture, but that's just my personal preference.

          Thanks.

          Show
          Adrian Greeve added a comment - [Y] Syntax [N] Output [Y] Whitespace [-] Language [-] Databases [N] Testing [-] Security [-] Documentation [Y] Git [*] Sanity check Hi Jason, I just have a few things to mention about your patch: At least user/view.php would also have to be changed (also has an empty summary element). I think that perhaps a different file needs to be altered to change the style. (theme/base/style/user.css) You've introduced a new variable ($table="true") into the funciton profile_display_fields() and then never used it. Perhaps you are missing an if statement somewhere. You might also want to add a doc block to profile_display_fields and explain your new variable. You should probably use moodle_url and html_writer::link to show urls where possible e.g. $aimurl = new moodle_url('aim:goim', array('screenname' => urldecode($user->aim))); echo html_writer::link($aimurl, s($user->aim)); No testing instructions. I'm not really a huge fan of the information wrapping around the user profile picture, but that's just my personal preference. Thanks.
          Hide
          Jason Fowler added a comment -

          I added $table="true" then decided against it, must have forgotten to remove it after.

          I changed the css file I did because it was the one that already defined the areas I was changing.

          Other than that, I will implement the rest of the changes as described first thing in the morning. Thanks for the review.

          Show
          Jason Fowler added a comment - I added $table="true" then decided against it, must have forgotten to remove it after. I changed the css file I did because it was the one that already defined the areas I was changing. Other than that, I will implement the rest of the changes as described first thing in the morning. Thanks for the review.
          Hide
          Jason Fowler added a comment -

          Just tried re-writing the instant messenger links to use the moodle url and html writer - it breaks them all.

          Show
          Jason Fowler added a comment - Just tried re-writing the instant messenger links to use the moodle url and html writer - it breaks them all.
          Hide
          Jason Fowler added a comment -

          Pushing for integration now that all the other changes have been fixed

          Show
          Jason Fowler added a comment - Pushing for integration now that all the other changes have been fixed
          Hide
          Dan Poltawski 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
          Dan Poltawski 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
          Dan Poltawski added a comment -

          Reopening, sorry but this is sloppy.

          1. Please include testing instrucitons for all themes, and large amounts of content in the fields
          2. profile_display_fields still an additional argument passed to it, but seemingly you've removed that
          3. Since you are converting the rest to html_writer, remove the hardcoded '<div>' you are introducing
          4. {$webpageurl = new moodle_url(s($url))} is wrong. The input shouldn't be quoted.
          5. You should be able to convert most of the IM link/images to moodle_url and html_writer,
            please investigate the encoding properly and you'll be able to do it.
          Show
          Dan Poltawski added a comment - Reopening, sorry but this is sloppy. Please include testing instrucitons for all themes, and large amounts of content in the fields profile_display_fields still an additional argument passed to it, but seemingly you've removed that Since you are converting the rest to html_writer, remove the hardcoded '<div>' you are introducing {$webpageurl = new moodle_url(s($url))} is wrong. The input shouldn't be quoted. You should be able to convert most of the IM link/images to moodle_url and html_writer, please investigate the encoding properly and you'll be able to do it.
          Hide
          Jason Fowler added a comment -

          when I tried to do the IM links, they went from aim:phalacee to aim://phalacee - which doesn't work.

          Show
          Jason Fowler added a comment - when I tried to do the IM links, they went from aim:phalacee to aim://phalacee - which doesn't work.
          Hide
          Jason Fowler added a comment -

          just realised that multi-line profile details will totally break the layout of the page. will need to address this in CSS.

          Show
          Jason Fowler added a comment - just realised that multi-line profile details will totally break the layout of the page. will need to address this in CSS.
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Jason Fowler added a comment -

          Requesting a thorough peer review please.

          Show
          Jason Fowler added a comment - Requesting a thorough peer review please.
          Hide
          Frédéric Massart added a comment -

          Hi Jason, the patch looks good, here are some comments:

          • In general I don't like appending a colon to a string as it's not compliant with other languages. If we're going to change the HTML it might not be necessary to use those colons any more.
          • As mentioned by Dan, you can get away with any HTML using html_writer, but where you can't use html_writer::link(), I'd suggest to use html_writer::tag('a') (for AIM, Skype, etc...). Also <img> should be using html_writer::tag('img').
          • The MSN ID is usually an email right? I know we don't want a mailto: on that, but shouldn't we obfuscate it at well? Food for thoughts. See obfuscate_email().
          • user/view.php:294 Syntax error.
          • When the content of the <span> is getting long (IE other languages), the content is vertically aligned at the bottom, that does not look right.
          • ICQ, AIM and Skype icons should use the class 'icon' or 'iconsmall' depending on their size and use 'icon-post' for padding. Also any space in between the image and the text should be removed. I know you're not introducing this but we might as well fix it.
          • The description on the profile is not aligned with the avatar but is displayed underneath it.
          • Some custom profile fields (such as textarea) are not aligned as the other fields. (also add those to testing instructions)
          • There are still some places where HTML is used. IE you're closing 2 divs which were open without html_writer. When you'll convert them, could you comment next to the closing ones to identify which one you are closing? It'd getting really hard to understand what is closing what.
          • Can you confirm that setting up the size of 180px is fine if we reduce the window, and will fallback responsively if need be?

          The rest concerns accessibility and styling. Turning styles off to have an idea of what a screen reader does triggered my attention as the <span> is glued to the text and if we remove the colon, it will generate an unreadable word. Perhaps we could use <dd><dt><dl> for this. It's not tabular data so no table, but I think the semantic here is important or impaired users won't be able to get the information they're looking for. If the idea of using definitions is not good, then I think the <span> should be a <div> or non-inline element, and the content should be wrapped to allow for styling.

          Many thanks!
          Fred

          Show
          Frédéric Massart added a comment - Hi Jason, the patch looks good, here are some comments: In general I don't like appending a colon to a string as it's not compliant with other languages. If we're going to change the HTML it might not be necessary to use those colons any more. As mentioned by Dan, you can get away with any HTML using html_writer, but where you can't use html_writer::link(), I'd suggest to use html_writer::tag('a') (for AIM, Skype, etc...). Also <img> should be using html_writer::tag('img'). The MSN ID is usually an email right? I know we don't want a mailto: on that, but shouldn't we obfuscate it at well? Food for thoughts. See obfuscate_email(). user/view.php:294 Syntax error. When the content of the <span> is getting long (IE other languages), the content is vertically aligned at the bottom, that does not look right. ICQ, AIM and Skype icons should use the class 'icon' or 'iconsmall' depending on their size and use 'icon-post' for padding. Also any space in between the image and the text should be removed. I know you're not introducing this but we might as well fix it. The description on the profile is not aligned with the avatar but is displayed underneath it. Some custom profile fields (such as textarea) are not aligned as the other fields. (also add those to testing instructions) There are still some places where HTML is used. IE you're closing 2 divs which were open without html_writer. When you'll convert them, could you comment next to the closing ones to identify which one you are closing? It'd getting really hard to understand what is closing what. Can you confirm that setting up the size of 180px is fine if we reduce the window, and will fallback responsively if need be? The rest concerns accessibility and styling. Turning styles off to have an idea of what a screen reader does triggered my attention as the <span> is glued to the text and if we remove the colon, it will generate an unreadable word. Perhaps we could use <dd><dt><dl> for this. It's not tabular data so no table, but I think the semantic here is important or impaired users won't be able to get the information they're looking for. If the idea of using definitions is not good, then I think the <span> should be a <div> or non-inline element, and the content should be wrapped to allow for styling. Many thanks! Fred
          Hide
          Jason Fowler added a comment -

          Thanks for the feedback Fred.

          I'm not sure that responsive fallback is an issue in this case, that is something a responsive theme would need to handle, not the base or canvas themes.

          Show
          Jason Fowler added a comment - Thanks for the feedback Fred. I'm not sure that responsive fallback is an issue in this case, that is something a responsive theme would need to handle, not the base or canvas themes.
          Hide
          Jason Fowler added a comment -

          I think I have satisfied all the points you raised here Fred. Pushing for peer review again, just to make sure.

          Show
          Jason Fowler added a comment - I think I have satisfied all the points you raised here Fred. Pushing for peer review again, just to make sure.
          Hide
          Frédéric Massart added a comment - - edited

          Thanks for the patch Jason, it's looking good! Few comments here:

          • Could you add some testing instructions using each type of the standard profile fields?
          • All the src attributes could/should be a proper moodle_url.
          • The URLs should also be moodle_url's (ICQ, Yahoo, ...). (Except for the one using different protocols, ie: Skype, AIM, ...)
          • Wherever we used s() to escape the content which is now between <dd> you should keep the s() to make sure the user does not inject HTML/JS in here. I'm surprised that html_writer does not escape it by default, I never realised that! It only espaces attributes apparently.
          1. css:6: The description is still not aligned with the avatar.
          2. profile.php:318: Please remove the extra whitespace for this icon, especially now that you make use of icon and icon-pre.
          3. profile.php:404: Typo
          4. view.php:330 Why adding the email of the user here? Also the string suspended is not within the <dd> tag.

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - - edited Thanks for the patch Jason, it's looking good! Few comments here: Could you add some testing instructions using each type of the standard profile fields? All the src attributes could/should be a proper moodle_url. The URLs should also be moodle_url's (ICQ, Yahoo, ...). (Except for the one using different protocols, ie: Skype, AIM, ...) Wherever we used s() to escape the content which is now between <dd> you should keep the s() to make sure the user does not inject HTML/JS in here. I'm surprised that html_writer does not escape it by default, I never realised that! It only espaces attributes apparently. css:6: The description is still not aligned with the avatar. profile.php:318: Please remove the extra whitespace for this icon, especially now that you make use of icon and icon-pre . profile.php:404: Typo view.php:330 Why adding the email of the user here? Also the string suspended is not within the <dd> tag. Cheers, Fred
          Hide
          Jason Fowler added a comment -

          All those things are fixed now Fred. Don't really understand what you mean about the testing instructions though.

          Show
          Jason Fowler added a comment - All those things are fixed now Fred. Don't really understand what you mean about the testing instructions though.
          Hide
          Frédéric Massart added a comment - - edited

          Thanks Jason,

          had a very quick look at the patch, there is a mistake in the use of moodle_url().

          $imurl = new moodle_url('http://web.icq.com/wwp', array('uin'=>urlencode($user->icq)) );
          $iconurl = new moodle_url('http://web.icq.com/whitepages/online?icq='.urlencode($user->icq).'&img=5');
          

          $imurl: You must not encode the parameter passed to moodle_url(), the whole purpose of this class is to do it for you, so it's going to be done twice.
          $iconurl: Ideally you should not construct the URL this way, but pass both parameters to the class as you did on the line above.

          That does not only apply to ICQ.

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - - edited Thanks Jason, had a very quick look at the patch, there is a mistake in the use of moodle_url(). $imurl = new moodle_url('http://web.icq.com/wwp', array('uin'=>urlencode($user->icq)) ); $iconurl = new moodle_url('http://web.icq.com/whitepages/online?icq='.urlencode($user->icq).'&img=5'); $imurl : You must not encode the parameter passed to moodle_url(), the whole purpose of this class is to do it for you, so it's going to be done twice. $iconurl : Ideally you should not construct the URL this way, but pass both parameters to the class as you did on the line above. That does not only apply to ICQ. Cheers, Fred
          Hide
          Jason Fowler added a comment -

          Thanks for spotting that Fred, I totally missed it when I was going over it.

          Can you have another look at it now that I have pushed that change and let me know if it's fine.

          Show
          Jason Fowler added a comment - Thanks for spotting that Fred, I totally missed it when I was going over it. Can you have another look at it now that I have pushed that change and let me know if it's fine.
          Hide
          Frédéric Massart added a comment -

          Line 302 and 339 need to escape the data with s(), as it was previously.
          I haven't seen anything else.
          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - Line 302 and 339 need to escape the data with s(), as it was previously. I haven't seen anything else. Cheers, Fred
          Hide
          Jason Fowler added a comment -

          That should be it then. All cleaned up.

          Show
          Jason Fowler added a comment - That should be it then. All cleaned up.
          Hide
          Frédéric Massart added a comment -

          Hi Jason,

          Sorry for not spotting that before, but I just noticed that in the previously used function print_row(), we were setting 4 other classes: c0, c1, label and info. Do you think we should keep them just in case some themes used them? Also, I just realised that we could have used this function instead of using html_writer on every single line. And if we're not using it any more, we might as well remove it as it's unlikely that it would be used anywhere else...

          Please comment on the issue and take the actions you think are the best.

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - Hi Jason, Sorry for not spotting that before, but I just noticed that in the previously used function print_row(), we were setting 4 other classes: c0, c1, label and info. Do you think we should keep them just in case some themes used them? Also, I just realised that we could have used this function instead of using html_writer on every single line. And if we're not using it any more, we might as well remove it as it's unlikely that it would be used anywhere else... Please comment on the issue and take the actions you think are the best. Cheers, Fred
          Hide
          Jason Fowler added a comment -

          print_row is for tables, removing the function was a result of taking it out of a table layout. I will check to see if it is used else where, and either deprecate it or remove it.

          I have re-written the CSS to compensate for the removal of the c0 c1 label and info classes.

          Show
          Jason Fowler added a comment - print_row is for tables, removing the function was a result of taking it out of a table layout. I will check to see if it is used else where, and either deprecate it or remove it. I have re-written the CSS to compensate for the removal of the c0 c1 label and info classes.
          Hide
          Jason Fowler added a comment -

          it's not used anywhere else in the code, and it won't be possible to use it outside of the pages it resided on, so I have removed it.

          Pushing for integration now.

          Show
          Jason Fowler added a comment - it's not used anywhere else in the code, and it won't be possible to use it outside of the pages it resided on, so I have removed it. Pushing for integration now.
          Hide
          Damyon Wiese added a comment -

          Hi Jason,

          The changes look fine, the only thing that is missing is some comments in theme/upgrade.txt describing this change for themers.

          Show
          Damyon Wiese added a comment - Hi Jason, The changes look fine, the only thing that is missing is some comments in theme/upgrade.txt describing this change for themers.
          Hide
          Damyon Wiese added a comment -

          This will have to wait until next week - Jason can you add the extra information for themers and resubmit?

          Thanks, Damyon

          Show
          Damyon Wiese added a comment - This will have to wait until next week - Jason can you add the extra information for themers and resubmit? Thanks, Damyon
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Jason Fowler added a comment -

          Added the extra information now. Will rebase after the weekly release, ready for integration next week.

          Show
          Jason Fowler added a comment - Added the extra information now. Will rebase after the weekly release, ready for integration next week.
          Hide
          Damyon Wiese 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.

          Thanks!

          Show
          Damyon Wiese 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. Thanks!
          Hide
          Jason Fowler added a comment -

          rebased

          Show
          Jason Fowler added a comment - rebased
          Hide
          Damyon Wiese added a comment -

          Thanks Jason(s).

          This change has been pushed to master now.

          (This would have been a golden opportunity to add a renderer for this - keep this in mind in future.)

          Show
          Damyon Wiese added a comment - Thanks Jason(s). This change has been pushed to master now. (This would have been a golden opportunity to add a renderer for this - keep this in mind in future.)
          Hide
          Rajesh Taneja added a comment -

          Sorry guys I think it need a second look.
          Two things which I noted:

          1. Course are shown on second line
          2. Image is not placed next to table.

          Attaching image for more information. Please feel free to pass if you think this is fine.

          Show
          Rajesh Taneja added a comment - Sorry guys I think it need a second look. Two things which I noted: Course are shown on second line Image is not placed next to table. Attaching image for more information. Please feel free to pass if you think this is fine.
          Hide
          Jason Fowler added a comment -

          the image is next to the details if the details are small enough, I am guessing it got moved as a result of the course list being so long. Will have a look at what can be done to solve the issue. Thanks Raj

          Show
          Jason Fowler added a comment - the image is next to the details if the details are small enough, I am guessing it got moved as a result of the course list being so long. Will have a look at what can be done to solve the issue. Thanks Raj
          Hide
          Jason Fowler added a comment -

          pushed a new commit to fix the issue in testing.

          Show
          Jason Fowler added a comment - pushed a new commit to fix the issue in testing.
          Hide
          Frédéric Massart added a comment -

          Sorry for forgetting to mention this before, but while testing we should consider IE and RTL.

          Show
          Frédéric Massart added a comment - Sorry for forgetting to mention this before, but while testing we should consider IE and RTL.
          Hide
          Damyon Wiese added a comment -

          Missing RTL support - not pulling in the change.

          Show
          Damyon Wiese added a comment - Missing RTL support - not pulling in the change.
          Hide
          Jason Fowler added a comment -

          All changes for RTL have been included now.

          Show
          Jason Fowler added a comment - All changes for RTL have been included now.
          Hide
          Damyon Wiese added a comment -

          Thanks Jason,

          Extra patches have been pulled in for RTL fixes. This is ready for re-testing.

          Show
          Damyon Wiese added a comment - Thanks Jason, Extra patches have been pulled in for RTL fixes. This is ready for re-testing.
          Hide
          Damyon Wiese added a comment -

          The test has failed due to layout issues with the CSS on different screen sizes.

          I will reopen this issue for more work.

          Show
          Damyon Wiese added a comment - The test has failed due to layout issues with the CSS on different screen sizes. I will reopen this issue for more work.
          Hide
          Damyon Wiese added a comment -

          Reopening for more work on the CSS layout or an alternative solution.

          Show
          Damyon Wiese added a comment - Reopening for more work on the CSS layout or an alternative solution.
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Jason Fowler added a comment - - edited

          Finding it impossible to get this up to Raj's high standards of backwards compatibility. The code has been rewritten, and I get it looking right in high res screens, but at 1024x768 it collapses, and apparently that isn't acceptable.

          Putting this on hold for the week, will look at it in the joint scrum on Tuesday, see if someone else can think of something I can do to make it work nicely.

          Show
          Jason Fowler added a comment - - edited Finding it impossible to get this up to Raj's high standards of backwards compatibility. The code has been rewritten, and I get it looking right in high res screens, but at 1024x768 it collapses, and apparently that isn't acceptable. Putting this on hold for the week, will look at it in the joint scrum on Tuesday, see if someone else can think of something I can do to make it work nicely.
          Hide
          Jason Fowler added a comment -

          Solved it.
          Note to self, with CSS, Less is More.

          Show
          Jason Fowler added a comment - Solved it. Note to self, with CSS, Less is More.
          Hide
          Sam Hemelryk added a comment -

          Hi Jason,

          I really like how this is looking, its a huge improvement over how things were.
          There is just one little niggly thing that I've spotted that I think should be fixed.
          If you have a look at the attachment (35875.png) that I've added now you will notice that elements next to the image do not have any space between the title and the value. Its the same for fields with extra long titles.
          I think really we need some spacing there.
          Otherwise it looks really good thanks.

          Show
          Sam Hemelryk added a comment - Hi Jason, I really like how this is looking, its a huge improvement over how things were. There is just one little niggly thing that I've spotted that I think should be fixed. If you have a look at the attachment (35875.png) that I've added now you will notice that elements next to the image do not have any space between the title and the value. Its the same for fields with extra long titles. I think really we need some spacing there. Otherwise it looks really good thanks.
          Hide
          Sam Hemelryk added a comment -

          (I'll leave this as review in progress to give you time to address it/reply btw)

          Show
          Sam Hemelryk added a comment - (I'll leave this as review in progress to give you time to address it/reply btw)
          Hide
          Rajesh Taneja added a comment -

          FYI: Jason is not in today.

          Show
          Rajesh Taneja added a comment - FYI: Jason is not in today.
          Hide
          Sam Hemelryk added a comment -

          Aha thanks Raj, as this is a master only issue I'm happy to reopen as its not crucial it lands this week.
          Jason when you are back could you please address that one spacing issue at which point I think this is safe to land.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Aha thanks Raj, as this is a master only issue I'm happy to reopen as its not crucial it lands this week. Jason when you are back could you please address that one spacing issue at which point I think this is safe to land. Many thanks Sam
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Jason Fowler added a comment -

          Thanks Sam, Raj. That could be a very hard issue to solve, but I will give it a shot.

          Show
          Jason Fowler added a comment - Thanks Sam, Raj. That could be a very hard issue to solve, but I will give it a shot.
          Hide
          Jason Fowler added a comment -

          spacing issue fixed now.

          Show
          Jason Fowler added a comment - spacing issue fixed now.
          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
          Aparup Banerjee added a comment -

          Many iterations and reviews and scrutiny later - integrated into master!

          (updating test to use as many different types of fields as suggested by Fred)

          Show
          Aparup Banerjee added a comment - Many iterations and reviews and scrutiny later - integrated into master! (updating test to use as many different types of fields as suggested by Fred)
          Hide
          Rajesh Taneja added a comment -

          Sorry Jason,

          This fails again, any custom field name which warps on two lines is shifting following field to right.
          Screen shot attached MDL-35875.png

          Show
          Rajesh Taneja added a comment - Sorry Jason, This fails again, any custom field name which warps on two lines is shifting following field to right. Screen shot attached MDL-35875 .png
          Hide
          Aparup Banerjee added a comment -

          Thanks Raj. the long field names does create havoc. will reverting this tomorrow if Jason can't patch it by then.

          Show
          Aparup Banerjee added a comment - Thanks Raj. the long field names does create havoc. will reverting this tomorrow if Jason can't patch it by then.
          Hide
          Jason Fowler added a comment -

          Revert it, and I will sort it out. It will take a while to figure this one out.

          If I can't solve it, or it fails again, I will look at an whole new solution.

          Show
          Jason Fowler added a comment - Revert it, and I will sort it out. It will take a while to figure this one out. If I can't solve it, or it fails again, I will look at an whole new solution.
          Hide
          Jason Fowler added a comment -

          After working on this for over a month, I concede defeat in regards to removing the table in favour of semantic html structures. There is no way to anticipate and accommodate all the variables users will face with this page.

          I am reverting the issue back to its original intent, and simply removing the summary attribute from the table.

          Show
          Jason Fowler added a comment - After working on this for over a month, I concede defeat in regards to removing the table in favour of semantic html structures. There is no way to anticipate and accommodate all the variables users will face with this page. I am reverting the issue back to its original intent, and simply removing the summary attribute from the table.
          Hide
          Jason Fowler added a comment -

          As soon as this is re-opened, it can be put up for peer review. I have created a new branch (rather than replace the one all that hard work went in to) and that contains the summary removal patch.

          Show
          Jason Fowler added a comment - As soon as this is re-opened, it can be put up for peer review. I have created a new branch (rather than replace the one all that hard work went in to) and that contains the summary removal patch.
          Hide
          Aparup Banerjee added a comment -

          Jason, thats been reverted from integration and this is reopened now.

          ps: i did like the original direction.
          pps: watch for mistakes in your commit messages.

          Show
          Aparup Banerjee added a comment - Jason, thats been reverted from integration and this is reopened now. ps: i did like the original direction. pps: watch for mistakes in your commit messages.
          Hide
          Frédéric Massart added a comment - - edited

          I don't think you can change the summary of an issue because a part of it appeared as not fixable. Also the description states: _ It would be best to use CSS layout instead of a table to present this data_, and I think it's a valid point and we should address it.

          Perhaps my suggestion of using Definition Lists ( <dl> ) was not the best, but if we go away from tables then we should definitely have a valid semantic. If anything we don't want to use proper tags but stick with the tables I guess we could make use of aria attributes describing what defines what.

          And if starting from scratch, why not make a renderer out of this. After all the time spent by everyone involved in this issue, I'd find it a bit of a shame if we're going for a quick fix.

          Show
          Frédéric Massart added a comment - - edited I don't think you can change the summary of an issue because a part of it appeared as not fixable. Also the description states: _ It would be best to use CSS layout instead of a table to present this data_, and I think it's a valid point and we should address it. Perhaps my suggestion of using Definition Lists ( <dl> ) was not the best, but if we go away from tables then we should definitely have a valid semantic. If anything we don't want to use proper tags but stick with the tables I guess we could make use of aria attributes describing what defines what. And if starting from scratch, why not make a renderer out of this. After all the time spent by everyone involved in this issue, I'd find it a bit of a shame if we're going for a quick fix.
          Hide
          Jason Fowler added a comment -

          If you look at the history of the issue, it was originally about the empty summary attribute in the table. I modified the summary of the issue based on the fact that I personally would have liked it re-written to not use a table at all. That didn't work. So I am going back to the original intention of the issue.

          Can you please peer review the code for the issue as it stands. Discussions with Damyon and Michael have led me to believe I am wasting time trying to get a different html structure in place here when removing the summary of the table satisfies the accessibility requirements of the issue.

          Show
          Jason Fowler added a comment - If you look at the history of the issue, it was originally about the empty summary attribute in the table. I modified the summary of the issue based on the fact that I personally would have liked it re-written to not use a table at all. That didn't work. So I am going back to the original intention of the issue. Can you please peer review the code for the issue as it stands. Discussions with Damyon and Michael have led me to believe I am wasting time trying to get a different html structure in place here when removing the summary of the table satisfies the accessibility requirements of the issue.
          Hide
          Frédéric Massart added a comment -

          The code is perfect then . I'd consider a backport as it's not an HTML change any more. Cheers!

          Show
          Frédéric Massart added a comment - The code is perfect then . I'd consider a backport as it's not an HTML change any more. Cheers!
          Hide
          Jason Fowler added a comment -

          Attaching code from previous iteration of the solution incase anyone wants to give it a crack, or if I decide to come back to this as a dev project.

          Show
          Jason Fowler added a comment - Attaching code from previous iteration of the solution incase anyone wants to give it a crack, or if I decide to come back to this as a dev project.
          Hide
          Jason Fowler added a comment -

          back ported and ready for integration.

          Show
          Jason Fowler added a comment - back ported and ready for integration.
          Hide
          Frédéric Massart added a comment -

          Bootstrap has solved this issue. They add a text-overflow on the DT, which means that it'll be cropped, but I think it's fair. See Definition under http://twitter.github.com/bootstrap/base-css.html#typography.

          .dl-horizontal dt {
            float: left;
            width: 160px;
            overflow: hidden;
            clear: left;
            text-align: right;
            text-overflow: ellipsis;
            white-space: nowrap;
          }
          
          .dl-horizontal dd {
            margin-left: 180px;
          }
          
          Show
          Frédéric Massart added a comment - Bootstrap has solved this issue. They add a text-overflow on the DT, which means that it'll be cropped, but I think it's fair. See Definition under http://twitter.github.com/bootstrap/base-css.html#typography . .dl-horizontal dt { float: left; width: 160px; overflow: hidden; clear: left; text-align: right; text-overflow: ellipsis; white-space: nowrap; } .dl-horizontal dd { margin-left: 180px; }
          Hide
          Jason Fowler added a comment -

          the dt can't clear left, else it all sits under the image, while the dd sits beside the image still. that was the point that caused me to revert this issue to the previous summary.

          Show
          Jason Fowler added a comment - the dt can't clear left, else it all sits under the image, while the dd sits beside the image still. that was the point that caused me to revert this issue to the previous summary.
          Hide
          Frédéric Massart added a comment -

          It clears left to prevent incorrect behaviour when a DD is missing. I think it can be omitted.

          Show
          Frédéric Massart added a comment - It clears left to prevent incorrect behaviour when a DD is missing. I think it can be omitted.
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Jason Fowler added a comment -

          but then we will still get the staggering effect as shown in Raj's last test ... and that is the problem I can't over come at the moment.

          Show
          Jason Fowler added a comment - but then we will still get the staggering effect as shown in Raj's last test ... and that is the problem I can't over come at the moment.
          Hide
          Frédéric Massart added a comment -

          Sorry for my insistence Jason, but you're too close! https://github.com/FMCorz/moodle/compare/MDL-35875-master

          Show
          Frédéric Massart added a comment - Sorry for my insistence Jason, but you're too close! https://github.com/FMCorz/moodle/compare/MDL-35875-master
          Hide
          Jason Fowler added a comment -

          Pushing Fred's patch as a new commit on top of my original work, from his repo - because of the reversions etc, mine is now munted again.

          Show
          Jason Fowler added a comment - Pushing Fred's patch as a new commit on top of my original work, from his repo - because of the reversions etc, mine is now munted again.
          Hide
          Frédéric Massart added a comment -

          Why is this issue renamed to "Rewrite course completion block" ?!

          Show
          Frédéric Massart added a comment - Why is this issue renamed to "Rewrite course completion block" ?!
          Hide
          Jason Fowler added a comment -

          I grabbed the wrong line from the autocomplete drop down ... ooops

          Show
          Jason Fowler added a comment - I grabbed the wrong line from the autocomplete drop down ... ooops
          Hide
          Jason Fowler added a comment -

          Please note the commit message for the first commit is still wrong, it should have the MDL# for this issue, not the parent.

          Show
          Jason Fowler added a comment - Please note the commit message for the first commit is still wrong, it should have the MDL# for this issue, not the parent.
          Hide
          Damyon Wiese added a comment -

          Hi Jason,

          Sending this back again on 2 counts:

          1. You changed the repo to Freds and put in some confusing comments - so I need you to confirm that Freds branch is the one you wanted to submit.

          2. There are now no stable branches for this issue.

          (Sorry for sending this back again!)

          I checked the code for master on Freds branch and all looks is fine for integration so it's just the backports thats missing.

          Show
          Damyon Wiese added a comment - Hi Jason, Sending this back again on 2 counts: 1. You changed the repo to Freds and put in some confusing comments - so I need you to confirm that Freds branch is the one you wanted to submit. 2. There are now no stable branches for this issue. (Sorry for sending this back again!) I checked the code for master on Freds branch and all looks is fine for integration so it's just the backports thats missing.
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Jason Fowler added a comment -

          Okay, I have now added two branches for the back port - these just remove the summary attribute. The master branch solves the bigger problem.

          Show
          Jason Fowler added a comment - Okay, I have now added two branches for the back port - these just remove the summary attribute. The master branch solves the bigger problem.
          Hide
          Damyon Wiese added a comment -

          This has been integrated to 23, 24 and master now.

          Thanks Jason (Hurray!)

          Show
          Damyon Wiese added a comment - This has been integrated to 23, 24 and master now. Thanks Jason (Hurray!)
          Hide
          Rossiani Wijaya added a comment -

          Hi Jason,

          Sorry for failing this issue.

          When testing this with large font set from the browser, the description label is unreadable (eg: first ac...).

          Attaching screenshot for the issue.

          Show
          Rossiani Wijaya added a comment - Hi Jason, Sorry for failing this issue. When testing this with large font set from the browser, the description label is unreadable (eg: first ac...). Attaching screenshot for the issue.
          Hide
          Jason Fowler added a comment -

          Well, all avenues for making this a definition list have been exhausted. It seems it will stay a table for the time being. I have updated the master branch and title of the issue to deal with the original concern and not the "would be nice" aspect of it. There is a patch attached that will help the brave soul who tries to tackle this in the future...

          Show
          Jason Fowler added a comment - Well, all avenues for making this a definition list have been exhausted. It seems it will stay a table for the time being. I have updated the master branch and title of the issue to deal with the original concern and not the "would be nice" aspect of it. There is a patch attached that will help the brave soul who tries to tackle this in the future...
          Hide
          Frédéric Massart added a comment -

          I vote for keeping this in, I just tested Gmail with a larger font size and it does not support it badly but that's not very good either. Beside, you have to go into Firefox preferences > Content > Advanced > Minimum size... I think if you change that you have to expect that most websites won't appear as good as you'd hope. A theme can always enlarge the dt width. I don't think we will ever find solution that fit every need.

          PS: Using CTRL+ works as expected (Chrome, Firefox)

          Show
          Frédéric Massart added a comment - I vote for keeping this in, I just tested Gmail with a larger font size and it does not support it badly but that's not very good either. Beside, you have to go into Firefox preferences > Content > Advanced > Minimum size... I think if you change that you have to expect that most websites won't appear as good as you'd hope. A theme can always enlarge the dt width. I don't think we will ever find solution that fit every need. PS: Using CTRL+ works as expected (Chrome, Firefox)
          Hide
          Frédéric Massart added a comment -

          (Stop changing the issue summary >_<!)

          Show
          Frédéric Massart added a comment - (Stop changing the issue summary >_<!)
          Hide
          Damyon Wiese added a comment -

          Integrator vote has decided that this issue is an improvement and the font size issue should be dealt with separately.

          The reported issue will only affect master so stables are not a problem.

          Show
          Damyon Wiese added a comment - Integrator vote has decided that this issue is an improvement and the font size issue should be dealt with separately. The reported issue will only affect master so stables are not a problem.
          Hide
          Damyon Wiese added a comment -

          Passing test re integrator vote.

          Show
          Damyon Wiese added a comment - Passing test re integrator vote.
          Hide
          Aparup Banerjee added a comment -

          yay!

          Show
          Aparup Banerjee added a comment - yay!
          Hide
          Rajesh Taneja added a comment -

          I think we should reconsider this, fixing one accessibility will open another accessibility issue with probably more problems.
          http://support.mozilla.org/en-US/kb/accessibility-features-firefox-make-firefox-and-we#w_setting-a-minimum-font-size

          A custom profile field which is longer then 15/20 alphabets will truncate and probably not display enough information and as we support different languages we should consider average name of custom profile field name.

          Show
          Rajesh Taneja added a comment - I think we should reconsider this, fixing one accessibility will open another accessibility issue with probably more problems. http://support.mozilla.org/en-US/kb/accessibility-features-firefox-make-firefox-and-we#w_setting-a-minimum-font-size A custom profile field which is longer then 15/20 alphabets will truncate and probably not display enough information and as we support different languages we should consider average name of custom profile field name.
          Hide
          Jason Fowler added a comment -

          Thanks guys, will raise an issue and take it on myself to deal with the font size, although I doubt there is a way to control what the browser over rides...

          Show
          Jason Fowler added a comment - Thanks guys, will raise an issue and take it on myself to deal with the font size, although I doubt there is a way to control what the browser over rides...
          Hide
          Frédéric Massart added a comment -

          It's going to be difficult as soon as we set an overflow... but if we want to move away from tabular data, but still have a correct semantic I'd be curious to know the alternative.

          Show
          Frédéric Massart added a comment - It's going to be difficult as soon as we set an overflow... but if we want to move away from tabular data, but still have a correct semantic I'd be curious to know the alternative.
          Hide
          Jason Fowler added a comment -

          Raj, at the end of the day, 80% of Moodle is developed and tested without increasing font sizes, in terms of accessibility, zoom works, the code is more semantic, and the text is still present within the DOM, regardless of what is displayed on screen. I will consider adding a title attribute as a compromise, but at the end of the day, this is an improvement over what was there to begin with.

          Show
          Jason Fowler added a comment - Raj, at the end of the day, 80% of Moodle is developed and tested without increasing font sizes, in terms of accessibility, zoom works, the code is more semantic, and the text is still present within the DOM, regardless of what is displayed on screen. I will consider adding a title attribute as a compromise, but at the end of the day, this is an improvement over what was there to begin with.
          Hide
          Rajesh Taneja added a comment -

          Consider having a custom profile fields with name
          "You are now citizen of:"
          "Ahora eres ciudadano de"
          "أنت الآن مواطن"
          "Դուք այժմ քաղաքացին"

          Not sure if we should cater for more accessibility or normal people.

          Show
          Rajesh Taneja added a comment - Consider having a custom profile fields with name "You are now citizen of:" "Ahora eres ciudadano de" "أنت الآن مواطن" "Դուք այժմ քաղաքացին" Not sure if we should cater for more accessibility or normal people.
          Hide
          Jason Fowler added a comment -

          The thing is, the cut of was designed to do exactly what it is doing Raj. That was the purpose of it. Fred looked into it, and put forward that solution with this in mind.

          Show
          Jason Fowler added a comment - The thing is, the cut of was designed to do exactly what it is doing Raj. That was the purpose of it. Fred looked into it, and put forward that solution with this in mind.
          Hide
          Damyon Wiese added a comment -

          Thanks for your hard work. This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads).

          Show
          Damyon Wiese added a comment - Thanks for your hard work. This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads).
          Hide
          David Scotson added a comment -

          It would seem that the styles for this were taken (at least partly) from Bootstrap. I'm working on a theme that pulls in the standard Bootstrap CSS so it would have made things easier for me if you had also used the standard Bootstrap CSS names (i.e. in this case adding a class of "dl-horizontal" to the dl tag).

          This would have the added benefit (when the responsive Bootstrap CSS is present) of automatically reformatting the display on mobile devices such as iPhones by stacking the items vertically. (This issue is referred to in comments above around 24th Jan between Frederic and Jason).

          I can and have fixed this in the Bootstrap theme anyway by duplicating the Bootstrap styles and attaching them to the Moodle classnames, but a) I had to noticed this and b) I had to actually do some work to fix things, c) I had to duplicate some CSS that was already present in the theme. It would have been a pleasant surprise if you'd used the standard Bootstrap CSS conventions as well as their styles and it had all just magically worked.

          Show
          David Scotson added a comment - It would seem that the styles for this were taken (at least partly) from Bootstrap. I'm working on a theme that pulls in the standard Bootstrap CSS so it would have made things easier for me if you had also used the standard Bootstrap CSS names (i.e. in this case adding a class of "dl-horizontal" to the dl tag). This would have the added benefit (when the responsive Bootstrap CSS is present) of automatically reformatting the display on mobile devices such as iPhones by stacking the items vertically. (This issue is referred to in comments above around 24th Jan between Frederic and Jason). I can and have fixed this in the Bootstrap theme anyway by duplicating the Bootstrap styles and attaching them to the Moodle classnames, but a) I had to noticed this and b) I had to actually do some work to fix things, c) I had to duplicate some CSS that was already present in the theme. It would have been a pleasant surprise if you'd used the standard Bootstrap CSS conventions as well as their styles and it had all just magically worked.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: