Dialogs are not exclusively visible to screen reader

RESOLVED FIXED in 2.2 S6 (20feb)

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: eeejay, Assigned: eeejay)

Tracking

({access})

unspecified
2.2 S6 (20feb)
All
Gonk (Firefox OS)
access
Dependency tree / graph

Firefox Tracking Flags

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

Details

(Whiteboard: [b2ga11y p=1])

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
When a dialog is showing, the content underneath is still navigable by the screen reader. Some example dialogs:
- Delete photo
- Photo info
- Loading from sd card overlay
(Assignee)

Comment 1

4 years ago
Created attachment 8556791 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27800

I chose this approach because (at least in the gallery view) one of the dialogs is translucent. Otherwise, doing this in CSS would be preferable (and potentially cleaner).
Assignee: nobody → eitan
Attachment #8556791 - Flags: review?(pdahiya)
(In reply to Eitan Isaacson [:eeejay] from comment #1)
> Created attachment 8556791 [details] [review]
> Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27800
> 
> I chose this approach because (at least in the gallery view) one of the
> dialogs is translucent.Otherwise, doing this in CSS would be preferable
> (and potentially cleaner).

Can you pl. update which dialog is the translucent one, I looked at the flows and failed to figure it. 

It's r- because the patch needs to handle deleteSingleItem flow in frames.js
https://github.com/mozilla-b2g/gaia/blob/master/apps/gallery/js/frames.js#L128
Attachment #8556791 - Flags: review?(pdahiya) → review-
(Assignee)

Comment 3

4 years ago
(In reply to Punam Dahiya from comment #2)
> (In reply to Eitan Isaacson [:eeejay] from comment #1)
> > Created attachment 8556791 [details] [review]
> > Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27800
> > 
> > I chose this approach because (at least in the gallery view) one of the
> > dialogs is translucent.Otherwise, doing this in CSS would be preferable
> > (and potentially cleaner).
> 
> Can you pl. update which dialog is the translucent one, I looked at the
> flows and failed to figure it. 

https://github.com/mozilla-b2g/gaia/blob/master/apps/gallery/style/gallery_tablet.css#L342

> 
> It's r- because the patch needs to handle deleteSingleItem flow in frames.js
> https://github.com/mozilla-b2g/gaia/blob/master/apps/gallery/js/frames.
> js#L128

Oops! fixing that..
(Assignee)

Comment 4

4 years ago
Comment on attachment 8556791 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27800

Fixed patch to also hide fullscreen frame on singe item delete dialog.
Attachment #8556791 - Flags: review- → review?(pdahiya)
I'd prefer a CSS fix to a JS fix here. Tablet is not really supported anymore and the code is out of date. Let's just make the dialog opaque if that is all that is preventing a cleaner CSS-only fix to this bug.
Flags: needinfo?(eitan)
Comment on attachment 8556791 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27800

Hi Eitan
Cancelling the review in favor of CSS fix for this bug. As confirmed by David, we can update tablet css for an opaque dialog. Thanks
Attachment #8556791 - Flags: review?(pdahiya)
(Assignee)

Comment 7

4 years ago
(In reply to David Flanagan [:djf] from comment #5)
> I'd prefer a CSS fix to a JS fix here. Tablet is not really supported
> anymore and the code is out of date. Let's just make the dialog opaque if
> that is all that is preventing a cleaner CSS-only fix to this bug.

Sounds good. This will require putting the dialogs above the main content in markup order, so we could use +/~ selectors.
Flags: needinfo?(eitan)
(Assignee)

Comment 8

4 years ago
(In reply to Eitan Isaacson [:eeejay] from comment #7)
> (In reply to David Flanagan [:djf] from comment #5)
> > I'd prefer a CSS fix to a JS fix here. Tablet is not really supported
> > anymore and the code is out of date. Let's just make the dialog opaque if
> > that is all that is preventing a cleaner CSS-only fix to this bug.
> 
> Sounds good. This will require putting the dialogs above the main content in
> markup order, so we could use +/~ selectors.

Or alternatively, having a class like 'dialog-open' in a parent container.
(Assignee)

Comment 9

4 years ago
Comment on attachment 8556791 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27800

Added a body class 'showing-dialog' and have it toggled when a dialog is displayed. The main sections are styled with visibility: hidden to avoid a reflow.

This looks lower risk than messing more with markup order.
Attachment #8556791 - Flags: feedback?(dflanagan)
Comment on attachment 8556791 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27800

This looks good to me. As usual, my main comments are requests for more comments in the code.

I don't actually understand why this is necessary. Do all apps have to hide their content when they display a dialog, or is there something weird about Gallery that makes this necessary? 

Also, did you mean to include the python test changes in this PR, or are they there by mistake?
Attachment #8556791 - Flags: feedback?(dflanagan) → feedback+
(Assignee)

Comment 11

4 years ago
(In reply to David Flanagan [:djf] from comment #10)
> Comment on attachment 8556791 [details] [review]
> Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27800
> 
> This looks good to me. As usual, my main comments are requests for more
> comments in the code.
> 

Added comments in code, thanks!

> I don't actually understand why this is necessary. Do all apps have to hide
> their content when they display a dialog, or is there something weird about
> Gallery that makes this necessary? 
> 

Copying my response from gh:
The screen reader does not distinguished between obscured and un-obscured content. First, because it is really hard to do, you need to take into account the relative opacity of each obscuring layout frame, and false positives would be very bad. Also, historically developers would hide text intended for screen readers in different fashions, like putting it in a negative offset, using font-size:0 or obscuring it. The issue of modal dialogs is not unique to this module, and is the most painful part of making gaia accessible. We will need to do this until the <dialog> element is implemented in gecko.

> Also, did you mean to include the python test changes in this PR, or are
> they there by mistake?

Yes, the tests break without those changes.
(Assignee)

Updated

4 years ago
Attachment #8556791 - Flags: review?(pdahiya)
Comment on attachment 8556791 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27800

Thanks Eitan, it's r+ , with gaia-try green it should be good to land.
Attachment #8556791 - Flags: review?(pdahiya) → review+
(Assignee)

Comment 13

4 years ago
https://github.com/mozilla-b2g/gaia/commit/68c1588bf5a90b0fff8c9667ef2c8c68ab59569a
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 14

4 years ago
Comment on attachment 8556791 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27800

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined: Screen reader users will be able to see passed the dialogs into the hidden main content.
[Testing completed]: Python UI tests have been altered to test for this as well.
[Risk to taking this patch] (and alternatives if risky): Little.
[String changes made]: No.
Attachment #8556791 - Flags: approval-gaia-v2.2?
Attachment #8556791 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
v2.2: https://github.com/mozilla-b2g/gaia/commit/68e0d84c38e2dfdd90119e6d619b54da07a6c97f
status-b2g-v2.2: --- → fixed
status-b2g-master: --- → fixed
Target Milestone: --- → 2.2 S6 (20feb)
You need to log in before you can comment on or make changes to this bug.