Eitan Isaacson [:eeejay] Reporter
	Description • 10 years ago

While the drop down is active, I could still navigate to the flash button and switch camera button. The bottom controls are dimmed, but they are navigable as well.

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 1 • 9 years ago

Things that needs to be fixed:
* Drop down menu should have exclusive screen reader visibility
* Menu items must be labelled for screen reader
* All items should be actionable.

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 3 • 9 years ago

Comment on attachment 8572789 [details] [review]
[gaia] yzen:bug-1068880 > mozilla-b2g:master

Since Wilson is on PTO, asking Justin for a review.

	Justin D'Arcangelo [:justindarc]
	Comment 4 • 9 years ago

Comment on attachment 8572789 [details] [review]
[gaia] yzen:bug-1068880 > mozilla-b2g:master

One minor comment about `will-change`. Other than that, looks good. Thanks!

	GitHub Autolander Bot
	Comment 5 • 9 years ago

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/d08bb1fcf1373657a79269d34bb6d61ad1cd6c45

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 6 • 9 years ago

Comment on attachment 8572789 [details] [review]
[gaia] yzen:bug-1068880 > mozilla-b2g:master

Approval Request Comment] This PR makes camera app menus accessible.
[Bug caused by] (feature/regressing bug #): improvements not a bug
[User impact] if declined: if declined the menus will be inaccessible to the screen reader users.
[Testing completed]: unit tests + on device
[Risk to taking this patch] (and alternatives if risky): fairly low, mostly changes to accessibility related DOM
[String changes made]: https://github.com/mozilla-b2g/gaia/pull/28640/files#diff-e4bc27afcad6f10d1abaf8eced7aa340

	Ryan VanderMeulen [:RyanVM]
	Comment 7 • 9 years ago

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

	Stefan Plewako [:stef]
	Comment 8 • 9 years ago

https://hg.mozilla.org/integration/gaia-central/diff/2580234337d7/apps/camera/locales/camera.en-US.properties
> +setting-option.ariaLabel = {{value}}
> +setting-option-hdr.ariaLabel = HDR {{value}}
> +setting-option-self-timer.ariaLabel = Self-Timer {{value}}
> +setting-option-grid.ariaLabel = Grid Lines {{value}}
> +setting-option-scene-mode.ariaLabel = Scene Mode {{value}}
> +setting-option-camera-resolution.ariaLabel = Camera Resolution {{value}}
> +setting-option-video-resolution.ariaLabel = Video Resolution {{value}}

Are these strings for setting label or for actionable widget? What are values?

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 9 • 9 years ago

(In reply to Stefan Plewako [:stef] from comment #8)
> https://hg.mozilla.org/integration/gaia-central/diff/2580234337d7/apps/
> camera/locales/camera.en-US.properties
> > +setting-option.ariaLabel = {{value}}
> > +setting-option-hdr.ariaLabel = HDR {{value}}
> > +setting-option-self-timer.ariaLabel = Self-Timer {{value}}
> > +setting-option-grid.ariaLabel = Grid Lines {{value}}
> > +setting-option-scene-mode.ariaLabel = Scene Mode {{value}}
> > +setting-option-camera-resolution.ariaLabel = Camera Resolution {{value}}
> > +setting-option-video-resolution.ariaLabel = Video Resolution {{value}}
> 
> Are these strings for setting label or for actionable widget? What are
> values?

Hi Stefan, this is for settings labels inside the Camera app's dropdown menus, hope that helps.

	Stefan Plewako [:stef]
	Comment 10 • 9 years ago

Not much. :(

(In reply to Yura Zenevich [:yzen] from comment #9)
> Hi Stefan, this is for settings labels inside the Camera app's dropdown

setting-option-* strings only provide informations about current status and changing that status requires other action?

	Francesco Lodolo [:flod]
	Comment 11 • 9 years ago

These are accessibility labels, they are used by screenreader to describe the current status of a setting.

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 12 • 9 years ago

Essentially it mirrors exactly the textual content of the option, but we can't just use that because there's no way to hide content set in CSS. If this description suffices, it'd be nice if we resolve this bug.

	Stefan Plewako [:stef]
	Comment 13 • 9 years ago

I'm still don't know what will be replace {{value}}s - is it localizable, are hdr-* self-timer-* grid-* reused (forming things like "HDR (HDR deactivated)")? What with setting-option-scene-mode.ariaLabel, setting-option-camera-resolution.ariaLabel, setting-option-video-resolution.ariaLabel?

	Stefan Plewako [:stef]
	Comment 14 • 9 years ago

s/I'm still don't know what will be replace/I still don't know what will replace/

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 15 • 9 years ago

So the value is an already localized value for the setting. In cases you mentioned they would be:

setting-option-hdr: HDR Off, HRD On  , the deactivated part only used for status messages afaik
setting-option-self-timer: Self-Timer Off, Self-Timer 2 seconds, Self-Timer 5 seconds, Self-Timer 10 seconds
setting-option-grid: Grid Lines On, Grid Lines Off

The remaining 3: setting-option-scene-mode, setting-option-camera-resolution, setting-option-video-resolution are not currently enabled in settings (and not used), but I wanted to make sure they are also included when they become available.

	Stefan Plewako [:stef]
	Comment 16 • 9 years ago

Ah… and I forgot, what the hell setting-option.ariaLabel is for?

(In reply to Yura Zenevich [:yzen] from comment #15)
> So the value is an already localized value for the setting.

So reused - this should be workaroundable, at least in Polish, don't know about other locales.

> setting-option-hdr: HDR Off, HRD On  , the deactivated part only used for
> status messages afaik

Just to be sure, hdr-deactivated will not be used?

> The remaining 3: setting-option-scene-mode,
> setting-option-camera-resolution, setting-option-video-resolution are not
> currently enabled in settings (and not used), but I wanted to make sure they
> are also included when they become available.

Prelanding strings is bad idea.


Good comment in camera.en-US.properties about values and removal of unused strings is what I'm missing here.
While testing I also noticed incorrect strings reuse in status messages (ie when I activate flash with hdr on) but that seems to be different story/bug.

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 17 • 9 years ago

(In reply to Stefan Plewako [:stef] from comment #16)
> Ah… and I forgot, what the hell setting-option.ariaLabel is for?
> 
> (In reply to Yura Zenevich [:yzen] from comment #15)
> > So the value is an already localized value for the setting.
> 
> So reused - this should be workaroundable, at least in Polish, don't know
> about other locales.
> 
> > setting-option-hdr: HDR Off, HRD On  , the deactivated part only used for
> > status messages afaik
> 
> Just to be sure, hdr-deactivated will not be used?

Yes, correct.

> 
> > The remaining 3: setting-option-scene-mode,
> > setting-option-camera-resolution, setting-option-video-resolution are not
> > currently enabled in settings (and not used), but I wanted to make sure they
> > are also included when they become available.
> 
> Prelanding strings is bad idea.
> 
> 
> Good comment in camera.en-US.properties about values and removal of unused
> strings is what I'm missing here.
> While testing I also noticed incorrect strings reuse in status messages (ie
> when I activate flash with hdr on) but that seems to be different story/bug.

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 18 • 9 years ago

setting-option.ariaLabel is for an option that does not have a proceeding label, {{value}} would be already localized there.

I would like to close this in case there's a follow up bug we can discuss it there (I believe l10n.get use should be addressed for the entire app and it probably belongs in a different bug). Thanks

	Stefan Plewako [:stef]
	Comment 19 • 9 years ago

(In reply to Yura Zenevich [:yzen] from comment #18)
> setting-option.ariaLabel is for an option that does not have a proceeding
> label, {{value}} would be already localized there.

What option? Localized to what?

> I would like to close this in case there's a follow up bug we can discuss it

I don't see a point in pushing discussion to another obscure bug about how the core topic of this one should be implemented, ymmv.

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 21 • 9 years ago

Comment on attachment 8578794 [details] [review]
[gaia] yzen:bug-1068880 > mozilla-b2g:master

Follow up, that removes pre-landed strings, as Stefan suggested, and adds l10n comments.

	Stefan Plewako [:stef]
	Comment 22 • 9 years ago

Comment on attachment 8578794 [details] [review]
[gaia] yzen:bug-1068880 > mozilla-b2g:master

Much better, thanks!
Redirecting feedback request to Flod as probably he should have the last word.

	Francesco Lodolo [:flod]
	Comment 23 • 9 years ago

Comment on attachment 8578794 [details] [review]
[gaia] yzen:bug-1068880 > mozilla-b2g:master

LGTM, I'd rephrase the comment a bit just to clarify that the value will be localized.

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 24 • 9 years ago

(In reply to Francesco Lodolo [:flod] from comment #23)
> Comment on attachment 8578794 [details] [review]
> [gaia] yzen:bug-1068880 > mozilla-b2g:master
> 
> LGTM, I'd rephrase the comment a bit just to clarify that the value will be
> localized.

Thanks, I took care of it.

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 25 • 9 years ago

Hi Justin, I was wondering if you would have a moment to take a look at this one. I was hoping to land this before the March 19th cutoff? Thanks!

	Justin D'Arcangelo [:justindarc]
	Comment 26 • 9 years ago

Comment on attachment 8578794 [details] [review]
[gaia] yzen:bug-1068880 > mozilla-b2g:master

LGTM. Sorry for the delay!

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 27 • 9 years ago

https://github.com/mozilla-b2g/gaia/commit/07961e074e4151c02bfbaa4c873ad28c036048bf