1229206 - Labels for screen readers are missing from about:preferences#sync

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P3)

Description

[Edwin Wong [:edwong]]

Feedback has reported a user who discovered that we don't have labels for some of the controls in about:preferences#sync starting in Fx44, where it was redesigned.  I'm going to NI davidb to help us identify which controls have the missing labels.

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

Marco do you want this one?

Comment 2

[Marco Zehe (:MarcoZ)]

Yup, this window is a bit broken. ;)

1. The Unlink button has no label.
2. The Change Profile Picture button cannot be reached via the Tab key.
3. Same for the device name edit field. Make it read-only, but don't gray it out, please.
4. The groups don't have proper labels. There is a label that is the first child of each grouping, but it is not properly associated with the group itself, so it doesn't get picked up by the screen reader automatically.
5. No guarantee I didn't catch other items that cannot be reached via Tab.

Comment 3

[Leo Ufimtsev]

~University of Toronto, CSC302H1S-2016 assignment. Patch by Feb 26,2016~

I would like to work on this as my first FF bug fix, contribute a patch and have it merged into the main code base. (It's important for me to get it merged, my mark depends on it :-)).

I'm running on Linux Mint. I have checked out the gecko-dev source code, build FF 46 and am able to run it.

1) Would somebody advise me where to look for the source code that will generate the sync plane?
2) Is there some article on how to work with Accessibility in the gui framework that is used?
3) Would anyone recommend Linux screen readers with which I could test my patch?
4) Any other tips/recommendations? 

Thank you.

Comment 4

[Mark Hammond [:markh] [:mhammond]]

(In reply to Leo Ufimtsev from comment #3)
> 1) Would somebody advise me where to look for the source code that will
> generate the sync plane?

The sync pane's layout is at https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/sync.xul - the javascript for that is sync.js in the same directory.

> 2) Is there some article on how to work with Accessibility in the gui
> framework that is used?

Not that I'm aware of, but comment 2 should give you a good place to start.

> 3) Would anyone recommend Linux screen readers with which I could test my
> patch?

I don't know of any. IIUC, Windows has the good screenreader support - Marco might be able to tell you more.

I'd suggest implementing comment 2 and we can needinfo Marco once a reviewed patch is up.

Comment 5

[Leo Ufimtsev]

Attachment: 0001-Bug-1229206-Labels-for-screen-readers-are-missing-fr.patch

# Overview:
Patch to fix the mentioned issues.

I have tested manually with OSx's screen reader and did the unit tests with:
./mach mochitest browser/components/preferences/in-content/tests

Please kindly review, let me know if any changes are needed. 
If not, please merge the patch as I lack contributor rights.


# Details:
- I added a label attribute to to buttons that lacked it, as such screen reader now reads buttons in the sync preference plane correctly. This includes the buttons mentioned in initial comment and two other buttons that I found were lacking a label. See [0]
- I added <caption><label>text</label><caption> sections to groupboxes (where it made sense), so that it can be recognized by screen readers. See [0]
- I added -moz-user-focus CSS style to the profile picture image. As such, when in accessibility mode, the picture can be selected via tab key and opened. [0]


# Testing notes:
- (contrary to initial comment), I now build FF on OS X via [9]. (for quicker iterative development and OSX has nice build in screen reader) (as oppose to linux).

- I tested with OSX's build in "Voice over" feature. I.e:
-- I would turn on voice over and navigate via tab key.
-- The screen reader would navigate from one control to the other and read out label of controls.
-- The screen reader also successfully read out the profile image label.

- I tested the sync preference plane as following:
-- Initially, I was not signed in. I signed in via keyboard navigation and verified buttons were read out loud.
-- After signing in, I verified that I could navigate all options via tab key (and activation of controls can be achieved via Ctrl+Option+space in accessibility mode).

- Unit testing:
-- I ran the related unit tests:
-- ./mach mochitest browser/components/preferences/in-content/tests
-- all tests passed 


# References used during development:
[0] https://developer.mozilla.org/en/docs/XUL_accessibility_guidelines
    (main accessibility guide)
[1] https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Getting_started/XUL_user_interfaces
[2] https://developer.mozilla.org/en/docs/Browser_chrome_tests
[2b] https://developer.mozilla.org/en-US/docs/Mozilla/QA/Automated_testing
[2c] https://developer.mozilla.org/en-US/docs/Mochitest
[3] https://developer.mozilla.org/en-US/Learn/CSS/Using_CSS_in_a_web_page
[4] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/accesskey
[5] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Tutorial/Focus_and_Selection
[8] https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
[9] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Mac_OS_X_Prerequisites
[10] https://www.apple.com/voiceover/info/guide/

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8719349 [details] [diff] [review]
0001-Bug-1229206-Labels-for-screen-readers-are-missing-fr.patch

Thanks! Flagging for review so I don't forget it - it will probably take me a day or 2 to get to it.

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8719349 [details] [diff] [review]
0001-Bug-1229206-Labels-for-screen-readers-are-missing-fr.patch

Review of attachment 8719349 [details] [diff] [review]:
-----------------------------------------------------------------

Thans heaps for the contribution, and I'm sorry for the delay here - I'll look at followup patches ASAP.

As mentioned in the comments below, I think we need to change all the "click" handlers in sync.js for <button> elements to use "command" instead - but some elements will need to remain as "click" when "command" doesn't work - we should also identify them and we can open a new bug to deal with them later.

::: browser/components/preferences/in-content/sync.xul
@@ +139,4 @@
>      </groupbox>
>  
>      <groupbox class="syncGroupBox">
> +    <!-- Caption+label omitted as it may not make sense to add it here as there is only one control that can be altered-->

please remove this comment - it will not make sense in the future beyond the context of this bug.

@@ +190,4 @@
>    <vbox id="noFxaAccount">
>      <hbox>
>        <vbox id="fxaContentWrapper">
> +        <!-- this is seen before logging in, 'create account' and 'sign in' buttons etc.. -->

I think we might as well kill this comment too as (a) there are no equivalent comments for the other deck items and (b) it's actually in the wrong place - it applies to the "noFxaAccount" parent box.

@@ +194,2 @@
>          <groupbox id="noFxaGroup">
> +          <caption><label id="noFxaCaption">&signedOut.caption;</label></caption>

This changes the styling of the panel but IIUC doesn't change the accessibility. While this change may make sense, it doesn't as part of this bug.

@@ +202,4 @@
>                <vbox flex="1">
>                  <label id="signedOutAccountBoxTitle">&signedOut.accountBox.title;</label>
>                  <hbox class="fxaAccountBoxButtons" align="center">
> +                  <button label="&signedOut.accountBox.create;" id="noFxaSignUp">&signedOut.accountBox.create;</button>

it looks like these buttons should not have content - eg, this should just read:

<button label="&signedOut.accountBox.create;" id="noFxaSignUp"/>

but please keep the id as the first attribute. Same for all buttons here.

@@ +238,4 @@
>              <hbox id="fxaLoginVerified" class="fxaAccountBox">
>                <vbox>
>                  <image id="fxaProfileImage"
> +                    style="-moz-user-focus: normal;"

This should be added to 3 preferences.css files (one for windows, osx and linux) - see the "/* Sync Pane */" comment in those files. However, that does make it tabable, but still doesn't allow the image to be acted on via the keyboard (ie, it only has an onclick handler) - so I'm not sure this alone is enough to make this accessible. We probably also want |role="button"|, but that also doesn't help with the keyboard.

Many of the explicit buttons also have the same problem - sync.js adds "click" handlers for them, which mean they don't work for the keyboard. For the buttons we can make them use the "command" handler instead (which should also be done) but that doesn't work for the image.

I think we should move making this image accessible in another bug as it is likely to be rabbit-hole.

Comment 8

[Leo Ufimtsev]

Attachment: 0001-Bug-1229206-Labels-for-screen-readers-are-missing-fr.patch

Patch #2:

Changes:
- Profile Image:
  * Added onkeypress event handler that would be triggered when you press enter. 
    This is done in a similar style as already existing onClick handler. See [1]
  * I moved the CSS to the preference.css for OSX/win/Linux. I tested (and it works) on OSX, but should work on Win/Linux also. 
  * This is the only 'selectable' image that I could find in the preference plane, so I'm not really sure how they're 'suppose' to handle things.

- Added some command handlers for commands that were registered only for clicks. I tested those that I added.
  However, some click event handlers were registered for controls that I could not find or don't seem to exist (e.g remove device button). These may be left overs from previous versions because one no-longer 'removes' a device in the sync preferences but instead disconnects. Not quite sure what to do with those. 
  However, all actions (logging in, disconnecting/checking checkboxes" can be done with keyboard buttons space/enter.

- I removed redundant comments.
- Removed the irrelevant Caption.


Thoughts/Feedback?



[1]
* Enter only, space bar does not work:
  The following suggests to use "event.which" as character 32(space) is not recognized in Mozilla:
    http://stackoverflow.com/questions/14029708/space-bar-keycode32-not-working-on-mozilla-firefox
  but event.which is deprecated:
  https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/which
  Thus it looks like we have to stick with enter for now.
* handler coded into the XUL
  For some reason the following would not attach handlers to the code:
   setEventListener("fxaProfileImage", "click", function () {
      gSyncPane.openChangeProfileImage();
    });
    setEventListener("fxaProfileImage", "command", function () {
      gSyncPane.openChangeProfileImage();
    });
  As such, I followed the existing convention 'onclick' and added 'onkeypress' with a guard to only trigger on 'enter'.
    onclick="gSyncPane.openChangeProfileImage();" hidden="true"
  + onkeypress="return gSyncPane.openChangeProfileImageKeyHandler(event);"

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8721403 [details] [diff] [review]
0001-Bug-1229206-Labels-for-screen-readers-are-missing-fr.patch

Review of attachment 8721403 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking great, although I haven't tested it yet - I'll do that on Monday. Can you please upload a new patch with the suggested changes? It would also be good if you can try and find other examples of keyHandlers on images and see whether return or space (or both) is typically used - if you can't find anything I'll let you know on Monday. I expect this will be fine to merge early next week - thanks!

::: browser/components/preferences/in-content/sync.js
@@ +273,4 @@
>      });
>      setEventListener("verifiedManage", "click",
>        gSyncPane.manageFirefoxAccount);
> +    setEventListener("verifiedManage", "command", function () {

I'm fairly sure we can just replace the "click" with "command" - ie, we don't need both.

@@ +578,5 @@
>        });
>    },
>  
> +  openChangeProfileImageKeyHandler: function(event) {
> +    if (event.keyCode == 13) { //13 = enter.

I *think* we might want to handle "space" here - I'm not sure if we want both. It's a bit weird - buttons seem to activate on "space" but not "enter", while the <a> tags seem the opposite - I'll dig deeper on Monday.

::: browser/components/preferences/in-content/sync.xul
@@ +200,4 @@
>                <vbox flex="1">
>                  <label id="signedOutAccountBoxTitle">&signedOut.accountBox.title;</label>
>                  <hbox class="fxaAccountBoxButtons" align="center">
> +                  <button label="&signedOut.accountBox.create;" id="noFxaSignUp"></button>

Can you please swap the attributes so id= is first and label= second.

@@ +243,5 @@
>                <vbox flex="1">
>                  <label id="fxaEmailAddress1"/>
>                  <label id="fxaDisplayName" hidden="true"/>
>                  <hbox class="fxaAccountBoxButtons" align="center">
> +                  <button label="&disconnect.label;" id="fxaUnlinkButton"></button>

ditto here.

Comment 10

[Leo Ufimtsev]

Attachment: 0001-Bug-1229206-Labels-for-screen-readers-are-missing-fr.patch

Hello, 

Thank you for your feedback, very beneficial. 

Patch #3 attached. 

Changes:
- I removed 'click' handlers. I tested, clicking the buttons still works as well as pressing with keyboard. It does indeed seem that command handler handles clicking also. 
- I swapped ID and Label for relevant controls so that Id is first (for code style I guess?).
- I added a bit of code to handle spacebar also. Please see[1]
- Tested with OSX Acessibility / Voice over
- Ran mochitest for sync plane.

Please let me know if anything further needs fixing/improving :-).

Thank you.

[1] keyCode doesn't recognize Space (It shows up as '0').
To get around that I used charCode.
Now, charCode is deprecated but the suggested replacement char is not implemented ಠ_ಠ.  
Please see links below.
As such, I went along with charCode for now and left a comment for the future. I have a feeling it might take some years until char is fully functional, as it's been in the works since 2012. So perhaps we can get away with using charCode until then.

./side note:
I greped around for a bit, but haven't found another instance where an image is used as a control. (Unless you can find one?).
Also images don't seem to bend themselves to 'command' events, e.g adding the following to the image:
oncommand="alert('You pressed me!');”
does not have any effect on images as it does on buttons.

charcode not implemented: 
   https://bugzilla.mozilla.org/show_bug.cgi?id=680830
keypress events and their bugs: 
   https://developer.mozilla.org/en-US/docs/Web/Events/keypress

Comment 11

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8721687 [details] [diff] [review]
0001-Bug-1229206-Labels-for-screen-readers-are-missing-fr.patch

Review of attachment 8721687 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking great!

I think a good example of what we are trying to do for the image is at https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul#722 and https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#7034. In short, this mean adding |role="button"| to the element and have the keypress handler check for |event.charCode == KeyEvent.DOM_VK_SPACE || event.keyCode != KeyEvent.DOM_VK_RETURN| - which is exactly what you have except for the symbolic constants.

Could you please make those 2 changes (ie, role attribute and use of constants instead of literals) and I'm quite sure this will be good to go after I give it a quick test on Monday.

(and yeah, the id= being first is simply a style issue)
Thanks!

Comment 12

[Mark Hammond [:markh] [:mhammond]]

(In reply to Mark Hammond [:markh] from comment #11)
> the keypress handler check for |event.charCode == KeyEvent.DOM_VK_SPACE ||
> event.keyCode != KeyEvent.DOM_VK_RETURN| - which is exactly what you have
> except for the symbolic constants.

Oops - I (hopefully) obviously meant |event.charCode == KeyEvent.DOM_VK_SPACE || event.keyCode == KeyEvent.DOM_VK_RETURN|

Comment 13

[Leo Ufimtsev]

Attachment: 0001-Bug-1229206-Labels-for-screen-readers-are-missing-fr.patch

Hello Mark, 

Thank you for taking the time to find that example, interesting and neat bit of code there.

I used the concepts from the code you pointed out and merged the handlers into a single function. I adjusted the logic accordingly so that it only kicks in when (left mouse button|space|enter) are pressed. + added button role as attribute.

Tested as above via manual voice over + mochitests, works well. 

Let me know if I can improve the patch any further :-).

Thank you for your time and attention on this bug.

Comment 14

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8721785 [details] [diff] [review]
0001-Bug-1229206-Labels-for-screen-readers-are-missing-fr.patch

Review of attachment 8721785 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great, thanks! FTR I made a slight tweak to the indentation in openChangeProfileImage().

Comment 15

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8721785 [details] [diff] [review]
0001-Bug-1229206-Labels-for-screen-readers-are-missing-fr.patch

Actually, I spoke too soon :(

The <a> saying "Manage Account" doesn't work, because the "command" event doesn't work with those elements. I think you need to do roughly the same as you did for the image - ie, handle click and keypress commands. Note that you probably do *not* want to check for button==0 in that case - it currently works with any button.

Also, please change openChangeProfileImage to as shown below - it's functionally identical but the indentation of the last few lines has changed.

>  openChangeProfileImage: function(event) {
>    // Note: charCode is deprecated, but 'char' not yet implemented.
>    // Replace charCode with char when implemented, see Bug 680830
>    if ((event.type == "click" && event.button == 0) ||    // button 0 = 'main button', typically left click.
>        (event.type == "keypress" &&
>          (event.charCode == KeyEvent.DOM_VK_SPACE || event.keyCode == KeyEvent.DOM_VK_RETURN))) {
>      fxAccounts.promiseAccountsChangeProfileURI(this._getEntryPoint(), "avatar").then(url => {
>        this.openContentInBrowser(url, {
>          replaceQueryString: true
>        });
>      });
>    }
>  },

Comment 16

[Leo Ufimtsev]

Attachment: 0001-Bug-1229206-Labels-for-screen-readers-are-missing-fr.patch

Oh, my bad. Not sure how I managed to miss that one X-D 

I've patched things up:
- Button is now like other buttons in preference, i.e it's triggered solely by space. (Adding 'enter' trigger may make it inconsistent in respect with all the other buttons in preference.?). 
- Manage account link can be activated via space or enter
- Open profile picture can be activated via space or enter (as before). 
- fixed indentation (thank you for pointing that out).
- sprinkled positive energy onto the patch so as to make it bring goodness to the world.

@ ran through usual tests. Afaik now I can click/press my way all of the sync preferences.

Please let me know if I missed something or if there is a style issue somewhere else. I've gotten pretty quick at patching things.

Thank you for your continued time and effort on this patch.

Comment 17

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8722022 [details] [diff] [review]
0001-Bug-1229206-Labels-for-screen-readers-are-missing-fr.patch

Review of attachment 8722022 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome, thanks!

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/786bf7c76f8a

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/786bf7c76f8a

Comment 20

[Edwin Wong [:edwong]]

Nice patch Leo!

Comment 21

[Leo Ufimtsev]

Thank you. 

I appreciate all the feedback on how to improve the code, it was a valuable learning experience.