1086930 - Settings: Sync button should be a live region.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Calendar, defect)

Description

[Yura Zenevich [:yzen] (please use "needinfo")]

If the sync button is clicked, the graphic animates while the syncing is in process.

The screen reader needs to be aware of it happening via the live region. We need to update the aria-label for the time of syncing to ensure that the screen reader pick it up.

Comment 1

[jobara]

yura, maybe this should be an indeterminate progress.

Comment 2

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to jobara from comment #1)
> yura, maybe this should be an indeterminate progress.

I think you are right, you should give it a try, one thing is that you need to make sure that it is also a live region because it needs to be spoken to the user without being focused on the button.

Comment 3

[Yura Zenevich [:yzen] (please use "needinfo")]

Your bug might depend on bug 1087353 for localization.

Comment 4

[Yura Zenevich [:yzen] (please use "needinfo")]

Justin, FYI: http://yzen.github.io/firefoxos/2014/10/24/today-i-learned.html

Comment 5

[jobara]

Yura, I've been looking through the setup you recommended. I think I am connected although it seems that I have to install apps to debug them even though they are already installed. Not sure if I'm doing something wrong. Also, I can't find the sync button in the settings app. Where exactly is it, or is it not present in the simulation?

Comment 6

[Ross]

jobara, I think you may need to change a setting in your profile preferences to enable Certified App Debugging.  With B2G closed, browse to your profile directory (typically found within your local Gaia Source directory, if using the "make" command to generate your profile) and open up "prefs.js".  In "prefs.js", add the following line...

user_pref("devtools.debugger.forbid-certified-apps", false);

That should allow you to see Certified Apps from the drop-down list in WebIDE.  More info here: https://developer.mozilla.org/en-US/docs/Tools/WebIDE#Unrestricted_app_debugging_(including_certified_apps.2C_main_process.2C_etc.)

To answer your inquiry about the "sync" button, it is located in the "Calendar" app but only after you've successfully added another web-based calendar account for syncing.  I successfully used a test Yahoo account.

Hope that helps!

Comment 7

[jobara]

(In reply to Ross from comment #6)
> jobara, I think you may need to change a setting in your profile preferences
> to enable Certified App Debugging.  With B2G closed, browse to your profile
> directory (typically found within your local Gaia Source directory, if using
> the "make" command to generate your profile) and open up "prefs.js".  In
> "prefs.js", add the following line...
> 
> user_pref("devtools.debugger.forbid-certified-apps", false);
> 
> That should allow you to see Certified Apps from the drop-down list in
> WebIDE.  More info here:
> https://developer.mozilla.org/en-US/docs/Tools/
> WebIDE#Unrestricted_app_debugging_(including_certified_apps.2C_main_process.
> 2C_etc.)
> 
> To answer your inquiry about the "sync" button, it is located in the
> "Calendar" app but only after you've successfully added another web-based
> calendar account for syncing.  I successfully used a test Yahoo account.
> 
> Hope that helps!

Thanks Ross that seems to have helped.

Comment 8

[jobara]

(In reply to Yura Zenevich [:yzen] from comment #2)
> (In reply to jobara from comment #1)
> > yura, maybe this should be an indeterminate progress.
> 
> I think you are right, you should give it a try, one thing is that you need
> to make sure that it is also a live region because it needs to be spoken to
> the user without being focused on the button.

Yura, is this because the button and not the progress animation gets focus?

Also, do you have thoughts on what the progress should be. This appears to be an indeterminate progressbar. According to the spec, for indeterminate progressbars the aria-valuenow should not be set. We could set the aria-valuetext instead, to something like "complete" when it's not syncing, and "in progress" when it is.

http://www.w3.org/TR/wai-aria/roles#progressbar 
http://oaa-accessibility.org/examplep/progressbar1/

Comment 9

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to jobara from comment #8)
> (In reply to Yura Zenevich [:yzen] from comment #2)
> > (In reply to jobara from comment #1)
> > > yura, maybe this should be an indeterminate progress.
> > 
> > I think you are right, you should give it a try, one thing is that you need
> > to make sure that it is also a live region because it needs to be spoken to
> > the user without being focused on the button.
> 
> Yura, is this because the button and not the progress animation gets focus?
> 
> Also, do you have thoughts on what the progress should be. This appears to
> be an indeterminate progressbar. According to the spec, for indeterminate
> progressbars the aria-valuenow should not be set. We could set the
> aria-valuetext instead, to something like "complete" when it's not syncing,
> and "in progress" when it is.
> 
> http://www.w3.org/TR/wai-aria/roles#progressbar 
> http://oaa-accessibility.org/examplep/progressbar1/

Yeah, I agree, I think it should be valuetext indeed. I think that when synced it should just go back to being a button label though, but feel free to see what works best for the user.

Comment 10

[jobara]

Yura, I hadn't thought about this switching between a button and progressbar. I've made a first pass using just the indeterminate progressbar without aria-valuetext. Could you please let me what you think of the AT experience.

https://github.com/jobara/gaia/tree/1086930

Comment 11

[jobara]

Attachment: Added a status area inside of the sync button this is only added to the a11y tree during syncing. It will specify that syncing has started and completed.

Yura, I wasn't able to get the calendar settings tests to run. I kept getting an error. This is both with master and my branch. Please let me know if you are able to get the tests running and I'll look into more. I was unable to write tests because of this.

Comment 12

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to jobara from comment #11)
> Yura, I wasn't able to get the calendar settings tests to run. I kept
> getting an error. This is both with master and my branch. Please let me know
> if you are able to get the tests running and I'll look into more. I was
> unable to write tests because of this.

I tested with latest master and all Calendar tests seem to pass. Try removing all of profile* and build_stage directories and re-running:
bin/gaia-test /full/path/to/gaia /full/path/to/firefox/nightly

Comment 13

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8527346 [details] [review]
Added a status area inside of the sync button this is only added to the a11y tree during syncing. It will specify that syncing has started and completed.

Left some comments in PR. thanks!

Comment 14

[jobara]

(In reply to Yura Zenevich [:yzen] from comment #13)
> Comment on attachment 8527346 [details] [review]
> Added a status area inside of the sync button this is only added to the a11y
> tree during syncing. It will specify that syncing has started and completed.
> 
> Left some comments in PR. thanks!

I made the changes you recommended in the PR. I'm still having issues running the tests, even after removing the profile* and build_stage directories.

Comment 15

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8527346 [details] [review]
Added a status area inside of the sync button this is only added to the a11y tree during syncing. It will specify that syncing has started and completed.

Sorry, I should've mentioned that there are 2 flags like that and I meant the one for the attachment :). You can then also select me as Requestee there

Comment 16

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to jobara from comment #14)
> (In reply to Yura Zenevich [:yzen] from comment #13)
> > Comment on attachment 8527346 [details] [review]
> > Added a status area inside of the sync button this is only added to the a11y
> > tree during syncing. It will specify that syncing has started and completed.
> > 
> > Left some comments in PR. thanks!
> 
> I made the changes you recommended in the PR. I'm still having issues
> running the tests, even after removing the profile* and build_stage
> directories.

So I tested it and it looks like the complete is not read out. What is happening is that the status is being hidden before the data-l10n-id is updated because the listener that you add fires after the one added via PendingManager. Also I think 'pending-operation' class is too broad and is used in other places, which means that the label will incorrectly get updated on events other than sync. 

So, the way forward, I think, is to add/remove our own CSS class within your listeners and handle visibility based on that. That will ensure the uniqueness of the styling (that it is only present when syncing actually happens) as well as it will give you control over the order of label change and visibility change.

Comment 17

[jobara]

(In reply to Yura Zenevich [:yzen] from comment #16)
> (In reply to jobara from comment #14)
> > (In reply to Yura Zenevich [:yzen] from comment #13)
> > > Comment on attachment 8527346 [details] [review]
> > > Added a status area inside of the sync button this is only added to the a11y
> > > tree during syncing. It will specify that syncing has started and completed.
> > > 
> > > Left some comments in PR. thanks!
> > 
> > I made the changes you recommended in the PR. I'm still having issues
> > running the tests, even after removing the profile* and build_stage
> > directories.
> 
> So I tested it and it looks like the complete is not read out. What is
> happening is that the status is being hidden before the data-l10n-id is
> updated because the listener that you add fires after the one added via
> PendingManager. Also I think 'pending-operation' class is too broad and is
> used in other places, which means that the label will incorrectly get
> updated on events other than sync. 
> 
> So, the way forward, I think, is to add/remove our own CSS class within your
> listeners and handle visibility based on that. That will ensure the
> uniqueness of the styling (that it is only present when syncing actually
> happens) as well as it will give you control over the order of label change
> and visibility change.

Made those changes, please let me know if that works better.

Comment 18

[jobara]

(In reply to jobara from comment #17)
> (In reply to Yura Zenevich [:yzen] from comment #16)
> > (In reply to jobara from comment #14)
> > > (In reply to Yura Zenevich [:yzen] from comment #13)
> > > > Comment on attachment 8527346 [details] [review]
> > > > Added a status area inside of the sync button this is only added to the a11y
> > > > tree during syncing. It will specify that syncing has started and completed.
> > > > 
> > > > Left some comments in PR. thanks!
> > > 
> > > I made the changes you recommended in the PR. I'm still having issues
> > > running the tests, even after removing the profile* and build_stage
> > > directories.
> > 
> > So I tested it and it looks like the complete is not read out. What is
> > happening is that the status is being hidden before the data-l10n-id is
> > updated because the listener that you add fires after the one added via
> > PendingManager. Also I think 'pending-operation' class is too broad and is
> > used in other places, which means that the label will incorrectly get
> > updated on events other than sync. 
> > 
> > So, the way forward, I think, is to add/remove our own CSS class within your
> > listeners and handle visibility based on that. That will ensure the
> > uniqueness of the styling (that it is only present when syncing actually
> > happens) as well as it will give you control over the order of label change
> > and visibility change.
> 
> Made those changes, please let me know if that works better.

I've made changes based on your PR comments. I'm still having trouble running the tests. I seem to be able to run all of the tests except for the ones for the calendar's view settings. It just comes up as 0. This is for both master and my branch. I even tried removing all of the files and directories that aren't versioned and re-ran make. Any thoughts?

Comment 19

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to jobara from comment #18)
> (In reply to jobara from comment #17)
> > (In reply to Yura Zenevich [:yzen] from comment #16)
> > > (In reply to jobara from comment #14)
> > > > (In reply to Yura Zenevich [:yzen] from comment #13)
> > > > > Comment on attachment 8527346 [details] [review]
> > > > > Added a status area inside of the sync button this is only added to the a11y
> > > > > tree during syncing. It will specify that syncing has started and completed.
> > > > > 
> > > > > Left some comments in PR. thanks!
> > > > 
> > > > I made the changes you recommended in the PR. I'm still having issues
> > > > running the tests, even after removing the profile* and build_stage
> > > > directories.
> > > 
> > > So I tested it and it looks like the complete is not read out. What is
> > > happening is that the status is being hidden before the data-l10n-id is
> > > updated because the listener that you add fires after the one added via
> > > PendingManager. Also I think 'pending-operation' class is too broad and is
> > > used in other places, which means that the label will incorrectly get
> > > updated on events other than sync. 
> > > 
> > > So, the way forward, I think, is to add/remove our own CSS class within your
> > > listeners and handle visibility based on that. That will ensure the
> > > uniqueness of the styling (that it is only present when syncing actually
> > > happens) as well as it will give you control over the order of label change
> > > and visibility change.
> > 
> > Made those changes, please let me know if that works better.
> 
> I've made changes based on your PR comments. I'm still having trouble
> running the tests. I seem to be able to run all of the tests except for the
> ones for the calendar's view settings. It just comes up as 0. This is for
> both master and my branch. I even tried removing all of the files and
> directories that aren't versioned and re-ran make. Any thoughts?

Oh yeah I think several of those test suites are disabled(using return; in the js file before all test cases are defined). Threw me off too.

Comment 20

[Yura Zenevich [:yzen] (please use "needinfo")]

this: https://github.com/jobara/gaia/blob/1086930/apps/calendar/test/unit/views/settings_test.js#L15

Comment 21

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8527346 [details] [review]
Added a status area inside of the sync button this is only added to the a11y tree during syncing. It will specify that syncing has started and completed.

I left some comments in Github. This is pretty much in a great state but I guess we ll be better of switching to progress since we will add better support for it (bug 1105770). Also you should still be able to run your tests locally (to make sure they pass) if you remove that return; and comment out the possible broken ones.

Comment 22

[jobara]

I made the changes you suggested and pushed up to the PR. Could you please see if it is inline with what you were thinking.

(In reply to Yura Zenevich [:yzen] from comment #21)
> Comment on attachment 8527346 [details] [review]
> Added a status area inside of the sync button this is only added to the a11y
> tree during syncing. It will specify that syncing has started and completed.
> 
> I left some comments in Github. This is pretty much in a great state but I
> guess we ll be better of switching to progress since we will add better
> support for it (bug 1105770). Also you should still be able to run your
> tests locally (to make sure they pass) if you remove that return; and
> comment out the possible broken ones.

Comment 23

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8527346 [details] [review]
Added a status area inside of the sync button this is only added to the a11y tree during syncing. It will specify that syncing has started and completed.

Looks good from the a11y point of view. Once the remaining comments are addressed, please ask Miller Medeiros [:millermedeiros] for the review.

Thanks!

Comment 24

[jobara]

yzen I've updated the pull request with the changes you recommended. As you know the tests are a bit strange as they are intentionally skipped. I've made sure that my tests will run when the tests are permitted to run and the other tests are commented out.

Comment 25

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8527346 [details] [review]
Added a status area inside of the sync button this is only added to the a11y tree during syncing. It will specify that syncing has started and completed.

Looks good to me with last comments addressed. Thanks!

Comment 26

[jobara]

(In reply to Yura Zenevich [:yzen] from comment #25)
> Comment on attachment 8527346 [details] [review]
> Added a status area inside of the sync button this is only added to the a11y
> tree during syncing. It will specify that syncing has started and completed.
> 
> Looks good to me with last comments addressed. Thanks!

Made those changes to the tests you recommended.

Comment 27

[Miller Medeiros [:millermedeiros]]

Comment on attachment 8527346 [details] [review]
Added a status area inside of the sync button this is only added to the a11y tree during syncing. It will specify that syncing has started and completed.

good work!

Comment 28

[Yura Zenevich [:yzen] (please use "needinfo")]

https://github.com/mozilla-b2g/gaia/commit/a5c5ac093814a80b0627514c3bd5f9e96c096a4b

Comment 29

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8527346 [details] [review]
Added a status area inside of the sync button this is only added to the a11y tree during syncing. It will specify that syncing has started and completed.

[Approval Request Comment] Will improve screen reader usability when syncing calendars. Right now screen reader user has no feedback if they press on Sync button. However sighted user can see a spinning progress in place of the button icon. This changes adds a progress element (that screen reader can see) when syncing is happening.
[Bug caused by] (feature/regressing bug #): improvement
[User impact] if declined: screen reader users will not have any feedback when syncing calendars
[Testing completed]: added unit tests
[Risk to taking this patch] (and alternatives if risky): fairly small, the change adds and removes a screen reader only visible markup to the button.
[String changes made]: 2 strings added: https://github.com/mozilla-b2g/gaia/pull/26386/files#diff-2

Comment 30

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/3b1fdfc82bdb8c36296b76ae94cd37297692316b