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

Login error message text presented in red only. An icon is also needed.

    Details

    • Testing Instructions:
      Hide
      1. Go to the login page
      2. Enter a username and no password
      3. Click the login button
      4. See the icon appear in the error message
      5. Inspect the icon in a webdev tool like firebug
      6. Make sure the title attrib is absent
      7. Make sure the alt attrib contains "Error" (if using English as your language)
      Show
      Go to the login page Enter a username and no password Click the login button See the icon appear in the error message Inspect the icon in a webdev tool like firebug Make sure the title attrib is absent Make sure the alt attrib contains "Error" (if using English as your language)
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-35847-master
    • Rank:
      44611

      Description

      Issue
      Sensory characteristics - Error messaging for logging in is presented in red only. Some users will not be able to perceive this error message as such based only on the text color. Message should also be supported through another means (ex. An icon before the text).

      Color use - Error messaging for logging in is presented in red only. Some users will not be able to perceive this error message as such based only on the text color. Message should also be supported through another means (ex. An icon before the text).

      Standard Level
      WCAG 2 1.3.3 (A) http://www.w3.org/WAI/WCAG20/quickref/#qr-content-structure-separation-understanding
      WCAG 2 1.4.1 (A) http://www.w3.org/WAI/WCAG20/quickref/#qr-visual-audio-contrast-without-color

      Impact
      Serious

      Example Link
      http://demo.moodle.net/

      Test Steps

      1. Enter teacher username in the user name field
      2. Enter incorrect password
      3. Click the login button
      4. Error message does not say error or have an icon that has an alt text error so a user only has the red color to know it is an error.
      1. warning.svg
        0.9 kB
        Barbara Ramiro
      1. warning.png
        0.3 kB

        Activity

        Hide
        Michael de Raadt added a comment -

        I mentioned this on another issue. Perhaps we should add an optional image with the notification function.

        Show
        Michael de Raadt added a comment - I mentioned this on another issue. Perhaps we should add an optional image with the notification function.
        Hide
        Barbara Ramiro added a comment -

        Jason, attached warning.png and warning.svg icons in gray. Should be in pix/i. Please consult Fred for the icon class he used for all the new icons for consistency.

        Cheers

        Show
        Barbara Ramiro added a comment - Jason, attached warning.png and warning.svg icons in gray. Should be in pix/i. Please consult Fred for the icon class he used for all the new icons for consistency. Cheers
        Hide
        Jason Fowler added a comment -

        Thanks Barbara, wonderful work as always

        Show
        Jason Fowler added a comment - Thanks Barbara, wonderful work as always
        Hide
        Jason Fowler added a comment -

        Submitted as two commits, one for Barbara's icons, one for the code changes.

        Show
        Jason Fowler added a comment - Submitted as two commits, one for Barbara's icons, one for the code changes.
        Hide
        Barbara Ramiro added a comment -

        Thanks Jason =)

        Show
        Barbara Ramiro added a comment - Thanks Jason =)
        Hide
        Frédéric Massart added a comment - - edited

        Hi Jason, here are my comments:

        1. Should we really add a new parameter $icon to this function? I know that the purpose of this issue is to add an icon to the login page, but to me that would beneficial for any other error message. Also, wouldn't we want the renderer to take care of whether an icon is present or not? With this option I'm scared that developers would inconsistently use the icon or not. And if the parameter is needed, perhaps setting its default value to true is ideal. What are your thoughts?
        2. You should translate the alt attribute of the icon. It should also start with a capital letter.
        3. I don't think you should overwrite the title for it to be empty. I understand you don't want to duplicate it, but this is an issue coming from pix_icon itself and we should probably fix it there rather than manually here and there.
        4. I think the commit message for Barbara's should not be linked to the login issue. This icon will be generic and used anywhere we need it. As the icon will be generic I'd use 'Usability' as component which is the one we used for 2.4 icons.
        5. Could you comment on the reason why you didn't branch for 2.3 (I understand, just leave a message for the integration team).
        6. You're missing some testing instructions.

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

        Cheers!
        Fred

        Show
        Frédéric Massart added a comment - - edited Hi Jason, here are my comments: Should we really add a new parameter $icon to this function? I know that the purpose of this issue is to add an icon to the login page, but to me that would beneficial for any other error message. Also, wouldn't we want the renderer to take care of whether an icon is present or not? With this option I'm scared that developers would inconsistently use the icon or not. And if the parameter is needed, perhaps setting its default value to true is ideal. What are your thoughts? You should translate the alt attribute of the icon. It should also start with a capital letter. I don't think you should overwrite the title for it to be empty. I understand you don't want to duplicate it, but this is an issue coming from pix_icon itself and we should probably fix it there rather than manually here and there. I think the commit message for Barbara's should not be linked to the login issue. This icon will be generic and used anywhere we need it. As the icon will be generic I'd use 'Usability' as component which is the one we used for 2.4 icons. Could you comment on the reason why you didn't branch for 2.3 (I understand, just leave a message for the integration team). You're missing some testing instructions. [Y] Syntax [Y] Output [Y] Whitespace [N] Language [-] Databases [N] Testing [-] Security [-] Documentation [?] Git [Y] Sanity check Cheers! Fred
        Hide
        Jason Fowler added a comment -
        1. The param is there to make it optional, there are a few places this function is used, and it may not always be ideal to have the icon there. Much more research would need to be done before it can be made the default setting.
        2. Forgot that, yes, a string would be needed
        3. The title isn't needed. Fixing the pix_icon problem is a different issue altogether, and should be handled as such.
        4. What should I put for the icon issue on Barbara's commit then? The icons were created as part of this issue. They belong here.
        5. I didn't branch for 2.3 because of the icon changes that are only present in 2.4+
        6. Will write them as soon as I have finished the string for the ALT
        Show
        Jason Fowler added a comment - The param is there to make it optional, there are a few places this function is used, and it may not always be ideal to have the icon there. Much more research would need to be done before it can be made the default setting. Forgot that, yes, a string would be needed The title isn't needed. Fixing the pix_icon problem is a different issue altogether, and should be handled as such. What should I put for the icon issue on Barbara's commit then? The icons were created as part of this issue. They belong here. I didn't branch for 2.3 because of the icon changes that are only present in 2.4+ Will write them as soon as I have finished the string for the ALT
        Hide
        Jason Fowler added a comment -

        Changed the error alt text to a get_string call

        Show
        Jason Fowler added a comment - Changed the error alt text to a get_string call
        Hide
        Frédéric Massart added a comment -

        1/ I understand but creating the parameter for the only purpose of using it on the login page (for now) does not really convince me, however it's up to you.
        3/ I know it should be handled in a different issue, I'm just saying that you should maybe not use a workaround.
        4/ MDL-35847 usability: New warning icon

        Show
        Frédéric Massart added a comment - 1/ I understand but creating the parameter for the only purpose of using it on the login page (for now) does not really convince me, however it's up to you. 3/ I know it should be handled in a different issue, I'm just saying that you should maybe not use a workaround. 4/ MDL-35847 usability: New warning icon
        Hide
        Damyon Wiese added a comment -

        -1 for the icon parameter. This usability issue would affect everywhere error_text is called so they should all get the icon.

        Show
        Damyon Wiese added a comment - -1 for the icon parameter. This usability issue would affect everywhere error_text is called so they should all get the icon.
        Hide
        Jason Fowler added a comment - - edited

        Okay, I'll remove the icon, as Damyon and Fred have advised, and make this a master only change then.

        Show
        Jason Fowler added a comment - - edited Okay, I'll remove the icon, as Damyon and Fred have advised, and make this a master only change then.
        Hide
        Dan Poltawski added a comment -

        Yep, I also agree. Reopening for that. Thanks Jason

        Show
        Dan Poltawski added a comment - Yep, I also agree. Reopening for that. Thanks Jason
        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 -

        Done.

        Show
        Jason Fowler added a comment - Done.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Integrated (master only), thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Integrated (master only), thanks!
        Hide
        Jason Fowler added a comment -

        Eloy! the param for $icon that was removed, but I left the doc block in for it, I have now updated it with a new commit.

        Show
        Jason Fowler added a comment - Eloy! the param for $icon that was removed, but I left the doc block in for it, I have now updated it with a new commit.
        Hide
        Dan Poltawski added a comment -

        Pulled that fix in.

        Show
        Dan Poltawski added a comment - Pulled that fix in.
        Hide
        Rossiani Wijaya added a comment -

        This is working as expected.

        Test it for master only.

        Test passed.

        Show
        Rossiani Wijaya added a comment - This is working as expected. Test it for master only. Test passed.
        Hide
        Damyon Wiese added a comment -

        Congratulations! This issue has been resolved. Thanks for helping to make Moodle better for everyone!

        Regards, Damyon

        Show
        Damyon Wiese added a comment - Congratulations! This issue has been resolved. Thanks for helping to make Moodle better for everyone! Regards, Damyon

          People

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

            Dates

            • Created:
              Updated:
              Resolved: