Closed Bug 1068880 Opened 5 years ago Closed 5 years ago

[Camera] Improve accessibility of the drop down menu.

Categories

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

All
Gonk (Firefox OS)
defect
Not set

Tracking

(b2g-v2.2 fixed, b2g-master fixed)

RESOLVED FIXED
2.2 S7 (6mar)
Tracking Status
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: eeejay, Assigned: yzen)

References

Details

(Keywords: access, late-l10n, Whiteboard: [b2ga11y p=1])

Attachments

(2 files)

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.
Summary: Drop down menu should have exclusive screen reader visibility → [Camera] Drop down menu should have exclusive screen reader visibility
Assignee: nobody → yzenevich
Status: NEW → ASSIGNED
Summary: [Camera] Drop down menu should have exclusive screen reader visibility → [Camera] Improve accessibility of the drop down menu.
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.
Attachment #8572789 - Flags: review?(wilsonpage)
Blocks: 1068883
Blocks: 1068884
Comment on attachment 8572789 [details] [review]
[gaia] yzen:bug-1068880 > mozilla-b2g:master

Since Wilson is on PTO, asking Justin for a review.
Attachment #8572789 - Flags: review?(wilsonpage) → review?(jdarcangelo)
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!
Attachment #8572789 - Flags: review?(jdarcangelo) → review+
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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
Attachment #8572789 - Flags: approval-gaia-v2.2?
Attachment #8572789 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
Flags: needinfo?(splewako)
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?
Flags: needinfo?(splewako)
These are accessibility labels, they are used by screenreader to describe the current status of a setting.
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.
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?
s/I'm still don't know what will be replace/I still don't know what will replace/
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.
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.
(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.
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
Flags: needinfo?(splewako)
(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.
Flags: needinfo?(splewako)
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.
Attachment #8578794 - Flags: review?(jdarcangelo)
Attachment #8578794 - Flags: feedback?(splewako)
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.
Attachment #8578794 - Flags: feedback?(splewako) → feedback?(francesco.lodolo)
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.
Attachment #8578794 - Flags: feedback?(francesco.lodolo) → feedback+
(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.
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!
Flags: needinfo?(jdarcangelo)
Comment on attachment 8578794 [details] [review]
[gaia] yzen:bug-1068880 > mozilla-b2g:master

LGTM. Sorry for the delay!
Flags: needinfo?(jdarcangelo)
Attachment #8578794 - Flags: review?(jdarcangelo) → review+
https://github.com/mozilla-b2g/gaia/commit/07961e074e4151c02bfbaa4c873ad28c036048bf
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.