1115829 - Exploring status bar icons by touch again broken for the screen reader

Status: VERIFIED FIXED

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

Description

[Marco Zehe (:MarcoZ)]

[Tracking Requested - why for this release]:

Exploring the status bar icons with the screen reader is once again broken in 2.2 nightly builds. They can only be swiped to by navigational swipes left and right. To reproduce:

1. Configure your device.
2. Turn on screen reader by rocking the volume up, then down 3 times, then 3 times more after you hear the announcement.
3. On the home screen, touch the area where the status bar icons are.

Expected: You should hear an icon, and it should be highlighted.
Actual: Nothing is found.

5. Touch the address bar.
6. Perform a swipe to the left a few times and listen.

Result: Now, the status bar icons are spoken.

So they can be swiped to, but not touched.

Comment 1

[Evan Tseng [:evanxd]]

Yes, it could be reproduced in 2.2[1].
And I could not reproduce this in 2.1[2], so it is a regression issue.

[1]: The environment:
Build ID               20141229160214
Gaia Revision          322ef5116a5827a30c9a3cd9b842449a9c66a5b3
Gaia Date              2014-12-29 18:03:02
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/2c0ef7315830
Gecko Version          37.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20141229.192338
Firmware Date          Mon Dec 29 19:23:49 EST 2014
Bootloader             L1TC00011880

[2]: The environment:
Build ID               20141229161203
Gaia Revision          73be51f998031f06db0cd660c0e388fa621c9f4c
Gaia Date              2014-12-26 16:54:56
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/ea426e47bfc4
Gecko Version          34.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20141229.192338
Firmware Date          Mon Dec 29 19:23:49 EST 2014
Bootloader             L1TC00011880

Comment 2

[Pi Wei Cheng [:piwei] (inactive)]

b2g-inbound regression window:

Last Working Environmental Variables:
Device: Flame
BuildID: 20141218042802
Gaia: 99be66baf4146835bed70e8105c2b8934af5cb7d
Gecko: 577aefbf76c3
Version: 37.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

First Broken Environmental Variables:
Device: Flame
BuildID: 20141218050403
Gaia: a1ec2268e7b467aa0ff07a2a3cbd1b9abc4e4dd1
Gecko: 32d360316ff3
Version: 37.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

First Broken Gaia & Last Working Gecko - issue DOES repro
Gaia: a1ec2268e7b467aa0ff07a2a3cbd1b9abc4e4dd1
Gecko: 577aefbf76c3

First Broken Gecko & Last Working Gaia - issue does NOT repro
Gaia: 99be66baf4146835bed70e8105c2b8934af5cb7d
Gecko: 32d360316ff3

https://github.com/mozilla-b2g/gaia/compare/99be66baf4146835bed70e8105c2b8934af5cb7d...a1ec2268e7b467aa0ff07a2a3cbd1b9abc4e4dd1

Caused by Bug 1074332.

Comment 3

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

Caused by bug 1074332 - Alberto can you take a look please?

Comment 4

[GitHub Autolander Bot]

Attachment: [PullReq] albertopq:1115829-screen-reader to mozilla-b2g:master

Comment 5

[Alberto Pastor [:albertopq]]

Comment on attachment 8544455 [details] [review]
[PullReq] albertopq:1115829-screen-reader to mozilla-b2g:master

I would like to hear Etienne's opinion here. We are using mozelements now for the statusbar, so the screenreader can't access to the icons. This patch uses the real statusbar when the screenreader is active, but I'm not sure if is the best solution. Any idea?

Comment 6

[Etienne Segonzac (:etienne)]

Comment on attachment 8544455 [details] [review]
[PullReq] albertopq:1115829-screen-reader to mozilla-b2g:master

The change looks okay, but I want to make sure it's really needed before adding a screenreader exception.

The way Vivien made it work originally was to display the in-app status bar (moz-element) during transitions and edge gestures but to always make the real "canonical" status bar visible while the UI was at rest.

Looks like bug 1074332 changed this, but was it done deliberately to fix something else or were we just missing an integration test? :)

Comment 7

[Alberto Pastor [:albertopq]]

It was intentionally done in order to have 1 statusbar per app (and avoid all the icons color issues that we had with only 1 statusbar). The other main reason was to, when opening the utility tray on an app with different icon colors, being able to transition between those 2 colors correctly.

May be we need to go back to only one statusbar, and only apply the shadows on the gestures and utility tray, but my impression is that we'll go back to icon colors exceptions again.

Comment 8

[Etienne Segonzac (:etienne)]

(In reply to Alberto Pastor [:albertopq] from comment #7)
> It was intentionally done in order to have 1 statusbar per app (and avoid
> all the icons color issues that we had with only 1 statusbar).

Not sure I follow this part (since the "before" also had 1 statusbar per app), do you mean 1 moz-element + filter or a completely separate dom?

Comment 9

[Alberto Pastor [:albertopq]]

Yes, the filter was applied to the statusbar, not to the mozelement. Now we always hide the real statusbar, and show the mozelement of that specific app, with the filter applied to it. Does that make sense?

Comment 10

[Etienne Segonzac (:etienne)]

(In reply to Alberto Pastor [:albertopq] from comment #9)
> Yes, the filter was applied to the statusbar, not to the mozelement. Now we
> always hide the real statusbar, and show the mozelement of that specific
> app, with the filter applied to it. Does that make sense?

Yep.

Kinda sad the original code wasn't commented/tested better :/
The accessibility regression shouldn't have been a surprise since this new approach removed code that was here solely for accessibility purpose.

Anyway, onward.
I don't know enough about the icon color exceptions we had in the past (how bad, numerous they were) to make a judgment here, but I trust yours!
Let's make sure we have a solid integration test for this screenreader-only code path.

Comment 11

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Alberto Pastor [:albertopq] from comment #5)
> Comment on attachment 8544455 [details] [review]
> [PullReq] albertopq:1115829-screen-reader to mozilla-b2g:master
> 
> I would like to hear Etienne's opinion here. We are using mozelements now
> for the statusbar, so the screenreader can't access to the icons. This patch
> uses the real statusbar when the screenreader is active, but I'm not sure if
> is the best solution. Any idea?

I would strongly argue against using screenreader specific code path. This patch uses .screenreader as an implementation approach where the intention was to use it where screen reader truly requires alternative interactions (such as edge gestures). Ideally, we should have something similar to what used to be prior to this regression, otherwise we are setting ourselves up for another regression as soon as statusbar/utility tray/app titlebar stuff is revisited.

Comment 12

[Alberto Pastor [:albertopq]]

Thanks for the feedback guys. I'll work on going back to show the real statusbar always but when transitioning. I'll try to get the color filter working with the app window titlebar instead of the statusbar as well, so we get the best thing of both approaches.

Comment 13

[Alberto Pastor [:albertopq]]

Attachment: Link to Pull Request: https://github.com/mozilla-b2g/gaia/pull/27190

Comment 14

[Alberto Pastor [:albertopq]]

Comment on attachment 8545222 [details] [review]
Link to Pull Request: https://github.com/mozilla-b2g/gaia/pull/27190

Is basically a backout of the statusbar.css and app_titlebar.css of bug 1074332. I restored the tests again in order to check the real statusbar and not the shadows. Everything is passing but 1 error with the FTU (which seems to fail in master currently)

Comment 15

[Alberto Pastor [:albertopq]]

I think the test that is failing is related to Bug 1118361

Comment 16

[Etienne Segonzac (:etienne)]

Comment on attachment 8545222 [details] [review]
Link to Pull Request: https://github.com/mozilla-b2g/gaia/pull/27190

The change looks good, please update the commit message to explicitly say that this is a "partial backout of the statusbar changes from bug XXXX".

Guillaume might have ideas about the permared test so pinging him and redirecting the final review while I'm at it :)

Comment 17

[Guillaume Marty [:gmarty]]

The test in the FTU is failing because the minimised status bar is showing instead of the maximised.
Alberto, can you fix this?

Comment 18

[Guillaume Marty [:gmarty]]

Comment on attachment 8545222 [details] [review]
Link to Pull Request: https://github.com/mozilla-b2g/gaia/pull/27190

Looks good to me! You need to rebase though.

Comment 19

[Alberto Pastor [:albertopq]]

master: https://github.com/mozilla-b2g/gaia/commit/c94faf8a21acd2a381014c45b0fe4aa58d965270

Comment 21

[Ryan VanderMeulen [:RyanVM]]

This missed the v2.2 branching. Please request Gaia v2.2 approval for this when you get a chance.

Comment 22

[Alberto Pastor [:albertopq]]

Comment on attachment 8545222 [details] [review]
Link to Pull Request: https://github.com/mozilla-b2g/gaia/pull/27190

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): 1074332 
[User impact] if declined: Screenreader doesn't work properly on the statusbar
[Testing completed]: Restored all the ui tests, for checking all the statusbar visibility cases.
[Risk to taking this patch] (and alternatives if risky): This is a partial backout. This is the less risky option to make the screenreader work again.
[String changes made]: -

Comment 23

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/5abf05b5987985c312a55026fc3519792fe0b78b

Comment 24

[Yeojin Chung [:YeojinC]]

This issue is verified fixed on Flame Master.

Result: The status bar icons are selected properly when touched, and the screen reader can be heard.
 
Device: Flame Master (319mb, full flash)
BuildID: 20150121010204
Gaia: 5e98dc164b17fd6decb48a9eaddef0e55b82e249
Gecko: 540077a30866
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 38.0a1 ( Master)
Firmware: V18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0

Comment 25

[KTucker [:KTucker][Inactive 3/4/2016]]

Yeojin, can you please verify 2.2 as well?

Comment 26

[Yeojin Chung [:YeojinC]]

This issue is verified fixed on Flame 2.2.

Result: The status bar icons are selected properly when touched, and the screen reader can be heard.

Environmental Variables:
Device: Flame 2.2 (319mb, full flash)
BuildID: 20150121002607
Gaia: e4f9b5da3751798f9cc5d95f302c30722cc11fca
Gecko: 75a462a58d7a
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0a2 (2.2)
Firmware: V18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0