1068987 - Dialogs are not exclusively visible to screen reader

Status: RESOLVED FIXED

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

Description

[Eitan Isaacson [:eeejay]]

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

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: 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).

Comment 2

[Punam Dahiya [: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

Comment 3

[Eitan Isaacson [:eeejay]]

(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..

Comment 4

[Eitan Isaacson [:eeejay]]

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.

Comment 5

[David Flanagan [:djf]]

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.

Comment 6

[Punam Dahiya [:pdahiya]]

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

Comment 7

[Eitan Isaacson [:eeejay]]

(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.

Comment 8

[Eitan Isaacson [:eeejay]]

(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.

Comment 9

[Eitan Isaacson [:eeejay]]

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.

Comment 10

[David Flanagan [:djf]]

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?

Comment 11

[Eitan Isaacson [:eeejay]]

(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.

Comment 12

[Punam Dahiya [: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.

Comment 13

[Eitan Isaacson [:eeejay]]

https://github.com/mozilla-b2g/gaia/commit/68c1588bf5a90b0fff8c9667ef2c8c68ab59569a

Comment 14

[Eitan Isaacson [:eeejay]]

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.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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