As discussed with :mhenretty; taking to investigate.

This patch updates the utility tray to make use of native scrolling, leading to better performance. Flagging for `feedback?` rather than review, because I haven't yet updated the tests. I'm working on the tests now, and will ask for a full review pass when I've been able to incorporate your feedback and update the tests. For best understanding, I'd recommend visiting js/utility_tray_motion.js first, which contains detailed comments on the implementation of the tray's scroll and motion behavior, followed by js/utility_tray.js. Since native scrolling limits us in ways touches do not, the behavior of the tray has changed slightly, as follows: - In order to maintain the "peek at notifications" behavior, the transition is slightly different. Previously, the tray appeared stationary as you swiped down, i.e. just being unclipped as the tray expanded. This cannot be done currently with scrolling alone. Instead, we hide the footer when the tray is in transition. Along with careful use of "position: sticky", we retain the ability to peek at notifications in this patch. - You can now drag from most any area of the tray, rather than only the bottom grippy. - The momentum of the tray differs slightly, but should feel more natural and platform-like. I've been careful to ensure this behaves correctly when faced with screen rotation, software-home-button settings changes, rocketbar clicks, and dragging from the bottom and top of the screen. There are a lot of edge cases to consider, though, so do let me know if I missed something. Also, as this is my first contribution to System, feel free to note if I've missed other style/conventions as well.

Comment on attachment 8639518 [details] Gaia PR <https://github.com/mozilla-b2g/gaia/pull/31084> Overwhelmingly positive feedback :) Left a few comments on github. The work left is mostly testing indeed, my recommendation would be to * make sure the marionette tests pass, should be easy-enough since updating the test/marionette/lib/utility_tray.js should cover most of it * add basic unit testing for UtilityTrayMotion ie. events in, events out and maybe some `scrollTo` spying to cover open()/close(). * remove most of the failing UtilityTray unit tests since they're not relevant anymore I'd like to make sure we file/reference the scroll snapping bugs too. From my testing "flinging" was actually working okay but it's really easy to get the utility tray "stuck" by doing a drag + tap. Cheers!

Hey Rob, I know :mhenretty showed you this on a phone, and that you had some concerns about the UX implications vs. performance, in particular regarding peekability. (Here, we retain the ability to peek at all notifications, but if there are multiple notifications, you see the oldest one first as you pull down.) Do you think this is worth landing? If not, what modifications would convince you it's worth it? I don't think it's possible to improve the peek behavior beyond what you see here and still retain the performance improvements due to platform limitations. Most of the people I've talked to believe the performance is worth the modified peekability, but in the end, it's your call.

Hi Marcus... I talked with Michael a bit about this last week. I was hoping that we might be able to retain the peakability but the performance gains are so huge that the trade off is more than worth it. The tray feels really natural and I like how easy it is to close. I'm off next week and won't get a chance to review the latest patch before I go. So, if you've made a lot of changes, track down Francis in the office and he can do a quick review. Otherwise, big thumbs up on this. Thanks! - Rob

(In reply to Rob MacDonald [:robmac] from comment #4) > Hi Marcus... > > I talked with Michael a bit about this last week. I was hoping that we might > be able to retain the peakability but the performance gains are so huge that > the trade off is more than worth it. Hey! Since the UX team is on board, any news? update? blocking issue?

No blocking issues; it's been a lower priority the past couple weeks due to task manager work. I'll try to push this forward sometime this week.

Comment on attachment 8639518 [details] Gaia PR <https://github.com/mozilla-b2g/gaia/pull/31084> I've added the test coverage and addressed your feedback in a second commit in the same pull request. (I'll squash the commits when it's time to land.) I removed the outdated motion and swipe checking from unit tests, and added additional coverage to the integration tests to compensate. Instead, the unit tests now cover basic sanity-checking: that UtilityTrayMotion properly scrolls its element, and that the UtilityTray handles the rocketbar element edge cases correctly. The integration tests remain largely unchanged, with some additional verification to ensure that show and hide events are dispatched appropriately when the tray moves. I've removed the tray-motion-moved event per your github comment to reduce event churn. (Turns out we didn't need it anyway.)

Comment on attachment 8639518 [details] Gaia PR <https://github.com/mozilla-b2g/gaia/pull/31084> We're close! Made a few comments on github, but mostly about details. Hopefully the last review round will be the last, let me know if you have any issues getting a green try (it's not currently but that's not telling much this week :))

I'm sad to discover that GitHub eats my line-note comments when I post a new patch, so file notes here will have to suffice. Going from top to bottom in the latest commit: ## index.html Because our mozFullScreen mode forces us to have everything with a z-index of MAX_INT32, and because the utility tray previously came sequentially before #windows in the DOM, I had to move #windows up even higher in the DOM to make everything stack properly. ## js/app_statusbar.js It seems that along the way, support for the "drag once for statusbar, twice for utility tray" feature in mozFullScreen broke. Several changes here have been made to enable the proper interaction for both fullscreen modes; a more detailed comment and video have been left in a block comment in utility_tray.js. At the end, some additional event cleanup to ensure that when hiding the utility tray, we also release the appStatusbar. (Unit test added in app_statusbar_test.js.) ## js/app_window_manager.js I know we talked about not adding classes to #screen, but this appears to be a case where we still have references to `screen.classList.remove('fullscreen-app')` but lost the code that added that class. Given that we are already toggling classes here (line 845), adding this back in should make no performance difference. ## js/rocketbar.js Whitespace/obvious cleanup. ## js/statusbar.js These changes support doing the proper swipe sequence in fullscreen modes. (Again, noting here that we didn't seem to handle mozFullScreen properly before.) ## js/utility_tray.js Mostly additional documentation (and a link to the video) about the statusbar/utility-tray interaction in fullscreen mode. ## (the rest) The rest of the changes are mostly test fixes; I had missed a bunch in the previous round in the chaos of infrastructure issues. ---- I'm not seeing a completely green run at the moment; there appear to be a couple non-System app tests that are failing, for which I need to verify/triage in the morning to be sure I didn't regress. Posting here in case you'd like to look at it in the meantime; otherwise I'll submit r? again when the test run is fully green.

Comment on attachment 8639518 [details] Gaia PR <https://github.com/mozilla-b2g/gaia/pull/31084> This comment supersedes Comment 9. - Your GitHub comments have been addressed: * fullscreen-app and -moz-full-screen-ancestor support * incorrect bounce height on recoil with software-home-button * various comments/nits - The <div#windows> element had to be moved earlier in index.html to ensure proper stacking when in -moz-fullscreen-ancestor mode (where every z-index must equal MAX_INT). - The <div#tray-rocketbar> element has been removed. Instead, the utility tray's invisible gripper forwards clicks to the element or iframe underneath. `touch_forwarder.js` has been updated to forward touches and click events to normal HTML elements as well as iframes. The rocketbar click handling is now paired with <div.urlbar> in AppChrome. The new <div.urlbar-hit-area> extends the range of tappability to the top of the screen, to ensure proper hit detection. (It wasn't possible to achieve this with only margin and padding on the urlbar. - Fixed support for the fullscreen statusbar swipe-once-to-open, twice-for-utility-tray gestures. This required updating CSS and JS to properly support the two types of fullscreen modes: * -moz-full-screen-ancestor, tested with http://blogs.sitepointstatic.com/examples/tech/full-screen/index.html * fullscreen-app (from the manifest), tested with gallery/camera This required a couple z-index fixes as well to ensure correct stacking for the utility tray and #top-panel in all conditions. - Assorted test fixes, which unfortunately make this commit much larger than I had hoped. We should be fully green now: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=19d6dafd8026541d51fb28f1dbe59d2b9a5b9fb4

Comment on attachment 8639518 [details] Gaia PR <https://github.com/mozilla-b2g/gaia/pull/31084> Well sometimes thorough and thoughtful work leads to a big patches :) I have a couple of comments (some missing tests mostly) that need to be addressed but r=me with that! It took a lot to get it right, but it's really solid and I'll happily help out with any potential regression. Given the size/impact of the patch I'd like QA to give this a run before landing. Naoki, feel free to redirect, I wasn't sure who to ask. (Since my comments are mostly test-related the QA work can start right away.)

Thanks! For reference, the following commit contains the added tests and "poor-man's elementFromPoint": https://github.com/mcav/gaia/commit/2f164ae7ee431ff887d487e6eea3a2bdaa2427fd Feel free to note anything else you see there, otherwise I'll wait for QA approval to squash and land.

This commit uses constants per your suggestion: https://github.com/mozilla-b2g/gaia/commit/0d7738aad6ed3ac9f273e2fe6ffffcd4799bdd4c I have a question about reflow: I know that reading/writing from the DOM in ways which invalidate the layout will cause multiple reflows, and that we avoid that problem by grouping reads/writes together (leading to only one layout/reflow where possible). In this case, document.elementFromPoint is a read operation. Since we don't modify the DOM prior to `elementFromPoint`, and gecko must have necessarily ran layout in order to render the current state, I would have thought that reading `elementFromPoint` would just be able to use the already-calculated layout, rather than triggering a new layout. Is that not the case?

Comment on attachment 8639518 [details] Gaia PR <https://github.com/mozilla-b2g/gaia/pull/31084> So I took a look at the patch and played around with it some. There's 2 things I found: 1) sometimes I can get the notifications to appear without the status at the bottom at all or it has a 5 second delay or more of showing up 2) sometimes I can flick the notifications down and it will automatically scroll back up on me. I let mcav borrow my flame device that I have his patch on. Waiting for the next round. His testing was more on the aries device so that may explain why he didn't notice these things.

FWIW, I've been dog-fooding with this patch (on Aries) for weeks now and I haven't noticed anything major. The only thing I did notice (which may be fixed already?) was that the music player controls notification appears laid out incorrectly.

(In reply to Marcus Cavanaugh [:mcav] (Mozilla SF) from comment #13) > This commit uses constants per your suggestion: > > https://github.com/mozilla-b2g/gaia/commit/ > 0d7738aad6ed3ac9f273e2fe6ffffcd4799bdd4c > > I have a question about reflow: > > I know that reading/writing from the DOM in ways which invalidate the layout > will cause multiple reflows, and that we avoid that problem by grouping > reads/writes together (leading to only one layout/reflow where possible). > > In this case, document.elementFromPoint is a read operation. Since we don't > modify the DOM prior to `elementFromPoint`, and gecko must have necessarily > ran layout in order to render the current state, I would have thought that > reading `elementFromPoint` would just be able to use the already-calculated > layout, rather than triggering a new layout. Is that not the case? The correct term might be "style flush". My understanding is that there might be pending (async) reflows that all need to be flushed before the read. And this is definitely not free and shows up as "reflow" in the devtools.

(In reply to Chris Lord [:cwiiis] from comment #15) > FWIW, I've been dog-fooding with this patch (on Aries) for weeks now and I > haven't noticed anything major. The only thing I did notice (which may be > fixed already?) was that the music player controls notification appears laid > out incorrectly. Good catch! It's not fixed yet :)

Comment on attachment 8639518 [details] Gaia PR <https://github.com/mozilla-b2g/gaia/pull/31084> The latest commit fixes all known issues (and some unknown issues): - Media player buttons have been fixed; this was due to an overzealous CSS override (line 205, utility_tray.css). - AppChrome was improperly activating the rocketbar in response to clicking buttons like 'reload' (missing call to `evt.stopPropagation()`). Unit tests updated. - The footer feels much more responsive on all devices; I think it feels very natural now, whereas before it felt like a bug. To do this, I added the "tray-motion-footer-position" event, which only fires when the tray is almost open. This prevents the scroll-event-overloading problem seen with the previous tray-motion-scroll event. - The logic in utility_tray.js has been reworked to be clearer and more correct; this fixes both of Naoki's problems in my testing, which was caused by some incorrect logic, exacerbated by the Flame. - Notifications are somewhat less prone to inadvertent clicks, as they will no longer respond to taps or drags if the tray isn't fully open. The proper fix here will be to rework the notification list to only activate in response to 'click'. (Unit test added in notification_screen_test.js.) Naoki, I left your flame flashed with these changes on your desk in front of your keyboard on Friday. I have not squashed these changes in order to make it easy to review. (If it's still hard for you to test when PRs aren't squashed, let's figure out how to make it easy to do so; I'd be happy to contribute a script or something.) Direct link to the latest commit: https://github.com/mozilla-b2g/gaia/commit/1005fe3be0fcd5ead6c91900285b7b9fe8855845

Comment on attachment 8639518 [details] Gaia PR <https://github.com/mozilla-b2g/gaia/pull/31084> A few comments on github but carrying the r+ !

Out of all the concerning things, I think #3, #5, #6, #7 would be of concern. Issues occurs on both flame and aries. 1) In regards to the PR, for flame on landscape after flicking up and down and then pulling down, some of the notifications could get cut. See screenshot. I can't seem to repro. It might be because I hit some wierd crash which I can't also repro. Symbols aren't associated so it's hard to debug : https://crash-stats.mozilla.com/report/index/53c4d133-cc78-4a5c-ab39-400282150917 crash is probably unrelated anyhow. 2) the background of the widget will show through and over lap with the notification sometimes when pulling the notifications back up. 3) the bottom status doesn't update until you collapse the notifications : "ie unlock your sim to enable Usage", "Airplane mode, ..." : This is a regression. 4) I got the device stuck with a partial notification again using logshaking as an interrupt. I can't seem to reproduce this issue. 5) tapping on airplane mode, then tapping on wifi and tapping on settings, the notification panel doesn't dismiss. Settings does appear in the background. This is also a regression. 6) tapping on airplane mode, tapping on wifi, tapping on settings and then letting the device sleep and waking it up shows the notification pane; this is a regression. You can't even unlock the device on flame when you hit this. With Aries, the home button disappears on the lock screen giving you a small space to unlock the device. 7) tapping airplane mode then tapping home doesn't dismiss the notifications panel Note: The reasons I ask for a PR to be squashed is that : 1) it's easier to pull in 2) it's also easier to backout if something goes wrong. You may want to squash it before the main commit. QAing it isn't as much of an issue as long as the patches don't bitrot.

Comment on attachment 8639518 [details] Gaia PR <https://github.com/mozilla-b2g/gaia/pull/31084> Several regressions with the patch. Please look into those regressions. One is really bad where you can't unlock the phone unless you reboot for the flame.

Comment on attachment 8639518 [details] Gaia PR <https://github.com/mozilla-b2g/gaia/pull/31084> Thanks for the thoroughness, Naoki. Will address these and take another pass.

Comment on attachment 8639518 [details] Gaia PR <https://github.com/mozilla-b2g/gaia/pull/31084> Fortunately, most of the errors (#3, #5, #6, #7) discovered by Naoki all appear to originate from one logic error. I've addressed that issue and added tests accordingly. To mitigate issue #2 (footer-covering-notifications-briefly), I've reduced the animation duration when closing the tray. Unfortunately, there isn't a way to truly mitigate this due to APZ (we aren't guaranteed to receive events in time, particularly under load) without UX changes (e.g. we may be able to work around this in the future by making the footer opaque). I haven't been able to reproduce #1 and #4 either; it's possible that the logic error in this patch contributed to the problem. I've been testing on both Flame and Aries, and with this patch I have been unable to reproduce any of the errors described in Comment 20. Naoki, when you have a moment, please take another look. I can loan you a flashed Flame if you'd like.

Comment on attachment 8639518 [details] Gaia PR <https://github.com/mozilla-b2g/gaia/pull/31084> No issues with the last code change, nice testing. But I found a problem that might not be easy to fix :/ When you have a lot of notifications, the #notifcations-container is scrollable, so we have a nested APZ scrolling area inside the #utility-tray-motion. And when we reach the end of the nested scrolling area, gecko starts scrolling the parent one. The result is that if you have tons of notification and you want to flick to go to the bottom of the list, we close the utility tray :/ We could play with dynamically setting the overflow-y of motion to "hidden" but it's pretty costly so I'm not sure it's viable...

(In reply to Etienne Segonzac (:etienne) from comment #24) > Comment on attachment 8639518 [details] > Gaia PR <https://github.com/mozilla-b2g/gaia/pull/31084> > > No issues with the last code change, nice testing. > > But I found a problem that might not be easy to fix :/ > When you have a lot of notifications, the #notifcations-container is > scrollable, so we have a nested APZ scrolling area inside the > #utility-tray-motion. And when we reach the end of the nested scrolling > area, gecko starts scrolling the parent one. > > The result is that if you have tons of notification and you want to flick to > go to the bottom of the list, we close the utility tray :/ > > We could play with dynamically setting the overflow-y of motion to "hidden" > but it's pretty costly so I'm not sure it's viable... Perhaps scrollgrab could help here? (I'm not sure about the problem being described and don't have the patch to hand to test right now)

(In reply to Chris Lord [:cwiiis] from comment #25) > (In reply to Etienne Segonzac (:etienne) from comment #24) > > Comment on attachment 8639518 [details] > > Gaia PR <https://github.com/mozilla-b2g/gaia/pull/31084> > > > > No issues with the last code change, nice testing. > > > > But I found a problem that might not be easy to fix :/ > > When you have a lot of notifications, the #notifcations-container is > > scrollable, so we have a nested APZ scrolling area inside the > > #utility-tray-motion. And when we reach the end of the nested scrolling > > area, gecko starts scrolling the parent one. > > > > The result is that if you have tons of notification and you want to flick to > > go to the bottom of the list, we close the utility tray :/ > > > > We could play with dynamically setting the overflow-y of motion to "hidden" > > but it's pretty costly so I'm not sure it's viable... > > Perhaps scrollgrab could help here? (I'm not sure about the problem being > described and don't have the patch to hand to test right now) I think we want the opposite of scrollgrab :) ie. don't propagate to the parent scrolling area instead of starting there.

(In reply to Etienne Segonzac (:etienne) from comment #26) > I think we want the opposite of scrollgrab :) > ie. don't propagate to the parent scrolling area instead of starting there. Oh, I think I know what you mean now - that the fling continues on to other containers? Irritatingly, there are some things that break when we don't have that behaviour (most importantly, the dynamic toolbar)... I think your suggestion of modifying overflow dynamically is probably the only current way to go about it, but also like you say, error/jank prone... I guess we want some kind of 'flinggrab' behaviour, but that's almost certainly not the nicest way to express it...

Hmm, yes, that's a tough one. I've tried to mitigate that problem in the past, by modifying 'overflow' as you suggest, but with APZ it's really jank-prone. Another suggestion was to add extra dead space at the bottom of the notifications container (i.e. a tall blank div as the last child of the notifications-container), but that didn't really help because scroll velocity is so powerful.

With the latest patch I noticed this in the console log: 1) E/GeckoConsole( 319): [JavaScript Error: "TypeError: timestamps[i] is undefined" {file: "app://system.gaiamobile.org/js/notification_screen.js" line: 381}] To note, I installed Vaani which gives a notification that the mic is on in the notification panel. 2) I ran into an issue where the screenshots weren't pulling up, and logs were auto defaulting to bugzilla lite... having said that somewhere along the line it fixed itself? Not sure. 3) I also ran into etienne's situation. It's especially apparent when you have music running and a couple of notifications. 4) The issues I mentioned in the previous comment with 3, 5, 6, 7 seemed to be resolved.

addition to part 3 of last comment, you have to be in landscape. landscape + music app makes the notificaiton space really narrow...

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #29) > With the latest patch I noticed this in the console log: > > 1) E/GeckoConsole( 319): [JavaScript Error: "TypeError: timestamps[i] is > undefined" {file: "app://system.gaiamobile.org/js/notification_screen.js" > line: 381}] > Getting those on master too BTW.

Only one reproducible quirk remains with this new implementation: the nested-scroll-bounce effect described in Comment 24. There doesn't seem to be a good way to avoid that problem without discarding native scrolling for the notifications container and using simulated JS scrolling instead. Should we attempt to work around that issue in this patch, or should that be addressed in a followup?

(In reply to Marcus Cavanaugh [:mcav] (Mozilla SF) from comment #32) > Only one reproducible quirk remains with this new implementation: the > nested-scroll-bounce effect described in Comment 24. There doesn't seem to > be a good way to avoid that problem without discarding native scrolling for > the notifications container and using simulated JS scrolling instead. > > Should we attempt to work around that issue in this patch, or should that be > addressed in a followup? imho, this should be followup - even with this quirk, this version of the utility tray is superior to what we currently have.

(In reply to Chris Lord [:cwiiis] from comment #33) > (In reply to Marcus Cavanaugh [:mcav] (Mozilla SF) from comment #32) > > Only one reproducible quirk remains with this new implementation: the > > nested-scroll-bounce effect described in Comment 24. There doesn't seem to > > be a good way to avoid that problem without discarding native scrolling for > > the notifications container and using simulated JS scrolling instead. > > > > Should we attempt to work around that issue in this patch, or should that be > > addressed in a followup? > > imho, this should be followup - even with this quirk, this version of the > utility tray is superior to what we currently have. +1 :)

PR has been squashed and rebased in preparation for landing. Naoki, is there anything further you'd like to see here before landing? Just waiting on your qa-approval flag.

Comment on attachment 8639518 [details] Gaia PR <https://github.com/mozilla-b2g/gaia/pull/31084> Just the bug number for your follow up. :)

The nested-scroll-grab bug has been filed as bug 1209387. Huge thanks to all involved in helping prepare this to land. Here's hoping it sticks. master: https://github.com/mozilla-b2g/gaia/commit/28f3b92cc23f3b0a0d95c111f182485283a1f006