Yura Zenevich [:yzen] (please use "needinfo") Reporter
	Description • 9 years ago

Another -moz-element used to display the statusbar. Caused by bug 1067913

	Ross Assignee
	Comment 2 • 9 years ago

So, after playing around with numerous animation ideas trying to keep things consistent from a UI standpoint, I realized that the Utilty Tray doesn't actually invoke an animation when the screenreader is active (it fires UtilityTray.show() with the 'instant' argument set to true -- More here: https://github.com/mozilla-b2g/gaia/blob/bc026b7a39dbc03bb6ce7a89569b24a0a512fbd0/apps/system/js/utility_tray.js#L465-#L493 and here: https://github.com/mozilla-b2g/gaia/blob/bc026b7a39dbc03bb6ce7a89569b24a0a512fbd0/apps/system/js/utility_tray.js#L257-L259).

With that in mind, it seems the most elegant solution is just to make the appropriate instantaneous changes to #statusbar and #statusbar-tray when the screenreader is active.

Let me know what you think -- tried to keep things simple. :)

	Yura Zenevich [:yzen] (please use "needinfo") Reporter
	Comment 3 • 9 years ago

Hi Ross, sorry for the delay!

(In reply to Ross from comment #2)
> So, after playing around with numerous animation ideas trying to keep things
> consistent from a UI standpoint, I realized that the Utilty Tray doesn't
> actually invoke an animation when the screenreader is active (it fires
> UtilityTray.show() with the 'instant' argument set to true -- More here:
> https://github.com/mozilla-b2g/gaia/blob/
> bc026b7a39dbc03bb6ce7a89569b24a0a512fbd0/apps/system/js/utility_tray.js#L465-
> #L493 and here:
> https://github.com/mozilla-b2g/gaia/blob/
> bc026b7a39dbc03bb6ce7a89569b24a0a512fbd0/apps/system/js/utility_tray.js#L257-
> L259).
> 
> With that in mind, it seems the most elegant solution is just to make the
> appropriate instantaneous changes to #statusbar and #statusbar-tray when the
> screenreader is active.
> 
> Let me know what you think -- tried to keep things simple. :)

I agree that this would work, however I would prefer we do not use screenreader class if possible. It is added mostly to be able to work with edge-gestures and should not be overused. 

When screen reader is not on, we have transitions. We need to insure that we display -moz-element(#statusbar); when transition is happening (performance afaik). As soon as transition is over, and utility tray is opened fully, we need to place the actual status bar over it (or perhaps what you suggest with removing background for the time being). 

From what I can tell this should also take care of the screen reader case because we ignore the transition altogether and just display the status bar in fully opened utility tray mode.

Please let me know if this works for you, if not we ll think of something else..

	Ross Assignee
	Comment 5 • 9 years ago

(In reply to Autolander from comment #4)
> Created attachment 8582917 [details] [review]
> [gaia] FunkTron:Bug1135233 > mozilla-b2g:master

This actually replaces the previous PR, which was accidentally closed and unable to be reopened.

	Ross Assignee
	Comment 6 • 9 years ago

Hey Yura, no worries on the delay.

When I first tried to do this, I attempted to get everything done using CSS animations (just tried to keep everything in as few places as possible, without adding extra conditions).  When I went that route though, there were a few UI glitches (such as the icons flickering) that left me a little dissatisfied with the results.  Those results were my main motivation for choosing to use the 'screenreader' class and focus on instantaneous changes.

This time around, I left the animations alone and just went with an intermediary 'in-transition' class and things actually seem to be pretty smooth (no flicker).

If this looks good, let me know, and I'll write a test checking for the class.

	Yura Zenevich [:yzen] (please use "needinfo") Reporter
	Comment 7 • 9 years ago

Hi Ross it looks pretty good. Definitely works well a11y wise! There one visual regression though. It is clearly visible when you don't use the screen reader and want to close the utility tray. What can be observed - status bar icons flicker. My guess is that the styles are changed to early, and not on the right end of transition. Check if you see the same thing, once we take care of it I think we should be done.

	Ross Assignee
	Comment 8 • 9 years ago

Okay, made the changes to get rid of the flicker (was just a z-index thing).  Also added tests to the PR.  Let me know!

	Yura Zenevich [:yzen] (please use "needinfo") Reporter
	Comment 9 • 9 years ago

Hi Ross, the tests look nice, however with the last commit the real status bar icons are off screen when utility tray is open.

	Yura Zenevich [:yzen] (please use "needinfo") Reporter
	Comment 10 • 9 years ago

Comment on attachment 8582917 [details] [review]
[gaia] FunkTron:Bug1135233 > mozilla-b2g:master

Actually scratch the last comment, I accidentally tested the wrong branch. With your PR applied, the status bar works great. I added 1 comment about a comment, but I think it's ready for review. Thanks! PS. Don't forget to squash your commits.

	Ross Assignee
	Comment 11 • 9 years ago

Okay, sounds good.  I went ahead and added a comment describing the changes to z-index as you suggested.

	Kevin Grandon :kgrandon
	Comment 12 • 9 years ago

Comment on attachment 8582917 [details] [review]
[gaia] FunkTron:Bug1135233 > mozilla-b2g:master

This looks pretty good, but for me to review it please do the following:

1 - Squash the commit into a single commit and add the bug number in the format of "Bug [num] - [Description]. You can use the git rebase command for this.
2 - Rebase against master to resolve the conflicts.

Once this is done please re-flag me or another system peer. Thanks!

	Ross Assignee
	Comment 13 • 9 years ago

Whoa, the rebase was quite a bit different than what I was working with!  Made the applicable changes though and squashed the commits.  Let me know if there's anything else!

	Kevin Grandon :kgrandon
	Comment 14 • 9 years ago

Comment on attachment 8582917 [details] [review]
[gaia] FunkTron:Bug1135233 > mozilla-b2g:master

Guillaume - Can I forward this statusbar patch review to you?

Please try to test it for regressions if you get a chance. Thanks!

	Guillaume Marty [:gmarty]
	Comment 15 • 9 years ago

Comment on attachment 8582917 [details] [review]
[gaia] FunkTron:Bug1135233 > mozilla-b2g:master

The code looks good to me.
However, I'd ask for a ui-r+ before landing as this patch changes how the status bar and utility tray interact together. If you do, Eric Pang is the one to ping.

	Ross Assignee
	Comment 16 • 9 years ago

>  Flags: ui-review?(epang@mozilla.com)

Hey Eric,

Had a recommendation to get your approval on this, so thought I'd send it your way.  Let me know if you have any considerations on the patch.  Thanks.

	Eric Pang [:epang] UX
	Comment 17 • 9 years ago

Hi Ross,

I've taken a look at the patch and I'm a bit concerned how the apps indicators transition (left in the attached image).  Because they overlay the handle when closing/opening the tray this causes the transition to look broken.  The image on the right shows the transition of the icons within the tray which looks really smooth.  I'm not completely clear on the accessibility issue, with a better understanding we should be able to find a way to make this work on all sides :).  Flagging Rob from UX for his input as well.  Thanks!

	Ross Assignee
	Comment 18 • 9 years ago

Hey Eric,

I see what you're saying, and I think with the latest update to the PR, we should be all set.  In any case, here's a quick rundown of what's going on...

The problem: Currently in Master, the icons in the "Statusbar" area of the "Utility Tray" are represented by a background-image.  This is a no-go for accessibility because the icons are both unresponsive to touch events and unable to be found/highlighted by the Virtual Cursor in the screen reader.

The solution: We allow the icons to be represented by a background image during the course of the transition for the opening/closing of the Utility Tray, but revert back to using the actual "Statusbar" once the "Utility Tray" is open so that they remain accessible for the screen reader.

Explanation of Most Recent Changes to PR: I fixed the icon overlay issue you identified at the outset of the "Utility Tray Open" transition by waiting for the presence of the .utility-tray class before allowing the "Statusbar" to sit on top of the "Utility Tray".   I additionally cleaned up the usage of the background image so that it's now in effect during the course of the whole transition, rather than just after the .utility-tray class was added.  This second change was made to prevent "icon flicker" at the end of the "Utility Tray Close" transition.

It looks to me that things are now consistent with the intended experience, but let me know if anything else is amiss.

	Rob MacDonald [:robmac]
	Comment 19 • 9 years ago

Hi Ross, Eric...

I just took a look at the transition issue that Eric pointed out and it appears to have been fixed. Thanks for taking care of this. 

FYI - Eric is away until Tuesday UK time and will likely respond then. 

- Rob

	Eric Pang [:epang] UX
	Comment 20 • 9 years ago

Comment on attachment 8582917 [details] [review]
[gaia] FunkTron:Bug1135233 > mozilla-b2g:master

Thinks for the clarification Ross!  This looks good to me now, R+

	Ross Assignee
	Comment 21 • 9 years ago

Hey Kevin,

I'm contacting you because I know you've manually landed a few of my other patches, but apologies if this should go to somebody else.  Anyway, it looks like all considerations have been satisfied to get this patch landed, but due to the fact that there are actually 2 Pull Requests featured in this thread (the first one is closed and not needed), I'm reluctant to use the "checkin-needed" keyword.  I guess I'm just trying to avoid any unexpected behavior and/or confusion by Autolander.

Would you be able to land this for me?  And just for future knowledge, would I still be okay using 'checkin-needed' here despite the 2 PR's?

	GitHub Autolander Bot
	Comment 22 • 9 years ago

http://docs.taskcluster.net/tools/task-graph-inspector/#6RQ6zYSFTyWlIu2_WsyWUg

The pull request failed to pass integration tests. It could not be landed, please try again.

	Ross Assignee
	Comment 23 • 9 years ago

Well, I guess I answered my own question and accidentally applied the keyword anyway.  Sorry about that.  It looks like Autolander ignored the first PR (which is good) and rejected the merge for the 2nd (proper) one due to failed integration tests (apparently CSSLint doesn't like "background-image:-moz-element(#statusbar)" as a solo rule, and considers it empty), so perhaps a manual landing is needed after all.  Please let me know where to go from here.  Thanks again.

	Kevin Grandon :kgrandon
	Comment 24 • 9 years ago

Comment on attachment 8578351 [details] [review]
[gaia] FunkTron:Bug1135233 > mozilla-b2g:master

It's best to just obsolete one then land, though I don't think there's a review from a listed peer yet?

	Kevin Grandon :kgrandon
	Comment 25 • 9 years ago

(In reply to Ross from comment #23)
> Well, I guess I answered my own question and accidentally applied the
> keyword anyway.  Sorry about that.  It looks like Autolander ignored the
> first PR (which is good) and rejected the merge for the 2nd (proper) one due
> to failed integration tests (apparently CSSLint doesn't like
> "background-image:-moz-element(#statusbar)" as a solo rule, and considers it
> empty), so perhaps a manual landing is needed after all.  Please let me know
> where to go from here.  Thanks again.

If the test failed it's because of the CSSLint error. We can't land if it failed because it's likely to fail after landing. Please address this first before attempting landing again.

	Ross Assignee
	Comment 26 • 9 years ago

>It's best to just obsolete one then land, 
>though I don't think there's a review from a listed peer yet?

Okay cool, noted about the obsolete.  As far as the approval is concerned, it was approved initially as R+, but a small change was made afterward to address a UI issue before it could be granted UI-R+.  Does it need to be reviewed again and marked R+ to be landed?

>If the test failed it's because of the CSSLint error. We can't land
>if it failed because it's likely to fail after landing.
>Please address this first before attempting landing again.

Okay.  This actually just stems from CSSLint thinking that a valid rule is empty when it isn't.  I'm not sure how I should go about addressing that, but perhaps I can work with Yura and come up with something.

	Kevin Grandon :kgrandon
	Comment 27 • 9 years ago

(In reply to Ross from comment #26)
> >It's best to just obsolete one then land, 
> >though I don't think there's a review from a listed peer yet?
> 
> Okay cool, noted about the obsolete.  As far as the approval is concerned,
> it was approved initially as R+, but a small change was made afterward to
> address a UI issue before it could be granted UI-R+.  Does it need to be
> reviewed again and marked R+ to be landed?

It's probably fine, and I could just leave a R+ stamp if needed and all of the tests are passing.

> >If the test failed it's because of the CSSLint error. We can't land
> >if it failed because it's likely to fail after landing.
> >Please address this first before attempting landing again.
> 
> Okay.  This actually just stems from CSSLint thinking that a valid rule is
> empty when it isn't.  I'm not sure how I should go about addressing that,
> but perhaps I can work with Yura and come up with something.

Yeah, we either need a fix or workaround for it, or we need to fix the CSSLinter in a follow-up. Maybe there's a new version out that would fix it?

	Kevin Grandon :kgrandon
	Comment 28 • 9 years ago

(In reply to Ross from comment #26)
> Okay.  This actually just stems from CSSLint thinking that a valid rule is
> empty when it isn't.  I'm not sure how I should go about addressing that,
> but perhaps I can work with Yura and come up with something.

Could this possibly be bug 1053721?

	Ross Assignee
	Comment 29 • 9 years ago

(In reply to Kevin Grandon :kgrandon from comment #28)
> Could this possibly be bug 1053721?

Hmm, yup, it's definitely the same "empty rule" error.  In this bug's case, instead of CSS variables causing the issue, it's the "-moz-element" value that CSSLint doesn't appear to like.  I looked around on the CSSLint page for future support for '-moz-element' but I didn't see anything definitive. I did see this about implementing ignores though: https://github.com/CSSLint/csslint/issues/558

Do you think adding "apps/system/style/utility_tray/utility_tray.css" to the "https://github.com/mozilla-b2g/gaia/blob/2bac78bdd09552ce42fa4eee88d96483b0a66dcf/build/csslint/xfail.list" file would be enough here?

	Ross Assignee
	Comment 30 • 9 years ago

Hey again Kevin,

If it does indeed look like adding the file to xfail.list will do the trick here per the recommendations from Bug 1053721, I've gone ahead and revised the PR to include that change.

	Kevin Grandon :kgrandon
	Comment 31 • 9 years ago

Comment on attachment 8582917 [details] [review]
[gaia] FunkTron:Bug1135233 > mozilla-b2g:master

More or less just review-stamping this based on Yuras review. Would be nice to have some way of writing integration tests for these..

	GitHub Autolander Bot
	Comment 32 • 9 years ago

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/d0bf656cdfa9d69721608c30bb5a79886b5e64d3