Closed Bug 1433019 Opened 6 years ago Closed 6 years ago

Dispatch scroll events right after resize events

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(1 file)

Bug 1433019 - Dispatch scroll events before dispatching animation events.
59 bytes, text/x-review-board-request
botond
: review+
mstange
: review+
Details 
From the spec;
https://html.spec.whatwg.org/multipage/webappapis.html#processing-model-8

 5. For each fully active Document in docs, run the resize steps for that Document, passing in now as the timestamp.

 6. For each fully active Document in docs, run the scroll steps for that Document, passing in now as the timestamp.

 7. For each fully active Document in docs, evaluate media queries and report changes for that Document, passing in now as the timestamp.

 8. For each fully active Document in docs, update animations and send events for that Document, passing in now as the timestamp.

 9. For each fully active Document in docs, run the fullscreen steps for that Document, passing in now as the timestamp.

 10. For each fully active Document in docs, run the animation frame callbacks for that Document, passing in now as the timestamp.

Though I don't quite understand why we don't do it after I read bug 1340684 comment 9 (I am sorry for my lack of understanding in English), as per the spec, we should do.

Let's see what happens if we dispatch at the position per the spec;

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b0cab01fca7137575666c54a7d3064f4b03da7c
There is a web platform test to check the scroll event is not dispatched right after a rAF callback;

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b0cab01fca7137575666c54a7d3064f4b03da7c&selectedJob=158373989
If I understand correctly bug 1340684 comment 9 (and a relevant one, bug 1250550 comment 3), it explains why we need to observe nsRefreshDriver apart from the flush style observers.  If it does, it's very similar to what we have to do for animation events (bug 1415780). :)

Anyway, at least for the spec compliance, I am convinced this is the right thing to do.
Comment on attachment 8945355 [details]
Bug 1433019 - Dispatch scroll events before dispatching animation events.

https://reviewboard.mozilla.org/r/215558/#review221838

I am happy to try dispatching scroll events earlier in the refresh driver tick (and in particular, earlier than rAF callbacks), as suggested by Markus in bug 1340684 comment 7.

However, it's important that scroll events are dispatched after FlushType::Style refresh observers (that is, after this loop [1] runs for iteration "i = 1" of the outer loop). This is because ScrollFrameHelper::AsyncScroll, which is used to drive main-thread scroll animations like wheel scroll, is a FlushType::Style observer [2], and it updates the scroll offset when run, and we want the scroll event to reflect that updated scroll offset so that web content can react to it (to e.g. implement scroll-linked effects) and the result can be rendered in the same frame as the scroll offset.

[1] https://searchfox.org/mozilla-central/rev/38bddf549db0f26d4d7575cf695958ff703a3ed7/layout/base/nsRefreshDriver.cpp#1860
[2] https://searchfox.org/mozilla-central/rev/38bddf549db0f26d4d7575cf695958ff703a3ed7/layout/generic/nsGfxScrollFrame.cpp#1783
Attachment #8945355 - Flags: review?(botond) → review-
I should add: if dispatching scroll events after FlushType::Style refresh observers poses a problem for spec compliance, we could try to resolve it by changing ScrollFrameHelper::AsyncScroll to not be a FlushType::Style refresh observer, but something else that runs before scroll events are dispatched.
(In reply to Botond Ballo [:botond] from comment #4)
> Comment on attachment 8945355 [details]
> Bug 1433019 - Dispatch scroll events right after dispatching resize events.
> 
> https://reviewboard.mozilla.org/r/215558/#review221838
> 
> I am happy to try dispatching scroll events earlier in the refresh driver
> tick (and in particular, earlier than rAF callbacks), as suggested by Markus
> in bug 1340684 comment 7.
> 
> However, it's important that scroll events are dispatched after
> FlushType::Style refresh observers (that is, after this loop [1] runs for
> iteration "i = 1" of the outer loop). This is because
> ScrollFrameHelper::AsyncScroll, which is used to drive main-thread scroll
> animations like wheel scroll, is a FlushType::Style observer [2], and it
> updates the scroll offset when run, and we want the scroll event to reflect
> that updated scroll offset so that web content can react to it (to e.g.
> implement scroll-linked effects) and the result can be rendered in the same
> frame as the scroll offset.

Ah I understand now. Thanks for the clarification.  The things are bit complicated.

Animations also observe FlushType::Style there, that means we process "update animations" at 8 in comment 0 in WillRefresh(), then as a result of the updating animations we process dispatching animation events later in the 'if (i == 1)' block.  So, we need to a machinery to process WillRefresh() and dispatching events in a set for both of scrolling and animations.  It sounds we need some amount of work.

That's said, that might not be a big problem, I am not sure though.  Anyway, in this bug I try to move DispatchScrollEvent() at the top of the 'if (i == 1)' block, this change should fix the web platform test as well.
test_scroll_event_ordering.html failed on the try with attachment 8945355 [details];
  
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7363a09acd16cd7791c9642dd80a32d67b015f24&selectedJob=158827498

I am not sure why the test did not fail on the first try in comment 0, I guess that's exactly because the issue Botond raised.

Anyway, the test is the very test case that checks scroll event is fired after requestAnimationFrame callbacks.  And the test was introduced in bug 785588.

Botond, what should we do for the test case?  Is that OK to modify the test case to check scroll event is fired prior to requestAnimationFrame callbacks?
Flags: needinfo?(botond)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> test_scroll_event_ordering.html failed on the try with attachment 8945355 [details]
> [details];
>   
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=7363a09acd16cd7791c9642dd80a32d67b015f24&selectedJob=1
> 58827498
> 
> I am not sure why the test did not fail on the first try in comment 0, I
> guess that's exactly because the issue Botond raised.

because *of* the issue.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> test_scroll_event_ordering.html failed on the try with attachment 8945355 [details]
> [details];
>   
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=7363a09acd16cd7791c9642dd80a32d67b015f24&selectedJob=1
> 58827498
> 
> I am not sure why the test did not fail on the first try in comment 0, I
> guess that's exactly because the issue Botond raised.
> 
> Anyway, the test is the very test case that checks scroll event is fired
> after requestAnimationFrame callbacks.  And the test was introduced in bug
> 785588.
> 
> Botond, what should we do for the test case?  Is that OK to modify the test
> case to check scroll event is fired prior to requestAnimationFrame callbacks?

Based on the bug, the problem was that the AsyncScroll update was happening before the rAF callback, and it was fixed by moving the AsyncScroll update to happen earlier (by changing it from a FlushType::Display observer to a FlushType::Style observer).

Accordingly, I think the important assertion in the test is this one in onFrame():

 ok(d.scrollTop > 0, "Must have scrolled by some amount (got " + d.scrollTop + ")");

which checks that the scroll offset update (from AsyncScroll) happened before the rAF callback.

The relative order between rAF and scroll event doesn't seem relevant to the test, so I think it's OK to change the test to expect them in the other order, as long as the above assertion is kept.
Flags: needinfo?(botond)
Comment on attachment 8945355 [details]
Bug 1433019 - Dispatch scroll events before dispatching animation events.

I am happy to try this, but we should be mindful of what Markus said in bug 1340684 comment 7:

> [...] On the other hand, if the
> requestAnimationFrame callback changes the scroll position, the page won't
> see a scroll event for that change until the next paint.
> I wonder what the right trade-off between these two orderings is, and how
> other browsers handle this.

We should be prepared for the possibility of the behaviour of some pages with scroll-linked effects regressing as a result of this change. If that happens, one possibility would be to try the mitigation Markus suggested:

> (Worst case, we could fire scroll events both
> before *and* after requestAnimationFrame callbacks. Not sure if that's a
> good idea...)
Attachment #8945355 - Flags: review?(botond) → review+
For reference, here are the previous changes to when scroll events fire that I'm aware of:

  - In bug 1209970, it went from a will-paint observer to a 
    FlushType::Style refresh observer

  - In bug 1250550, from a FlushType::Style refresh observer 
    to a FlushType::Layout refresh observer

  - In 1340684, from a FlushType::Layout refresh observer to 
    its own thing which runs in between FlushType::Style and 
    FlushType::Layout (and after rAF).

rAF callbacks also run in between FlushType::Style and FlushType::Layout, which means that between bug 1250550 (landed March 2016) and bug 1340684 (landed August 2017) - a period of 17 months - they were fired before rAF callbacks.

That gives me some confidence that moving them to be before rAF callbacks again should be fine.
Comment on attachment 8945355 [details]
Bug 1433019 - Dispatch scroll events before dispatching animation events.

https://reviewboard.mozilla.org/r/215558/#review222118

In light of botond's findings, and after briefly testing the patch locally, and in light of the fact that autoscroll now uses APZ instead of requestAnimationFrame, I think this is worth trying. If we discover that this is not web-compatible and we have to change the spec, it would be better to discover this sooner than later.
Attachment #8945355 - Flags: review?(mstange) → review+
Thank you!

(In reply to Botond Ballo [:botond] from comment #12)
> For reference, here are the previous changes to when scroll events fire that
> I'm aware of:
> 
>   - In bug 1209970, it went from a will-paint observer to a 
>     FlushType::Style refresh observer
> 
>   - In bug 1250550, from a FlushType::Style refresh observer 
>     to a FlushType::Layout refresh observer
> 
>   - In 1340684, from a FlushType::Layout refresh observer to 
>     its own thing which runs in between FlushType::Style and 
>     FlushType::Layout (and after rAF).
> 
> rAF callbacks also run in between FlushType::Style and FlushType::Layout,
> which means that between bug 1250550 (landed March 2016) and bug 1340684
> (landed August 2017) - a period of 17 months - they were fired before rAF
> callbacks.

This is great summary!  It's really easy to understand what's been happening there. :)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #14)
> Comment on attachment 8945355 [details]
> Bug 1433019 - Dispatch scroll events before dispatching animation events.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/215558/diff/2-3/

Hmm, why did MozReview sent the review request again?
Anyway, this will be a final try with the test modification.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e23aa058a3cd89f76f865b1da00fe5d52dc9370d
We had to drop is() check in the requestAnimationFrame callback in test_scroll_event_ordering.html, it was called after SimpleTest.finish() in the test verify run. :/

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0b9c213a61ba6d4c38f118dfd0384f8611ee4f7
Comment on attachment 8945355 [details]
Bug 1433019 - Dispatch scroll events before dispatching animation events.

https://reviewboard.mozilla.org/r/215558/#review222180

Sorry, I forgot to review the test before! Just one comment, it should fix your failure.

::: layout/base/tests/test_scroll_event_ordering.html:38
(Diff revision 4)
>  }
>  
>  function onScroll() {
> -  is(state, "didOnFrame", "Must have got requestAnimationFrame callback already");
> +  is(state, "initial", "Must be in initial state");
>    ok(d.scrollTop > 0, "Must have scrolled by some amount (got " + d.scrollTop + ")");
>    SimpleTest.finish();

The SimpleTest.finish() call should now be in onFrame() since that now happens after onScroll().
Attachment #8945355 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #20)
> Comment on attachment 8945355 [details]
> Bug 1433019 - Dispatch scroll events before dispatching animation events.
> 
> https://reviewboard.mozilla.org/r/215558/#review222180
> 
> Sorry, I forgot to review the test before! Just one comment, it should fix
> your failure.
> 
> ::: layout/base/tests/test_scroll_event_ordering.html:38
> (Diff revision 4)
> >  }
> >  
> >  function onScroll() {
> > -  is(state, "didOnFrame", "Must have got requestAnimationFrame callback already");
> > +  is(state, "initial", "Must be in initial state");
> >    ok(d.scrollTop > 0, "Must have scrolled by some amount (got " + d.scrollTop + ")");
> >    SimpleTest.finish();
> 
> The SimpleTest.finish() call should now be in onFrame() since that now
> happens after onScroll().

Ah indeed, we should. Thanks!
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 2 changes to 2 files
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: Error accessing https://treestatus.mozilla-releng.net/trees/autoland :
remote: HTTP Error 503: Service Unavailable
remote: Unable to check if the tree is open - treating as if CLOSED.
remote: To push regardless, include "CLOSED TREE" in your push comment.
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.a_treeclosure hook failed
abort: push failed on remote
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ced1881a5daa
Dispatch scroll events before dispatching animation events. r=botond,mstange
https://hg.mozilla.org/mozilla-central/rev/ced1881a5daa
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: