Bug 45631 – Scroll event should be fired asynchronously

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Mike Lawther]

Created attachment 67356 [details]
test for behaviour change 2)

Patch http://trac.webkit.org/changeset/67001/ introduced smooth scrolling on Windows, which changes two behaviours:
 1) the scroll event triggered by scrollByLines() is no longer fired synchronously; 
 2) the value of scrollTop is not synchronously set to the target value

It's 1) that causes fast/events/scroll-event-does-not-bubble.html to break on Windows.

Attached are two tests for these changed behaviours.

[Mike Lawther]

Created attachment 67357 [details]
test for behaviour change 1)

[Ojan Vafai]

It's not clear to me whether it's OK to make this method async, but we should at least be consistent across platforms.

Looks like Firefox is the only other browser to implement scrollByLines and it only implements it on Window (where WebKit only implements it on Element). So, if there are regressions from making this async, they'd be in WebKit-only code paths.

Alternately, maybe when scrollByLines is called from JS, it shouldn't smooth scroll? It seems we don't smooth scroll if you set scrollTop.

[Peter Kasting]

We should make this method scroll synchronously and without smooth scrolling, like it used to.

It should be easy enough to do; the underlying implementation needs to call something like setValue() instead of scroll().  I can look into this in a week when I'm back from vacation, if no one else can get it sooner.

[Peter Kasting]

Possibly separately, Darin Fisher tells me that scroll DOM events should be async per the HTML5 spec.  I'm not sure how that interacts with this.  Maybe internally we need to scroll synchronously when asked but use queues for events we fire back to the page.  Darin, feel free to enlighten me.

[Darin Fisher (:fishd, Google)]

(In reply to comment #0)
> Created an attachment (id=67356) [details]
> test for behaviour change 2)
> 
> Patch http://trac.webkit.org/changeset/67001/ introduced smooth scrolling on Windows, which changes two behaviours:
>  1) the scroll event triggered by scrollByLines() is no longer fired synchronously; 
>  2) the value of scrollTop is not synchronously set to the target value

3) a single scroll action may result in multiple scroll events

i wonder if this third artifact isn't the cause of bugs like:
http://code.google.com/p/chromium/issues/detail?id=55218

The HTML5 spec does not document the behavior of the 'scroll' event.  I recall a whatwg (or public-webapps) thread where it was asserted that the 'scroll' event should be dispatch asynchronously.

[Peter Kasting]

It seems to me that we shouldn't be sending scroll events as a side effect of doing the final low-level scrolls on the ScrollbarClient.  Instead we should be sending them in the course of generating the scroll request in the first place.

This way, I'd get a single "scroll" event for a single wheel click, regardless of how the ScrollAnimator breaks it apart.

Or am I misguided?  I don't have a good sense here.  At least the event isn't cancelable!

[Peter Kasting]

Testing other browsers makes it clear that smooth scrolls generally dispatch several scroll events per high-level scroll, and do so asynchronously.

Right now, everything should be synchronous since Darin (Fisher) turned off smooth scrolling at compile time.  So the test is actually passing again.  But we should change the event to be async and then also change the test.

Mihai is going to work on this stuff, I think.  I'll check in a change that unmarks this test as "failing".

[Mihai Parparita]

I've put up some test cases at http://persistent.info/webkit/test-cases/onscroll-async/

document.html (scrolling of the whole document)
  Chrome: sync
  WebKit: sync
  Firefox: async
  Opera: async
  IE: sync

div.html (scrolling of a div with overflow: auto)
  Chrome: sync
  WebKit: sync
  Firefox: async
  Opera: async
  IE: async
  
history.html (scrolling induced by navigating to fragments and going back)
  Chrome: sync for fragment change, async for back*
  WebKit: sync for fragment change, async for back*
  Firefox: async
  Opera: async
  IE: sync for fragment change, doesn't restore scroll for back

* going back is async because history.back() is async, presumably the scroll event itself is still dispatched synchronously, like in all other cases

Chrome is 9.0.572.0/Mac
WebKit is r71367/Mac
Firefix is 4.0b6/Mac
Opera is 10.62/Mac
IE is IE8/Win7

The spec for this is in the CSSOM (http://dev.w3.org/csswg/cssom-view/), and says "queue a task to fire a simple event named scroll that bubbles at the Document object, unless a task to fire that event at the Document object was already queued." (and the same thing for elements). That makes it pretty clear that scroll should be async (though it doesn't clarify the smooth scrolling issues that Peter brought up).

[Ojan Vafai]

I certainly support making this event async if we can without breaking existing content. Is there any history to this in WebKit code? Has it always been sync?

[Mihai Parparita]

(In reply to comment #9)
> I certainly support making this event async if we can without breaking existing content. Is there any history to this in WebKit code? Has it always been sync?

I went back ~5 years, and it's always been sync:
http://trac.webkit.org/browser/trunk/WebCore/kwq/KWQKHTMLPart.mm?rev=11945#L1757

That ends up in:
http://trac.webkit.org/browser/trunk/WebCore/khtml/xml/dom_nodeimpl.cpp?rev=11945#L549

Given that it's already async in Firefox, I'm not as concerned about compatibility issues.

[Mihai Parparita]

Created attachment 73328 [details]
Patch

[Mihai Parparita]

Darin, Dimitri said you might have opinions about this approach (the EventQueue class, and attaching it to Node via NodeRareData).

Ojan, is any of this helpful for your plan to make editing events async?

[Mihai Parparita]

Created attachment 73927 [details]
Patch

[Mihai Parparita]

OK, this is now ready for review (only difference from the patch from last week is that I added the EventQueue.cpp/h files to the various port build systems).

[Darin Adler]

Comment on attachment 73927 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=73927&action=review

I don’t think there’s a need to have a separate queue for each node. A single queue per document should suffice.

> WebCore/dom/NodeRareData.h:120
> +            m_eventQueue.set(new EventQueue(target));

This creates a circular reference and hence leaks any node that ever has an event queue. The node owns the rare data, the rare data owns the event queue, and the event queue refs the node.

[Mihai Parparita]

(In reply to comment #15)
> (From update of attachment 73927 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73927&action=review
> 
> I don’t think there’s a need to have a separate queue for each node. A single queue per document should suffice.

OK. Since multiple scroll position changes are supposed to be coalesced into one event, I wasn't sure if it'd be inefficient to have to do a linear scan through the queued events for a document, but I'm guessing the queue will be empty the vast majority of the time.

Do you have a preference about keeping the EventQueue class, or moving things back into the Document class?

> > WebCore/dom/NodeRareData.h:120
> > +            m_eventQueue.set(new EventQueue(target));
> 
> This creates a circular reference and hence leaks any node that ever has an event queue. The node owns the rare data, the rare data owns the event queue, and the event queue refs the node.

Oops, thanks for pointing this out.

[Darin Adler]

(In reply to comment #16)
> Since multiple scroll position changes are supposed to be coalesced into one event, I wasn't sure if it'd be inefficient to have to do a linear scan through the queued events for a document, but I'm guessing the queue will be empty the vast majority of the time.

We should consider including an efficient mechanism for finding already-outstanding events that are supposed to be coalesced. For example, in addition to the vector, we could keep a map of events in the vector using the target and type of the event as a key. This would guarantee we could efficiently find an existing event to coalesce with even if something pathological is going on.

> Do you have a preference about keeping the EventQueue class, or moving things back into the Document class?

Having this in a class seems OK, good factoring. The Document class is big it’s good not to add things to it.

The real issue is the functions in the interface, not whether those functions are grouped in a class or not. We need that set of functions to be tight and clear.

[Mihai Parparita]

Created attachment 73934 [details]
Patch

[Mihai Parparita]

(In reply to comment #17)
> (In reply to comment #16)
> > Since multiple scroll position changes are supposed to be coalesced into one event, I wasn't sure if it'd be inefficient to have to do a linear scan through the queued events for a document, but I'm guessing the queue will be empty the vast majority of the time.
> 
> We should consider including an efficient mechanism for finding already-outstanding events that are supposed to be coalesced. For example, in addition to the vector, we could keep a map of events in the vector using the target and type of the event as a key. This would guarantee we could efficiently find an existing event to coalesce with even if something pathological is going on.

Now that I think about it, how necessary do you think this is? The overhead of the map seems like it would hurt more in the common case with an empty queue. To generate O(N^2) behavior the attacker would have to create O(N) unique nodes and generate scroll events for each one, I think the overhead from having that many nodes (which we can't avoid) would outweigh the O(N^2) behavior from enqueueEventIfNotAlreadyEnqueued.

> The real issue is the functions in the interface, not whether those functions are grouped in a class or not. We need that set of functions to be tight and clear.

For now I've kept Node::enqueueEvent(IfNotAlreadyEnqueued) which are convenience versions that call those functions in Document with the node as the target. Exposing the EventQueue in Document didn't seem like a good idea.

[Darin Adler]

Comment on attachment 73934 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=73934&action=review

> WebCore/dom/Document.h:1021
> +    void enqueueEvent(PassRefPtr<Event> event, PassRefPtr<Node> eventTarget);

I suggest having the caller call setTarget on the event before calling enqueueEvent. Then there would be no need to pass the target separately.

I don’t think it’s a good idea to have functions with two different purposes on the same object. Having enqueueEvent in Node isn’t really all that helpful, and creates ambiguity.

The argument name “event” should not be written here since the type already makes it clear.

> WebCore/dom/Document.h:1023
> +    void enqueueEventIfNotAlreadyEnqueued(PassRefPtr<Event> event, PassRefPtr<Node> eventTarget);

This long awkward name is not even accurate. This enqueues an event if another of the same type targeted at the same target is not already enqueued. That’s not the same as *this* event already being enqueued.

I don’t think that creating an event and then later discovering that it is already enqueued is a suitable API. Instead, I suggest that the function pass only the type of the event and the target and actual event creation be done inside the function if it’s needed. The only tricky part is making sure we pass the right values for canBubble and cancelable, but I think we can start with those hardcoded at first, and add arguments if needed later.

And we can come up with a more accurate name. Maybe enqueueCoalescedEvent(const String& type, PassRefPtr<EventTarget>).

> WebCore/dom/EventQueue.cpp:36
> +struct EnqueuedEvent : Noncopyable {

Why Noncopyable?

> WebCore/dom/EventQueue.cpp:38
> +    RefPtr<Event> m_event;
> +    RefPtr<Node> m_eventTarget;

Normally we do not use the "m_" prefix for the members of a struct.

> WebCore/dom/EventQueue.cpp:55
> +void EventQueue::enqueueEvent(PassRefPtr<Event> event, PassRefPtr<Node> eventTarget)

The targets should be stored in the events, not separately. We don’t need the EnqueuedEvent struct at all.

> WebCore/dom/EventQueue.cpp:96
> +    Node* eventTarget = event->m_eventTarget.get();
> +    if (!eventTarget->inDocument())
> +        return;

What about if the entire document is no longer being displayed, say if you navigated away from it or destroyed it. We need to test that case.

> WebCore/page/EventHandler.cpp:2760
> -        m_frame->document()->dispatchEvent(Event::create(eventNames().scrollEvent, true, false));
> +        m_frame->document()->enqueueEventIfNotAlreadyEnqueued(Event::create(eventNames().scrollEvent, true, false));

I noticed that the multiple places emitting scrollEvent don’t agree on the setting of canBubble. This call site passes true.

> WebCore/rendering/RenderLayer.cpp:1401
> -    if (view) {
> -        if (FrameView* frameView = view->frameView())
> -            frameView->scheduleEvent(Event::create(eventNames().scrollEvent, false, false), renderer()->node());
> -    }
> +    if (view && view->frameView())
> +        renderer()->node()->enqueueEventIfNotAlreadyEnqueued(Event::create(eventNames().scrollEvent, false, false));

This call site passes false for canBubble.

It does not make sense to check view and view->frameView() for 0 any more here. That was only done so we could call the scheduleEvent function.

The pauseScheduledEvents and resumeScheduledEvents function will no longer affect the dispatching of this event. Why is that OK?

> WebCore/rendering/RenderListBox.cpp:543
> -        node()->dispatchEvent(Event::create(eventNames().scrollEvent, false, false));
> +        node()->enqueueEventIfNotAlreadyEnqueued(Event::create(eventNames().scrollEvent, false, false));

This call site passes false for canBubble.

[Darin Adler]

(In reply to comment #19)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > Since multiple scroll position changes are supposed to be coalesced into one event, I wasn't sure if it'd be inefficient to have to do a linear scan through the queued events for a document, but I'm guessing the queue will be empty the vast majority of the time.
> > 
> > We should consider including an efficient mechanism for finding already-outstanding events that are supposed to be coalesced. For example, in addition to the vector, we could keep a map of events in the vector using the target and type of the event as a key. This would guarantee we could efficiently find an existing event to coalesce with even if something pathological is going on.
> 
> Now that I think about it, how necessary do you think this is? The overhead of the map seems like it would hurt more in the common case with an empty queue. To generate O(N^2) behavior the attacker would have to create O(N) unique nodes and generate scroll events for each one, I think the overhead from having that many nodes (which we can't avoid) would outweigh the O(N^2) behavior from enqueueEventIfNotAlreadyEnqueued.

You can just build a test case and see. No need to guess.

> Exposing the EventQueue in Document didn't seem like a good idea.

I think it probably is a good idea. But for now with only two functions, we might be OK.

[Mihai Parparita]

I'll upload a new patch as soon as we can decide on the questions below.

(In reply to comment #20)
> I suggest having the caller call setTarget on the event before calling enqueueEvent. Then there would be no need to pass the target separately.

Unfortunately Event::target() returns the target as an EventTarget pointer, and I need it as a Node to have access to the isDocumentNode() and document()->dispatchWindowEvent() methods. This (having a struct with the event and the target as a Node) is the same pattern as used by ScheduledEvent in FrameView.cpp.

> > +    void enqueueEventIfNotAlreadyEnqueued(PassRefPtr<Event> event, PassRefPtr<Node> eventTarget);
> 
> This long awkward name is not even accurate. This enqueues an event if another of the same type targeted at the same target is not already enqueued. That’s not the same as *this* event already being enqueued.
> 
> I don’t think that creating an event and then later discovering that it is already enqueued is a suitable API. Instead, I suggest that the function pass only the type of the event and the target and actual event creation be done inside the function if it’s needed. The only tricky part is making sure we pass the right values for canBubble and cancelable, but I think we can start with those hardcoded at first, and add arguments if needed later.
> 
> And we can come up with a more accurate name. Maybe enqueueCoalescedEvent(const String& type, PassRefPtr<EventTarget>).

Unfortunately, it looks like the scroll event only bubbles if the target is the document, so we can't hardcoded it one way or the other that easily. Here are the bits from the CSSOM spec that talk about firing of scroll events:

- If the aligning caused content to move queue a task to fire a simple event named scroll that bubbles at the Document object, unless a task to fire that event at the Document object was already queued.
- Queue a task to fire a simple event named scroll at the element associated with the scrolling box, unless a task to fire that event at that element was already queued.
- Queue a task to fire a simple event named scroll that bubbles at the Document object associated with the viewport, unless a task to fire that event at that Document object was already queued.
- If the aligning caused content to move queue a task to fire a simple event named scroll at the element, unless a task to fire that event at that element was already queued.

So it'd have to be enqueueCoalescedEvent(const String& type, PassRefPtr<EventTarget>, bool bubbles).

Alternatively, rather that having this much flexibility in the API at this early stage, we could have something more specialized:

1. EventQueue:enqueueScrollEvent(PassRefPtr<Node> target, bool canBubble): does the check for other scroll events with that target being in enqueued
1. Node::enqueueScrollEvent(): calls document->eventQueue()->enqueueScrollEvent(this, false)
2. Document::enqueueScrollEvent(): calls m_eventQueue->enqueueScrollEvent(this, true)
 
> > WebCore/dom/EventQueue.cpp:36
> > +struct EnqueuedEvent : Noncopyable {
> 
> Why Noncopyable?

It seemed like the default was to make things Noncopyable, unless there was a reason why things should be copied. I can remove it if you'd like.

> > WebCore/dom/EventQueue.cpp:96
> > +    Node* eventTarget = event->m_eventTarget.get();
> > +    if (!eventTarget->inDocument())
> > +        return;
> 
> What about if the entire document is no longer being displayed, say if you navigated away from it or destroyed it. We need to test that case.

How do I check for these things?
 
> The pauseScheduledEvents and resumeScheduledEvents function will no longer affect the dispatching of this event. Why is that OK?

They would just delay scroll event dispatching until layout was complete, but still run them sychronously. Now that they're async, they're guaranteed to run after layout is done.
 
> > Exposing the EventQueue in Document didn't seem like a good idea.
> 
> I think it probably is a good idea. But for now with only two functions, we might be OK.

I'll proably expose it if we go with the enqueueScrollEvent approach described above.

[Darin Adler]

(In reply to comment #22)
> Unfortunately Event::target() returns the target as an EventTarget pointer, and I need it as a Node

You can assert that it’s a node, and cast using static_cast.

[Mihai Parparita]

Created attachment 73948 [details]
Patch

[Mihai Parparita]

(In reply to comment #23)
> (In reply to comment #22)
> > Unfortunately Event::target() returns the target as an EventTarget pointer, and I need it as a Node
> 
> You can assert that it’s a node, and cast using static_cast.

Actually, there's an EventTarget::toNode that does exactly what I want.

The latest version of the patch uses the target in the event, and adds an enqueueScrolleEvent method that does the right thing for Document vs. Node (as far as bubbling) and implements the coalescing logic with a HashSet. If we need similar coalescing behavior later (e.g. for bug 36334 or bug 36202), it can be generalized, but I saw no need for that now.

[Darin Adler]

Comment on attachment 73948 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=73948&action=review

Better, but still needs some work.

> WebCore/dom/Document.cpp:403
> +    , m_eventQueue(new EventQueue)

This should have an adoptPtr.

> WebCore/dom/Document.cpp:3443
> +    event->setTarget(this);

It’s unclear that this function can only queue events that are intended for the document. Is there some way to make this more clear? I worry that if you meant to write document()->eventQueue()->enqueueEvent() but instead just wrote document()->enqueueEvent() you’d end up incorrectly changing the event target to the document.

> WebCore/dom/Document.cpp:3449
> +    m_eventQueue->enqueueScrollEvent(this, true);

In WebKit code we frown on boolean arguments that get boolean constants passed to them. Either you can define the function so it doesn’t need the argument or you can use an enum instead of true/false. There’s no way for someone reading “true” here to know what the “true” means.

> WebCore/dom/EventQueue.cpp:33
> +#include "Node.h"

No need to include Node.h if we are also including Document.h.

> WebCore/dom/EventQueue.cpp:46
> +    m_enqueuedEvents.clear();

No reason to do this.

> WebCore/dom/EventQueue.cpp:63
> +    if (m_nodesWithEnqueuedScrollEvents.contains(target.get()))
> +        return;
> +
> +    m_nodesWithEnqueuedScrollEvents.add(target.get());

This does two hash lookups, and you only need to do one. Like this:

    if (!m_nodesWithEnqueuedScrollEvents.add(target.get()).second)
        return;

That does the same thing with just one hash lookup.

> WebCore/dom/EventQueue.cpp:66
> +    enqueueEvent(scrollEvent);

Should be scrollEvent.release().

> WebCore/dom/EventQueue.cpp:77
> +    EventIterator end = enqueuedEvents.end();
> +    for (EventIterator it = enqueuedEvents.begin(); it != end; ++it)
> +        dispatchEvent(*it);

There’s no need to use iterators to walk through a vector. A for look using size_t and indices works just fine and reads more clearly. Iterators are fine if it’s an abstract algorithm, but no need for an iterator just for a vector.

Instead of *it this would be better if it was it->release() or if you fix the iteration enqueuedEvents[i].release().

> WebCore/dom/EventQueue.cpp:85
> +    if (eventNames().scrollEvent == event->type())
> +        m_nodesWithEnqueuedScrollEvents.remove(eventTarget);

No need to check the type. We could just always call this remove function.

> WebCore/dom/EventQueue.cpp:91
> +    if (eventTarget->isDocumentNode())
> +        eventTarget->document()->dispatchWindowEvent(event);

Why? Are all event targeted at the document window events? Can’t a document ever get a non-window event?

> WebCore/dom/EventQueue.h:39
> +struct EnqueuedEvent;

This doesn’t belong here.

> WebCore/dom/EventQueue.h:47
> +    void enqueueScrollEvent(PassRefPtr<Node> target, bool canBubble);

Your patch always passes false for canBubble, so you should remove the argument from this function. It’s always false. You also need a test case to show that it’s correct to have the scroll event not bubble in the one case you’re changing, the event queued in EventHandler::sendScrollEvent.

> WebCore/dom/EventQueue.h:54
> +    Vector<RefPtr<Event> > m_enqueuedEvents;

This should be named m_queuedEvents, not m_enqueuedEvents.

> WebCore/dom/EventQueue.h:55
> +    HashSet<Node*> m_nodesWithEnqueuedScrollEvents;

This should be named m_nodesWithQueuedScrollEvents, not m_nodesWithEnqueuedScrollEvents.

> WebCore/dom/Node.h:520
> +    void enqueueScrollEvent();

Do we really need this helper? Can’t the client callers with the event queue directly? I’d prefer not to touch the Node class at all; it’s not good to build the scroll event in like this.

[Mihai Parparita]

Created attachment 73952 [details]
Patch

[Mihai Parparita]

Just one question below.

(In reply to comment #26)
> > WebCore/dom/Document.cpp:403
> > +    , m_eventQueue(new EventQueue)
> 
> This should have an adoptPtr.

Fixed.

> > WebCore/dom/Document.cpp:3443
> > +    event->setTarget(this);
> 
> It’s unclear that this function can only queue events that are intended for the document. Is there some way to make this more clear? I worry that if you meant to write document()->eventQueue()->enqueueEvent() but instead just wrote document()->enqueueEvent() you’d end up incorrectly changing the event target to the document.

That was the case before this patch (since pendingEventTimerFired called dispatchWindowEvent). Should I rename it to enqueueWindowEvent?

> > WebCore/dom/Document.cpp:3449
> > +    m_eventQueue->enqueueScrollEvent(this, true);
> 
> In WebKit code we frown on boolean arguments that get boolean constants passed to them. Either you can define the function so it doesn’t need the argument or you can use an enum instead of true/false. There’s no way for someone reading “true” here to know what the “true” means.

Removed the boolean parameter altogether.

> > WebCore/dom/EventQueue.cpp:33
> > +#include "Node.h"
> 
> No need to include Node.h if we are also including Document.h.

Removed.

> > WebCore/dom/EventQueue.cpp:46
> > +    m_enqueuedEvents.clear();
> 
> No reason to do this.

Removed.

> > WebCore/dom/EventQueue.cpp:63
> > +    if (m_nodesWithEnqueuedScrollEvents.contains(target.get()))
> > +        return;
> > +
> > +    m_nodesWithEnqueuedScrollEvents.add(target.get());
> 
> This does two hash lookups, and you only need to do one. Like this:
> 
>     if (!m_nodesWithEnqueuedScrollEvents.add(target.get()).second)
>         return;
> 
> That does the same thing with just one hash lookup.

Changed.

> > WebCore/dom/EventQueue.cpp:66
> > +    enqueueEvent(scrollEvent);
> 
> Should be scrollEvent.release().

Changed.

> > WebCore/dom/EventQueue.cpp:77
> > +    EventIterator end = enqueuedEvents.end();
> > +    for (EventIterator it = enqueuedEvents.begin(); it != end; ++it)
> > +        dispatchEvent(*it);
> 
> There’s no need to use iterators to walk through a vector. A for look using size_t and indices works just fine and reads more clearly. Iterators are fine if it’s an abstract algorithm, but no need for an iterator just for a vector.
> 
> Instead of *it this would be better if it was it->release() or if you fix the iteration enqueuedEvents[i].release().

Switched to using indices for the loop.

> > WebCore/dom/EventQueue.cpp:85
> > +    if (eventNames().scrollEvent == event->type())
> > +        m_nodesWithEnqueuedScrollEvents.remove(eventTarget);
> 
> No need to check the type. We could just always call this remove function.

I switched to a m_nodesWithEnqueuedScrollEvents.clear() call in pendingEventTimerFired.

> > WebCore/dom/EventQueue.cpp:91
> > +    if (eventTarget->isDocumentNode())
> > +        eventTarget->document()->dispatchWindowEvent(event);
> 
> Why? Are all event targeted at the document window events? Can’t a document ever get a non-window event?

Document::pendingEventTimerFired used to call dispatchWindowEvent, I was just replicating that logic.

> > WebCore/dom/EventQueue.h:39
> > +struct EnqueuedEvent;
> 
> This doesn’t belong here.

Leftover from past versions of the patch, removed.

> > WebCore/dom/EventQueue.h:47
> > +    void enqueueScrollEvent(PassRefPtr<Node> target, bool canBubble);
> 
> Your patch always passes false for canBubble, so you should remove the argument from this function. It’s always false. You also need a test case to show that it’s correct to have the scroll event not bubble in the one case you’re changing, the event queued in EventHandler::sendScrollEvent.

I added fast/events/scroll-event-phase.html, which checks that it doesn't matter which type of event handler is used (which seems like the only thing that could conceivably be affected by this change).
 
> > WebCore/dom/EventQueue.h:54
> > +    Vector<RefPtr<Event> > m_enqueuedEvents;
> 
> This should be named m_queuedEvents, not m_enqueuedEvents.

Renamed.

> > WebCore/dom/EventQueue.h:55
> > +    HashSet<Node*> m_nodesWithEnqueuedScrollEvents;
> 
> This should be named m_nodesWithQueuedScrollEvents, not m_nodesWithEnqueuedScrollEvents.

Renamed.

> > WebCore/dom/Node.h:520
> > +    void enqueueScrollEvent();
> 
> Do we really need this helper? Can’t the client callers with the event queue directly? I’d prefer not to touch the Node class at all; it’s not good to build the scroll event in like this.

Removed now that we no longer need the bool param.

[Darin Adler]

(In reply to comment #28)
> That was the case before this patch (since pendingEventTimerFired called dispatchWindowEvent). Should I rename it to enqueueWindowEvent?

Sure, I think that’s a clearer name.

> > > WebCore/dom/EventQueue.cpp:91
> > > +    if (eventTarget->isDocumentNode())
> > > +        eventTarget->document()->dispatchWindowEvent(event);
> > 
> > Why? Are all event targeted at the document window events? Can’t a document ever get a non-window event?
> 
> Document::pendingEventTimerFired used to call dispatchWindowEvent, I was just replicating that logic.

Sure, but I’m concerned about the factoring.

If we can enqueue an event for an arbitrary node, that includes document nodes. Having the underlying machinery assume that any event targeted at the document is a window event is not necessarily correct.

I think the function to enqueue a window event should have a different name; if we are going to recognize window events by the fact that their target is the document, then the non-window event queuing needs to be illegal for documents. It needs both documentation and and assertion that the target node is not a document.

Later we may find that some events can be queued for the document that are not window events. If that happens we’ll need to figure out some way to accommodate both types in the queue, maybe by storing something in the event or maybe by making the queue store a boolean along with the event in the vector. In the mean time, we need a design and perhaps assertions to catch this in case it happens by mistake.

We don’t want a generic event that can go to any node such as a mutation event to turn into a window event because of a programming mistake.

> I added fast/events/scroll-event-phase.html, which checks that it doesn't matter which type of event handler is used (which seems like the only thing that could conceivably be affected by this change).

Good point. With no ancestors, then it’s not all that visible if canBubble was true or false, since it’s the AT_TARGET phase.

But we can also just check the bubbles property of the event inside the handler function. That will show us whether other browsers agree about whether this should have bubbles true. The HTML5 specification or some other specification may also explicitly state how this should work.

[Mihai Parparita]

(In reply to comment #29)
> But we can also just check the bubbles property of the event inside the handler function. That will show us whether other browsers agree about whether this should have bubbles true. The HTML5 specification or some other specification may also explicitly state how this should work.

Actually, it's the CSSOM Views spec that covers this (and says that document scroll events should bubble and element ones shouldn't, see comment 22). I added logging of the bubbles property to the test cases at:

http://persistent.info/webkit/test-cases/onscroll-async/document.html
http://persistent.info/webkit/test-cases/onscroll-async/div.html

Gecko and ToT WebKit and Chrome follow the spec (first one bubbles, second one doesn't). Opera doesn't (neither bubbles). IE 8 doesn't implement the W3C event system, so it doesn't have this property, but in IE 9 neither bubbles.

For the sake of correctness, I'm going to change the patch to follow the spec.

[Mihai Parparita]

Created attachment 74018 [details]
Patch

[Mihai Parparita]

(In reply to comment #29)
> (In reply to comment #28)
> > That was the case before this patch (since pendingEventTimerFired called dispatchWindowEvent). Should I rename it to enqueueWindowEvent?
> 
> Sure, I think that’s a clearer name.

Renamed.
 
> > Document::pendingEventTimerFired used to call dispatchWindowEvent, I was just replicating that logic.
> 
> Sure, but I’m concerned about the factoring.
> 
> If we can enqueue an event for an arbitrary node, that includes document nodes. Having the underlying machinery assume that any event targeted at the document is a window event is not necessarily correct.
<snip>

I changed Document::enqueueWindowEvent to just set the target to domWindow(), since that is its intention. EventQueue::dispatchEvent then checks for this and uses the overloaded DOMWindow::dispatchEvent method (the one that takes a target parameter) that has special behavior.

> But we can also just check the bubbles property of the event inside the handler function. That will show us whether other browsers agree about whether this should have bubbles true. The HTML5 specification or some other specification may also explicitly state how this should work.

As mentioned above, I added the bubble behavior back, to be compliant with the CSSOM spec. There's now a ScrollEventTargetType enum parameter to EventQueue, which should hopefully be cleaner than a bool parameter.

[WebKit Review Bot]

Attachment 74018 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6031102

[Mihai Parparita]

Created attachment 74027 [details]
Patch

Patch should build on Chromium now

[Ryosuke Niwa]

Comment on attachment 74027 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74027&action=review

I'm asking Mihai to separate EventQueue implementation so that I can use it for selectionchange event and async DOM mutation events.

> WebCore/dom/EventQueue.cpp:61
> +void EventQueue::enqueueScrollEvent(PassRefPtr<Node> target, ScrollEventTargetType targetType)
> +{
> +    if (!m_nodesWithQueuedScrollEvents.add(target.get()).second)
> +        return;
> +
> +    // Per the W3C CSSOM View Module, scroll events fired at the document should bubble, others should not.
> +    bool canBubble = targetType == ScrollEventDocumentTarget ? true : false;
> +    RefPtr<Event> scrollEvent = Event::create(eventNames().scrollEvent, canBubble, false /* non cancelleable */);
> +    scrollEvent->setTarget(target);
> +    enqueueEvent(scrollEvent.release());
> +}

I'm not happy about the fact this very general class (or seemingly so) has a specific function for scroll event.  Can we move it to somewhere else?   If we didn't have this particular function, we can deploy this class in other nodes where we fire async events such as HTMLMediaElement and get rid of redundantly implemented event queues.

[Darin Adler]

(In reply to comment #35)
> If we didn't have this particular function, we can deploy this class in other nodes where we fire async events such as HTMLMediaElement and get rid of redundantly implemented event queues.

This class should not be used per-node. We either want a global queue, or a per-document queue. I don’t think we have a reason to have a separate queue for each node.

[Mihai Parparita]

(In reply to comment #36)
> (In reply to comment #35)
> > If we didn't have this particular function, we can deploy this class in other nodes where we fire async events such as HTMLMediaElement and get rid of redundantly implemented event queues.
> 
> This class should not be used per-node. We either want a global queue, or a per-document queue. I don’t think we have a reason to have a separate queue for each node.

Right, if we keep the per-document queue then then enqueueScrollEvent (and other simialar convenience methods that may be needed from multiple call sites) can stay on EventQueue. HTMLMediaElement can use enqueueEvent directly.

[Ojan Vafai]

(In reply to comment #37)
> (In reply to comment #36)
> > This class should not be used per-node. We either want a global queue, or a per-document queue. I don’t think we have a reason to have a separate queue for each node.
> 
> Right, if we keep the per-document queue then then enqueueScrollEvent (and other simialar convenience methods that may be needed from multiple call sites) can stay on EventQueue. HTMLMediaElement can use enqueueEvent directly.

Is there a problem with using a global queue? A global queue has the advantage that it will preserve order across documents, which could be a compatibility problem when we try to make mutationevents async.

[Darin Adler]

(In reply to comment #38)
> Is there a problem with using a global queue? A global queue has the advantage that it will preserve order across documents, which could be a compatibility problem when we try to make mutationevents async.

The “problem” with any of these queues is that there are times when a queued event is no longer appropriate to deliver because the object it was going to be sent to is no longer in a position to receive events. We have not figured out exactly what those times are yet, which leaves me unable to be sure a patch is correct.

Once we figure out the issues with the lifetime of the events, we can figure out how to structure the queue. Having a single global ordering is one thing we probably want; we may have other requirements, like a way to efficiently remove all events associated with a given node or with all nodes in a document. Once we understand those requirements we can choose the right data structure.

[Mihai Parparita]

Created attachment 76581 [details]
Patch

[Mihai Parparita]

Updated the patch now that the EventQueue class has landed separately with r74062.

Regarding concerns from comments 38 and 39, mutation events ended up using a scoping object to delay dispatch (but still did it synchronously), so that should no longer be a concern.

[Mihai Parparita]

Darin (or Ryosuke, now that you're a reviewer), mind looking at this again?

[Mihai Parparita]

Created attachment 78475 [details]
Patch

[Mihai Parparita]

(rebaselined patch for the Source/WebCore move)

[Darin Fisher (:fishd, Google)]

Comment on attachment 78475 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78475&action=review

> LayoutTests/fast/events/fire-scroll-event.html:34
> +      testFailed('Scroll event fired synchronously');

nit: indent by 4 spaces

> LayoutTests/fast/events/remove-child-onscroll.html:19
> +                          this.removeChild(this.firstChild)

nit: indent by 4 spaces

> Source/WebCore/dom/EventQueue.cpp:57
> +    bool canBubble = targetType == ScrollEventDocumentTarget ? true : false;

nit:  can be written without the ?: operator.

bool canBubble = targetType == ScrollEventDocumentTarget;

[Mihai Parparita]

(In reply to comment #45)
> > LayoutTests/fast/events/fire-scroll-event.html:34
> > +      testFailed('Scroll event fired synchronously');
> 
> nit: indent by 4 spaces

Fixed.

> > LayoutTests/fast/events/remove-child-onscroll.html:19
> > +                          this.removeChild(this.firstChild)
> 
> nit: indent by 4 spaces

Fixed.

> > Source/WebCore/dom/EventQueue.cpp:57
> > +    bool canBubble = targetType == ScrollEventDocumentTarget ? true : false;
> 
> nit:  can be written without the ?: operator.

Fixed.

[Mihai Parparita]

Created attachment 78582 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 78582 [details]
Patch for landing

Clearing flags on attachment: 78582

Committed r75555: <http://trac.webkit.org/changeset/75555>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

This changed fast/events/remove-child-onscroll.html to assume that scroll events are async; it does the scroll before the listener gets registered.