Closed Bug 1038178 Opened 10 years ago Closed 10 years ago

[Vertical Home screen] Use scrollbar from root scrollframe

Categories

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

x86
macOS
defect
Not set
normal

Tracking

(b2g-v2.1 fixed, b2g-v2.2 fixed)

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

People

(Reporter: kgrandon, Assigned: kgrandon)

References

Details

(Whiteboard: [systemsfe])

Attachments

(2 files)

We want to fire native mozbrowserasyncscroll events for the vertical homescreen, in order to do so we need to stop using an inner scrollframe, and use the root scrollframe. This bug tracks that work.
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking-]
Whiteboard: [systemsfe]
Attached file Github pull request (deleted) —
Depends on: 995519
Can I ask why you want mozbrowserasyncscroll events? We're hoping to phase those out entirely (bug 898075). They don't really solve the problem of getting low-latency notifications of async scrolling, because it still requires dispatching messages to the main thread which might be busy.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2) > Can I ask why you want mozbrowserasyncscroll events? We're hoping to phase > those out entirely (bug 898075). They don't really solve the problem of > getting low-latency notifications of async scrolling, because it still > requires dispatching messages to the main thread which might be busy. In 2.0 we want to animate the rocketbar layout (UI lives in the system app), depending on the scroll state of the page. This is similar to how the current FxOS browser works. I suppose we just need some way of driving an animation, cross-process, based on scroll position. Looking over that bug, it appears that we thought we would no longer use this, but I think we had plans to be using it just as much (if not more).
Can you use the scroll event instead of the mozbrowserasyncscroll event? They should have approximately the same latency.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4) > Can you use the scroll event instead of the mozbrowserasyncscroll event? > They should have approximately the same latency. I'm not sure of the history, but I'm not sure why we have two different events? It should be possible to use, but I haven't done any experimentation with it yet.
In the distant past we fired scroll much less frequently and mozbrowserasyncscroll events much more frequently. Now they fire at pretty much the same rate.
No longer blocks: vertical-home-next
Blocks: 1053747
Depends on: 1056423
Summary: [Vertical Homescreen] Use scrollbar from root scrollframe → [Vertical Home screen] Use scrollbar from root scrollframe
Comment on attachment 8455345 [details] Github pull request Chris - would you mind giving this a quick review? Thanks!
Attachment #8455345 - Flags: review?(chrislord.net)
Comment on attachment 8455345 [details] Github pull request Code inspection looks good (some comments on the pull request) - I've not tested that this works, I'm slightly surprised/pleased that there are no required changes to GaiaGrid. I'm assuming that it does work fine, if stuff breaks we need more tests :)
Attachment #8455345 - Flags: review?(chrislord.net) → review+
Out of interest, why do we want to use mozbrowserasyncscroll events here? I'm for this change anyway as it simplifies the DOM, I'm just curious.
the scrollable element was added to show the grey scrollbar while users are scrolling on the right. Is it working with the body right now? I hope so, it is clearer without scrollable element
(In reply to Cristian Rodriguez (:crdlc) from comment #10) > the scrollable element was added to show the grey scrollbar while users are > scrolling on the right. Is it working with the body right now? I hope so, it > is clearer without scrollable element yes, the scrollbar draws for the body now (this was fixed in bug 995519).
Kevin, be careful if the bug 1062820 is landed before this one because I added there a new integration test. Although I would prefer to wait here for bug 1062820 because it should be uplifted to 2.1 and the scrollable element lives in that branch and the cherry pick failed done directly because of this new integration test. If you cannot wait I will modify the integration test for 2.1 if this bug in landed before mine. Thanks a lot
Please keep in mind that if we aren't able to fix bug 1043859 then there is a possibility that we might disable the root frame scrollbars again. Unlikely but something you should keep in mind.
(In reply to Cristian Rodriguez (:crdlc) from comment #12) > Kevin, be careful if the bug 1062820 is landed before this one because I > added there a new integration test. Although I would prefer to wait here for > bug 1062820 because it should be uplifted to 2.1 and the scrollable element > lives in that branch and the cherry pick failed done directly because of > this new integration test. If you cannot wait I will modify the integration > test for 2.1 if this bug in landed before mine. Thanks a lot Sure, I will give you right of way and will wait for your bug to land first, then update the test. (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #13) > Please keep in mind that if we aren't able to fix bug 1043859 then there is > a possibility that we might disable the root frame scrollbars again. > Unlikely but something you should keep in mind. This is needed to get the right rocketbar animations on the home screen, but I think it's a little late for us to get that into 2.1. I think we should land this, and maybe not request uplift to 2.1 until we see what happens in bug 1043859. I hope we can keep root frame scrollbars in 2.2 if we lose them in 2.1.
Kevin, the bug with the new integration test has been landed so I guess that you have to touch this line theoretically https://github.com/mozilla-b2g/gaia/commit/24fb519181335ff2fcf3a32cf1b775685f9089cf#diff-f0f7f9beab9ed2c6a86ea73d3c90779bR36
Thanks Cristian! The change seems simple, I've updated the pull request and will wait to see what travis says.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
The patch made icons uncontrollable in edit mode. It's normal in the previous commit 8746bc86466677c3e1ed019c58c288577017362e .
Flags: needinfo?(kgrandon)
(In reply to Yi-Fan Liao [:yifan][:yliao] from comment #18) > The patch made icons uncontrollable in edit mode. It's normal in the > previous commit 8746bc86466677c3e1ed019c58c288577017362e . Thanks, I'm looking into it now.
Here is a simple follow-up which uses the document scroll frame for calculations. r=me.
Attachment #8488423 - Flags: review+
Flags: needinfo?(kgrandon)
(In reply to Kevin Grandon :kgrandon from comment #20) > Created attachment 8488423 [details] > Follow-up - Use document scroll frame for drag drop > > Here is a simple follow-up which uses the document scroll frame for > calculations. r=me. Sorry I missed this, it was something I meant to check...
Blocks: 1033468
Target Milestone: --- → 2.1 S4 (12sep)
Comment on attachment 8455345 [details] Github pull request [Approval Request Comment] [Bug caused by] (feature/regressing bug #): Bug 1064918 [User impact] if declined: Janky experience when rearranging icons on the homescreen [Testing completed]: Been on master a long time [Risk to taking this patch] (and alternatives if risky): If bug 995519 is disabled, scrollbars will not appear on the home screen [String changes made]: None
Attachment #8455345 - Flags: approval-gaia-v2.1?
Comment on attachment 8488423 [details] Follow-up - Use document scroll frame for drag drop [Approval Request Comment] See comment #23
Attachment #8488423 - Flags: approval-gaia-v2.1?
Blocks: 1064918
Attachment #8455345 - Flags: approval-gaia-v2.1? → approval-gaia-v2.1+
Attachment #8488423 - 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.

Attachment

General

Created:
Updated:
Size: