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 Master Branch:
      wip-MDL-35875-master-alt

      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

        Gliffy Diagrams

        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: