Closed Bug 995035 Opened 6 years ago Closed 6 years ago

Invisible UI elements are visible to screen reader

Categories

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

All
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:2.1+, b2g-v2.1 fixed, b2g-v2.2 fixed)

RESOLVED FIXED
2.1 S4 (12sep)
blocking-b2g 2.1+
Tracking Status
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: eeejay, Assigned: eeejay)

References

Details

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

Attachments

(2 files)

When swiping through the interface with the screen reader, I am able to delete, deselect, select all buttons that are invisible at first. They need to be properly hidden.
Whiteboard: [b2ga11y 2.0]
Blocks: 995494
Whiteboard: [b2ga11y 2.0] → [b2ga11y p=2]
will be a lot easier once bug 881469 lands.
Depends on: 881469
Comment on attachment 8436002 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/20160

Adding Arnau for the BB change.
Attachment #8436002 - Flags: review?(arnau)
Comment on attachment 8436002 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/20160

Sorry Eitan, this doesn't work for the large panels :(

I guess we'll need to use aria-hidden here...
Attachment #8436002 - Flags: review?(felash) → review-
(In reply to Julien Wajsberg [:julienw] from comment #4)
> Comment on attachment 8436002 [details] [review]
> Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/20160
> 
> Sorry Eitan, this doesn't work for the large panels :(
> 
> I guess we'll need to use aria-hidden here...

How doesn't it work? Does it show/hide elements when it shouldn't? Performance?
(In reply to Eitan Isaacson [:eeejay] from comment #5)
> (In reply to Julien Wajsberg [:julienw] from comment #4)
> > Comment on attachment 8436002 [details] [review]
> > Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/20160
> > 
> > Sorry Eitan, this doesn't work for the large panels :(
> > 
> > I guess we'll need to use aria-hidden here...
> 
> How doesn't it work? Does it show/hide elements when it shouldn't?
> Performance?

Ah, just saw the PR comment..
Re-aligning priorities with 2.1 accessibility goals.
Whiteboard: [b2ga11y p=2] → [b2ga11y p=1]
Hey Milan,

when testing this patch, we see that when the slide animation starts (say: we want to display a thread, so the "inbox" panel and the "thread" panel move left), the panel that was not displayed (in that case: the thread) stays white for some time, whereas it should be displayed really at the start of the animation.

My initial guess was that the "will-change: scroll-position;" CSS property is doing this, but I tried removing it and there was no effect for this issue.

Do you think there is something we can do in Gaia? That there is an issue in Gecko?
Flags: needinfo?(milan)
Is this still the original bug?  Anyway, just looking at the video, it's probably a performance issue, but I'd really like to see what Flame does with this.  If the thing behind is not around anymore, either we have to display white and show the motion right away, or delay the motion before we render what we need to show.  I think showing white with the motion right away is a better way, but UX may want to chime in.
Flags: needinfo?(milan)
(In reply to Milan Sreckovic [:milan] from comment #11)
> Is this still the original bug?

Sorry, I don't follow the question. If the question is "does the proposed patch fix the issue in comment 0?" then yes :)

> Anyway, just looking at the video, it's
> probably a performance issue, but I'd really like to see what Flame does
> with this.  If the thing behind is not around anymore, either we have to
> display white and show the motion right away, or delay the motion before we
> render what we need to show.  I think showing white with the motion right
> away is a better way, but UX may want to chime in.

I think that displaying white looks broken, but delaying the motion can be too long and will look jerky (because I think we'll skip the steps instead of just delaying, right?).

IMO the long-term fix will be haida (separate windows should be faster to repaint), the short-term fix is to use aria-hidden.
(In reply to Julien Wajsberg [:julienw] (PTO 08/20 -> 09/15; contact schung instead) from comment #12)
> IMO the long-term fix will be haida (separate windows should be faster to
> repaint), the short-term fix is to use aria-hidden.

Our support for aria-hidden is about to get an upgrade, thanks to bug 1054454. I am going to have another go at this.. sorry for the delay.
Assignee: nobody → eitan
Comment on attachment 8436002 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/20160

I updated to PR to use aria-hidden
Attachment #8436002 - Flags: review- → review?(felash)
Comment on attachment 8436002 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/20160

Redirecting review request to Oleg as I'm in holidays soon and won't be able to deal with follow-ups.

Oleg, please especially check that we see both panels during the sliding transition; I'm afraid that since the patch still uses "visibility" we'd still have the issue outlined in comment 9.
Attachment #8436002 - Flags: review?(felash) → review?(azasypkin)
(In reply to Julien Wajsberg [:julienw] (PTO 08/19 -> 09/15; contact schung for SMS matters) from comment #15)
> Comment on attachment 8436002 [details] [review]
> Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/20160
> 
> Redirecting review request to Oleg as I'm in holidays soon and won't be able
> to deal with follow-ups.
> 
> Oleg, please especially check that we see both panels during the sliding
> transition; I'm afraid that since the patch still uses "visibility" we'd
> still have the issue outlined in comment 9.

We only use visibility for the edit header/buttons, not for the panel. So I don't think it should be an issue.
Sorry, still didn't have time to look at this patch, but it's on the top of my list for Monday.
Comment on attachment 8436002 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/20160

Everything looks good to me and I don't see any issues while transitioning.  I've just left one question on GitHub, once we sort out it, I think we're good to go. 

Also, please rebase your PR on the latest master, seems like we have some merge conflicts.

Thanks!
Attachment #8436002 - Flags: review?(azasypkin)
Comment on attachment 8436002 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/20160

Alrite.. I took your suggestion in the PR. here is another version.
Attachment #8436002 - Flags: review?(azasypkin)
Comment on attachment 8436002 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/20160

Eitan, now it looks good to me, except that screen reader doesn't see Report and Participants panels. I've left comment on Github about that + few tiny stylistic nits.

Thanks!
Attachment #8436002 - Flags: review?(azasypkin)
Comment on attachment 8436002 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/20160

Oops, I forgot to reflag this for review. Just rebased again, and corrected the issues you mentioned in the PR.
Attachment #8436002 - Flags: review?(azasypkin)
Comment on attachment 8436002 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/20160

Looks good now, r=me, just few indentation nits on Github.

Thanks!
Attachment #8436002 - Flags: review?(azasypkin) → review+
[Blocking Requested - why for this release]:

This accounts for a round of accessibility improvements for the SMS app that are required for the screen reader. The changes here are critical for basic functionality. The review process started a while ago, but slipped the branching deadline.
blocking-b2g: --- → 2.1?
triage: UI flaw.
blocking-b2g: 2.1? → 2.1+
Comment on attachment 8436002 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/20160

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined: SMS app is unusable to screen reader users.
[Testing completed]: yes.
[Risk to taking this patch] (and alternatives if risky): Low risk for general, non-screen reader users since this is mostly done only with aria-hidden which has no impact on layout.
[String changes made]:
Attachment #8436002 - Flags: approval-gaia-v2.1?
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Attachment #8436002 - Flags: approval-gaia-v2.1? → approval-gaia-v2.1+
You need to log in before you can comment on or make changes to this bug.