995035 - Invisible UI elements are visible to screen reader

Status: RESOLVED FIXED

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

Description

[Eitan Isaacson [:eeejay]]

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.

Comment 1

[Julien Wajsberg [:julienw]]

will be a lot easier once bug 881469 lands.

Comment 2

[Eitan Isaacson [:eeejay]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/20160

Comment 3

[Julien Wajsberg [:julienw]]

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.

Comment 4

[Julien Wajsberg [:julienw]]

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

Comment 5

[Eitan Isaacson [:eeejay]]

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

Comment 6

[Eitan Isaacson [:eeejay]]

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

Comment 7

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Comment on attachment 8436002 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/20160

Cancelling review

Comment 8

[Eitan Isaacson [:eeejay]]

Re-aligning priorities with 2.1 accessibility goals.

Comment 9

[Julien Wajsberg [:julienw]]

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?

Comment 10

[Julien Wajsberg [:julienw]]

Attachment: video demonstrating the issue

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 12

[Julien Wajsberg [:julienw]]

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

Comment 13

[Eitan Isaacson [:eeejay]]

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

Comment 14

[Eitan Isaacson [:eeejay]]

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

Comment 15

[Julien Wajsberg [:julienw]]

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.

Comment 16

[Eitan Isaacson [:eeejay]]

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

Comment 17

[Oleg Zasypkin [:azasypkin]]

Sorry, still didn't have time to look at this patch, but it's on the top of my list for Monday.

Comment 18

[Oleg Zasypkin [:azasypkin]]

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!

Comment 19

[Eitan Isaacson [:eeejay]]

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.

Comment 20

[Oleg Zasypkin [: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!

Comment 21

[Eitan Isaacson [:eeejay]]

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.

Comment 22

[Oleg Zasypkin [: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!

Comment 23

[Eitan Isaacson [:eeejay]]

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

Comment 24

[Eitan Isaacson [:eeejay]]

https://github.com/mozilla-b2g/gaia/commit/2317e0875826195377566c504e7e7fe819193fd8

Comment 25

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: UI flaw.

Comment 26

[Eitan Isaacson [:eeejay]]

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]:

Comment 27

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/40572280481d3f199ae25c1c072c64dd9a33c34f