Bug 101857 – Updating mouse cursor on style changes without emitting fake mousemove event

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Aivo Paas]

Updating mouse cursor on style changes without emitting fake mousemove event

[Aivo Paas]

Created attachment 173493 [details]
Patch

[Aivo Paas]

Created attachment 173504 [details]
Patch

[Aivo Paas]

Comment on attachment 173504 [details]
Patch

Added tests to the patch

[Build Bot]

Comment on attachment 173504 [details]
Patch

Attachment 173504 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14788838

New failing tests:
fast/events/mouse-cursor-change.html

[WebKit Review Bot]

Comment on attachment 173504 [details]
Patch

Attachment 173504 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14790823

New failing tests:
fast/events/mouse-cursor-change.html

[Aivo Paas]

Missing braces in cases like this should clearly be spotted and reported by automatic test. It's either wrong indentation or missing braces, which must be corrected in both cases before committing

@@ -1862,7 +1862,8 @@ bool EventHandler::handleMouseMoveEvent(const PlatformMouseEvent& mouseEvent, Hi
         if (FrameView* view = m_frame->view()) {
             OptionalCursor optionalCursor = selectCursor(mev, scrollbar);
             if (optionalCursor.isCursorChange())
-                view->setCursor(optionalCursor.cursor());
+                m_currentMouseCursor = optionalCursor.cursor();
+                view->setCursor(m_currentMouseCursor);
         }
     }

[Aivo Paas]

(In reply to comment #6)
Er, the missing braces comment was meant to go to a bug about style check not detecting such errors: https://bugs.webkit.org/show_bug.cgi?id=34189
I have the error fixed in my patch.

[Aivo Paas]

Created attachment 174512 [details]
Patch

[Aivo Paas]

Comment on attachment 174512 [details]
Patch

Fixed a step in a test that previously failed because eventSender.dragMode = false; was not set. It queued move events until mouseup was emitted instead of instantly emitting them. Also checked that the included tests failed without the patch.

[Eric Seidel (no email)]

Comment on attachment 174512 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        There should be no fake mousemove event used for chaning mouse cursor when style changes.

chaning

[Eric Seidel (no email)]

I've CC'd a couple folks who have worked in hit-testing recently.  Perhaps they'll have opinions to share.

[Aivo Paas]

Created attachment 174755 [details]
Patch

[Aivo Paas]

Updated Changelog with better explanation for why the new approach is better than previous and explained the flaws of old approach. Also gave a summary of what the patch changes.

[Eric Seidel (no email)]

Comment on attachment 174755 [details]
Patch

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

> Source/WebCore/rendering/RenderObject.cpp:1808
> +    if (oldStyle.get() && !areCursorsEqual(oldStyle.get(), this->style())) {
> +        if (Frame* frame = this->frame())
> +            frame->eventHandler()->updateCursor();
> +    }

Why move this to setStyle instead of styleDidChange?

[Aivo Paas]

(In reply to comment #14)
> (From update of attachment 174755 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174755&action=review
> 
> > Source/WebCore/rendering/RenderObject.cpp:1808
> > +    if (oldStyle.get() && !areCursorsEqual(oldStyle.get(), this->style())) {
> > +        if (Frame* frame = this->frame())
> > +            frame->eventHandler()->updateCursor();
> > +    }
> 
> Why move this to setStyle instead of styleDidChange?

That's already explained in the changelog.
I spotted strange behaviour on mousedown/up on text node vs no text node. Over text node the logic in EventHandler didn't pick up the correct cursor in the first pass. My guess is that something in the StyleDidChange caused the textnode to update after the updateCursor() call was made. So, to be sure that all the things that happen inside StyleDidChange do not affect wether or not cursor gets updated, I moved it out.

[Build Bot]

Comment on attachment 174755 [details]
Patch

Attachment 174755 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14858748

New failing tests:
fast/events/mouse-cursor-change.html
inspector-protocol/nmi-webaudio.html

[Aivo Paas]

Created attachment 174920 [details]
Patch

[Aivo Paas]

Changed timeouts in tests to 0 (completely removing timeout would fail detecting new cursor value).
Lowered timeout for detecting mousemove event on cursor change according to the fake mousemove timer delay.

[Allan Sandfeld Jensen]

Comment on attachment 174920 [details]
Patch

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

> Source/WebCore/rendering/RenderObject.cpp:1809
> +    if (oldStyle.get() && !areCursorsEqual(oldStyle.get(), this->style())) {
> +        if (Frame* frame = this->frame())
> +            frame->eventHandler()->updateCursor();
> +    }
> +

Would there be a problem in only updating the cursor when node is hovered?

[Aivo Paas]

(In reply to comment #19)
> (From update of attachment 174920 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174920&action=review
> 
> > Source/WebCore/rendering/RenderObject.cpp:1809
> > +    if (oldStyle.get() && !areCursorsEqual(oldStyle.get(), this->style())) {
> > +        if (Frame* frame = this->frame())
> > +            frame->eventHandler()->updateCursor();
> > +    }
> > +
> 
> Would there be a problem in only updating the cursor when node is hovered?

I think that would be great. Is there a flag set when a node is hovered?

[Build Bot]

Comment on attachment 174920 [details]
Patch

Attachment 174920 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14911139

New failing tests:
fast/events/mouse-cursor-change.html

[Allan Sandfeld Jensen]

(In reply to comment #20)
> (In reply to comment #19)
> > (From update of attachment 174920 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=174920&action=review
> > 
> > > Source/WebCore/rendering/RenderObject.cpp:1809
> > > +    if (oldStyle.get() && !areCursorsEqual(oldStyle.get(), this->style())) {
> > > +        if (Frame* frame = this->frame())
> > > +            frame->eventHandler()->updateCursor();
> > > +    }
> > > +
> > 
> > Would there be a problem in only updating the cursor when node is hovered?
> 
> I think that would be great. Is there a flag set when a node is hovered?

Yes, check Node::hovered().

[Aivo Paas]

(In reply to comment #22)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > (From update of attachment 174920 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=174920&action=review
> > > 
> > > > Source/WebCore/rendering/RenderObject.cpp:1809
> > > > +    if (oldStyle.get() && !areCursorsEqual(oldStyle.get(), this->style())) {
> > > > +        if (Frame* frame = this->frame())
> > > > +            frame->eventHandler()->updateCursor();
> > > > +    }
> > > > +
> > > 
> > > Would there be a problem in only updating the cursor when node is hovered?
> > 
> > I think that would be great. Is there a flag set when a node is hovered?
> 
> Yes, check Node::hovered().

Tried that, didn't work. Broke tests, probably related to text nodes, because those don't seem to get the hovered flag set. This optimization opportunity should be investigated in another bug, it's going out of the scope of this bug / patch.

[Aivo Paas]

No more comments?

I wonder why mac EWS dd not pass the tests.
Might it be some problem with the WES?
Could someone with a mac check if there's a problem with fast/events/mouse-cursor-change.html?

If there's no problem on mac, would the patch get landed or what happens next?

[Aivo Paas]

What does it take to get a review or at least a comment?
It's not a complicated patch..
I don't get it - if patches are welcome, why let a bug fix lay around for weeks?

[Allan Sandfeld Jensen]

(In reply to comment #25)
> What does it take to get a review or at least a comment?
> It's not a complicated patch..
> I don't get it - if patches are welcome, why let a bug fix lay around for weeks?

I think it looks fine. There are rooms for improvement, but they can come later, but you do need pass the tests on Mac.

[Allan Sandfeld Jensen]

*** Bug 85343 has been marked as a duplicate of this bug. ***

[Allan Sandfeld Jensen]

(In reply to comment #26)
> (In reply to comment #25)
> > What does it take to get a review or at least a comment?
> > It's not a complicated patch..
> > I don't get it - if patches are welcome, why let a bug fix lay around for weeks?
> 
> I think it looks fine. There are rooms for improvement, but they can come later, but you do need pass the tests on Mac.

Alternatively you add your failing test to LayoutTests/platform/mac/TestExpectations and open a bug on it failing.

[Aivo Paas]

(In reply to comment #28)
> (In reply to comment #26)
> > (In reply to comment #25)
> > > What does it take to get a review or at least a comment?
> > > It's not a complicated patch..
> > > I don't get it - if patches are welcome, why let a bug fix lay around for weeks?
> > 
> > I think it looks fine. There are rooms for improvement, but they can come later, but you do need pass the tests on Mac.
> 
> Alternatively you add your failing test to LayoutTests/platform/mac/TestExpectations and open a bug on it failing.

Sounds like a plan, since I have no access to a Mac to investigate. It might have something to do with eventSender implementation, but wouldn't be fun to debug without a test machine.

[Allan Sandfeld Jensen]

(In reply to comment #29)
> (In reply to comment #28)
> > (In reply to comment #26)
> > > (In reply to comment #25)
> > > > What does it take to get a review or at least a comment?
> > > > It's not a complicated patch..
> > > > I don't get it - if patches are welcome, why let a bug fix lay around for weeks?
> > > 
> > > I think it looks fine. There are rooms for improvement, but they can come later, but you do need pass the tests on Mac.
> > 
> > Alternatively you add your failing test to LayoutTests/platform/mac/TestExpectations and open a bug on it failing.
> 
> Sounds like a plan, since I have no access to a Mac to investigate. It might have something to do with eventSender implementation, but wouldn't be fun to debug without a test machine.

It never is. If it had been a pre-existing test you would have to though, but since it is a new test, you are allowed a bit more freedom.

Btw, besides only doing this when necessary, the other improvement that could be done is not to construct a PlatformMouseEvent in updateCursor since you are not sending it anyway, but that would require changing the API of the selectCursor method, so it can wait for another patch.

[Jocelyn Turcotte]

Comment on attachment 174920 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:1404
> +    Settings* settings = m_frame->settings();
> +    if (settings && !settings->deviceSupportsMouse())
> +        return;
> +
> +    FrameView* view = m_frame->view();
> +    if (!view)
> +        return;
> +
> +    if (!m_frame->page() || !m_frame->page()->isOnscreen())
> +        return;

Most of this code is duplicated from fakeMouseMoveEventTimerFired, it would be nice if it would be shared. Using a function like bool "EventHandler::canUpdateMouseCursor() const" for example.
Also, any reason for not checking m_frame->page()->focusController()->isActive()?

[Ryosuke Niwa]

(In reply to comment #28)
> (In reply to comment #26)
> > (In reply to comment #25)
> > > What does it take to get a review or at least a comment?
> > > It's not a complicated patch..
> > > I don't get it - if patches are welcome, why let a bug fix lay around for weeks?
> > 
> > I think it looks fine. There are rooms for improvement, but they can come later, but you do need pass the tests on Mac.
> 
> Alternatively you add your failing test to LayoutTests/platform/mac/TestExpectations and open a bug on it failing.

That's not really an acceptable approach. Please at least try to contact someone who has access to Mac. If people contributing to WebKit kept breaking ports they don't work on, it'll eventually break all ports.

[Aivo Paas]

(In reply to comment #32)
> (In reply to comment #28)
> > (In reply to comment #26)
> > > (In reply to comment #25)
> > > > What does it take to get a review or at least a comment?
> > > > It's not a complicated patch..
> > > > I don't get it - if patches are welcome, why let a bug fix lay around for weeks?
> > > 
> > > I think it looks fine. There are rooms for improvement, but they can come later, but you do need pass the tests on Mac.
> > 
> > Alternatively you add your failing test to LayoutTests/platform/mac/TestExpectations and open a bug on it failing.
> 
> That's not really an acceptable approach. Please at least try to contact someone who has access to Mac. If people contributing to WebKit kept breaking ports they don't work on, it'll eventually break all ports.

How does a new failing test break a port? The port must already be broken in the first place, unless the test is broken. And I don't think that's the case.

Anyways, I don't have the resources to debug on Mac and I don't know anyone with a Mac who is competent to debug.

[Ryosuke Niwa]

Ah, I misunderstood the situation. If the only failing tests are ones you're adding, then adding it to TestExpectations is fine.

[Aivo Paas]

(In reply to comment #34)
> Ah, I misunderstood the situation. If the only failing tests are ones you're adding, then adding it to TestExpectations is fine.

Thanks for making that clear. I'll update the patch soon, have to find some time.

EWS logs are a pain to read, every run lists different tests failing, most of them completely unrelated. Though, all of them seem to list fast/events/mouse-cursor-change.html for some reason. It might help if someone could at least give the diff of the result for an indication of what might be wrong with it.

[Aivo Paas]

(In reply to comment #31)
> (From update of attachment 174920 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174920&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:1404
> > +    Settings* settings = m_frame->settings();
> > +    if (settings && !settings->deviceSupportsMouse())
> > +        return;
> > +
> > +    FrameView* view = m_frame->view();
> > +    if (!view)
> > +        return;
> > +
> > +    if (!m_frame->page() || !m_frame->page()->isOnscreen())
> > +        return;
> 
> Most of this code is duplicated from fakeMouseMoveEventTimerFired, it would be nice if it would be shared. Using a function like bool "EventHandler::canUpdateMouseCursor() const" for example.
> Also, any reason for not checking m_frame->page()->focusController()->isActive()?

I must have removed that check while experimenting with something. Will add it back.

[Aivo Paas]

(In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #20)
> > > (In reply to comment #19)
> > > > (From update of attachment 174920 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=174920&action=review
> > > > 
> > > > > Source/WebCore/rendering/RenderObject.cpp:1809
> > > > > +    if (oldStyle.get() && !areCursorsEqual(oldStyle.get(), this->style())) {
> > > > > +        if (Frame* frame = this->frame())
> > > > > +            frame->eventHandler()->updateCursor();
> > > > > +    }
> > > > > +
> > > > 
> > > > Would there be a problem in only updating the cursor when node is hovered?
> > > 
> > > I think that would be great. Is there a flag set when a node is hovered?
> > 
> > Yes, check Node::hovered().
> 
> Tried that, didn't work. Broke tests, probably related to text nodes, because those don't seem to get the hovered flag set. This optimization opportunity should be investigated in another bug, it's going out of the scope of this bug / patch.

I did now dig a little deeper. There were 3 problems with that:
1) Hovered flag is not set on text nodes.
2) Style change is propagated to text nodes sometime later and when hit test gets a text node, it won't have the correct cursor until its style is updated.
3) While holding mouse button down (having an element active), hovered flags are not changed on new hovered elements.

There's an easy workaround for (1) and (2) by getting the cursor from parent node when hit test gets a text node. (3) needs a change so that hovered flag is changed even when mouse button is down. I have checked and that's also how IE and FF behave - hover styles are shown also on those elements that were not pressed on, while on Chrome, hover styles are not applied to the elements that were not pressed down.

[Aivo Paas]

(In reply to comment #30)
> Btw, besides only doing this when necessary, the other improvement that could be done is not to construct a PlatformMouseEvent in updateCursor since you are not sending it anyway, but that would require changing the API of the selectCursor method, so it can wait for another patch.

I have already done that, will be in the updated patch. Everything that was used from the event, is available on HitTestResult so there is no need for the event object. selectCursor() is only used in 2 locations, so it's an easy change.

[Aivo Paas]

Created attachment 177157 [details]
Patch

[Early Warning System Bot]

Comment on attachment 177157 [details]
Patch

Attachment 177157 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15119137

[Early Warning System Bot]

Comment on attachment 177157 [details]
Patch

Attachment 177157 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15117168

[WebKit Review Bot]

Comment on attachment 177157 [details]
Patch

Attachment 177157 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15100296

[Aivo Paas]

Created attachment 177158 [details]
Patch

[Aivo Paas]

Created attachment 177160 [details]
Patch

[Build Bot]

Comment on attachment 177160 [details]
Patch

Attachment 177160 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15120136

New failing tests:
fast/events/mouse-cursor-change.html

[Build Bot]

Comment on attachment 177160 [details]
Patch

Attachment 177160 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15119167

New failing tests:
fast/events/mouse-cursor-change.html

[Aivo Paas]

Created attachment 177187 [details]
Patch

[Aivo Paas]

Added the failing test to mac test expectations

[Allan Sandfeld Jensen]

Comment on attachment 177187 [details]
Patch

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

Nice to see the improvements, but since you made a change in behavior that was obviously intended for some reason, we should change that as part of another bug. Since it was an optimization, this patch can land without.

> Source/WebCore/dom/Document.cpp:-5755
> -    // If the mouse is down and if this is a mouse move event, we want to restrict changes in
> -    // :hover/:active to only apply to elements that are in the :active chain that we froze
> -    // at the time the mouse went down.
> -    bool mustBeInActiveChain = request.active() && request.move();
> -

I don't think it is a good idea to change this behaviour in this patch. We should figure out why it was introduced in the first place and fix it in another bug if we think it is still a good idea.

[Aivo Paas]

(In reply to comment #49)
> (From update of attachment 177187 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177187&action=review
> 
> Nice to see the improvements, but since you made a change in behavior that was obviously intended for some reason, we should change that as part of another bug. Since it was an optimization, this patch can land without.
> 
> > Source/WebCore/dom/Document.cpp:-5755
> > -    // If the mouse is down and if this is a mouse move event, we want to restrict changes in
> > -    // :hover/:active to only apply to elements that are in the :active chain that we froze
> > -    // at the time the mouse went down.
> > -    bool mustBeInActiveChain = request.active() && request.move();
> > -
> 
> I don't think it is a good idea to change this behaviour in this patch. We should figure out why it was introduced in the first place and fix it in another bug if we think it is still a good idea.

I also thought about making the behavior change separate. Skipping the hovered() check should be enough to go without the change. I'll also dig up where the condition was introduced to see if there's a good reason for not being consistent with other browsers.

[Aivo Paas]

Comment on attachment 177187 [details]
Patch

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

>>> Source/WebCore/dom/Document.cpp:-5755
>>> -
>> 
>> I don't think it is a good idea to change this behaviour in this patch. We should figure out why it was introduced in the first place and fix it in another bug if we think it is still a good idea.
> 
> I also thought about making the behavior change separate. Skipping the hovered() check should be enough to go without the change. I'll also dig up where the condition was introduced to see if there's a good reason for not being consistent with other browsers.

Found where this behavior was introduced: http://trac.webkit.org/changeset/9957

Quoting from the Changelog:
> (b) If you mouse down on a non-selectable region and start moving, then the behavior has changed.  Instead of not updating at all, we mark the chain at the time the mouse goes down, and we restrict :hover/:active updates to only apply to elements that are in that chain.  This yields perfect hover/active control behavior, even when :active has been applied hierarchically.

Not sure what the "perfect hover/active control behavior" meant 7 years ago but it clearly is not what other browsers think about it today. CSS spec does not mention such limiting condition for applying :hover pseudo class.

[Allan Sandfeld Jensen]

(In reply to comment #51)
> Not sure what the "perfect hover/active control behavior" meant 7 years ago but it clearly is not what other browsers think about it today. CSS spec does not mention such limiting condition for applying :hover pseudo class.

I am guessing it is to avoid showing something only being selected as being active. Active style should only be rendered if the element would be activated if the user released the mouse-button, but if you are just over the element as part of a mouse selection, then it will not be activated and should not be rendered with active style.

[Aivo Paas]

(In reply to comment #52)
> (In reply to comment #51)
> > Not sure what the "perfect hover/active control behavior" meant 7 years ago but it clearly is not what other browsers think about it today. CSS spec does not mention such limiting condition for applying :hover pseudo class.
> 
> I am guessing it is to avoid showing something only being selected as being active. Active style should only be rendered if the element would be activated if the user released the mouse-button, but if you are just over the element as part of a mouse selection, then it will not be activated and should not be rendered with active style.

This change only affects hover style, not active.

[Allan Sandfeld Jensen]

(In reply to comment #53)
> (In reply to comment #52)
> > (In reply to comment #51)
> > > Not sure what the "perfect hover/active control behavior" meant 7 years ago but it clearly is not what other browsers think about it today. CSS spec does not mention such limiting condition for applying :hover pseudo class.
> > 
> > I am guessing it is to avoid showing something only being selected as being active. Active style should only be rendered if the element would be activated if the user released the mouse-button, but if you are just over the element as part of a mouse selection, then it will not be activated and should not be rendered with active style.
> 
> This change only affects hover style, not active.

The part I commented on affects :active and :hover equally, but as we have discussed, it will be fine to put in a separate patch/bug, then we can discuss its implications there.

[Aivo Paas]

(In reply to comment #54)
> (In reply to comment #53)
> > (In reply to comment #52)
> > > (In reply to comment #51)
> > > > Not sure what the "perfect hover/active control behavior" meant 7 years ago but it clearly is not what other browsers think about it today. CSS spec does not mention such limiting condition for applying :hover pseudo class.
> > > 
> > > I am guessing it is to avoid showing something only being selected as being active. Active style should only be rendered if the element would be activated if the user released the mouse-button, but if you are just over the element as part of a mouse selection, then it will not be activated and should not be rendered with active style.
> > 
> > This change only affects hover style, not active.
> 
> The part I commented on affects :active and :hover equally, but as we have discussed, it will be fine to put in a separate patch/bug, then we can discuss its implications there.

Actually it doesn't. It only checks if the node is activated to decide if hovered flags should be changed or not. But you are right, this belongs in another bug.

[Alexey Proskuryakov]

Could you please clarify the relationship between this bug and bug 103857? How did you confirm that you are not introducing a regression on Mac here?

It is indeed sometimes acceptable to leave a test for new functionality failing on some platforms, but hover effects are not new functionality. I do not see how it's appropriate to land this with a mystery test failure that hasn't been investigated at all.

[Alexey Proskuryakov]

What platforms have you tested this with manually? The only other EWS that runs tests is cr-linux, so it could be that the new test fails everywhere except to cr-linux.

[Aivo Paas]

(In reply to comment #56)
> Could you please clarify the relationship between this bug and bug 103857? How did you confirm that you are not introducing a regression on Mac here?
> 
> It is indeed sometimes acceptable to leave a test for new functionality failing on some platforms, but hover effects are not new functionality. I do not see how it's appropriate to land this with a mystery test failure that hasn't been investigated at all.

There could be no regression because the test is for an improvement / new functionality. It is for the case when changing cursor while a mouse button is hold down. Before this patch there were no cursor changes happening in this case. There are other tests for simple hovering which worked before and works with the patch.

I have manually tested it only on linux because I have no access to a mac.

[Allan Sandfeld Jensen]

(In reply to comment #58)
> (In reply to comment #56)
> > Could you please clarify the relationship between this bug and bug 103857? How did you confirm that you are not introducing a regression on Mac here?
> > 
> > It is indeed sometimes acceptable to leave a test for new functionality failing on some platforms, but hover effects are not new functionality. I do not see how it's appropriate to land this with a mystery test failure that hasn't been investigated at all.
> 
> There could be no regression because the test is for an improvement / new functionality. It is for the case when changing cursor while a mouse button is hold down. Before this patch there were no cursor changes happening in this case. There are other tests for simple hovering which worked before and works with the patch.
> 
> I have manually tested it only on linux because I have no access to a mac.

On linux, it should still be possible to test with both Chrome and Qt ports of webkit, and sometimes even GTK+ if you are lucky.

[Simon Fraser (smfr)]

Comment on attachment 177187 [details]
Patch

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

> Source/WebCore/rendering/RenderObject.cpp:1813
> +    if (oldStyle.get() && !areCursorsEqual(oldStyle.get(), this->style())) {
> +        if (Frame* frame = this->frame()) {
> +            if (node() && node()->hovered())
> +                frame->eventHandler()->updateCursor();
> +        }
> +    }

The old code did some coalescing (by virtue of the "soon"), and yours does not. I'd be worried that this could create a performance issue.

[Aivo Paas]

(In reply to comment #60)
> (From update of attachment 177187 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177187&action=review
> 
> > Source/WebCore/rendering/RenderObject.cpp:1813
> > +    if (oldStyle.get() && !areCursorsEqual(oldStyle.get(), this->style())) {
> > +        if (Frame* frame = this->frame()) {
> > +            if (node() && node()->hovered())
> > +                frame->eventHandler()->updateCursor();
> > +        }
> > +    }
> 
> The old code did some coalescing (by virtue of the "soon"), and yours does not. I'd be worried that this could create a performance issue.

The old code used a path that was built for use in scrolling, that's why it has the throttling built in. I don't think that changing cursor is so common to affect performance. My code also checks if the node that had the cursor changed is hovered or not. Old code changed the cursor even when cursor was changed on a node that was not hovered, not to mention the completely unnecessary mousemove event that was fired all the way up to javascript.

By the way, there's a lot more code run on every mousemove event. My code is a cleaned up version of that doing only the things needed for changing the cursor. And I couldn't think of a reason why someone would change mouse cursor more often than a mousemove event is fired - unless, of course, if that's what they intend to do.

[Simon Fraser (smfr)]

It would be easy to make a testcase that changes cursor style in a tight JS loop. Have you tried that?

[Aivo Paas]

(In reply to comment #62)
> It would be easy to make a testcase that changes cursor style in a tight JS loop. Have you tried that?

I might try, but what's the use case for such test?

With a JS loop you can hang a browser without doing anything at all in the loop. There is no point for a plain stupid test, don't you think?

What I'm trying to say is that moving mouse over a page runs the same code plus a lot more for every single mousemove event. If moving mouse does not have performance issues then changing a cursor will be no different.

[Aivo Paas]

Created attachment 182485 [details]
Patch

[Aivo Paas]

Sorry, had not time to work on this issue for over a month.
I made minor changes to make the patch build with the changes that happened while I was away.

I am still waiting for a good explanation for why there could be need for changing mouse cursor in a tight loop or even as slow as 60 times per second, which is common monitor refresh rate. I tested changing cursor every millisecond and it did not have any noticeable performance issues. In fact, on the linux machine I develop on, it consumed about 2 times less CPU in Chrome than it does on FF with the same test (http://jsbin.com/umiwuy/3/edit).

[Aivo Paas]

ping, it's been over a week..

[Eric Seidel (no email)]

At 65 comments, and 23k patch (with an unexplained failing test), it makes this bug hard to review.  Definitely doable, but your best chance is to engage a reviewer over #webkit and walk them through it.

You might try connecting with carewolf (Allan Jensen), although he's not a reviewer, he's been in some of the mouse handling code recently and his "LGTM" is thus meaningful.

[Eric Seidel (no email)]

(In reply to comment #67)
> At 65 comments, and 23k patch (with an unexplained failing test), it makes this bug hard to review.  Definitely doable, but your best chance is to engage a reviewer over #webkit and walk them through it.
> 
> You might try connecting with carewolf (Allan Jensen), although he's not a reviewer, he's been in some of the mouse handling code recently and his "LGTM" is thus meaningful.

Sorry.  I don't know what I was thinking.  Allan has been a reviewer for years.
http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py#L481

[Allan Sandfeld Jensen]

(In reply to comment #55)
> (In reply to comment #54)
> > (In reply to comment #53)
> > > (In reply to comment #52)
> > > > (In reply to comment #51)
> > > > > Not sure what the "perfect hover/active control behavior" meant 7 years ago but it clearly is not what other browsers think about it today. CSS spec does not mention such limiting condition for applying :hover pseudo class.
> > > > 
> > > > I am guessing it is to avoid showing something only being selected as being active. Active style should only be rendered if the element would be activated if the user released the mouse-button, but if you are just over the element as part of a mouse selection, then it will not be activated and should not be rendered with active style.
> > > 
> > > This change only affects hover style, not active.
> > 
> > The part I commented on affects :active and :hover equally, but as we have discussed, it will be fine to put in a separate patch/bug, then we can discuss its implications there.
> 
> Actually it doesn't. It only checks if the node is activated to decide if hovered flags should be changed or not. But you are right, this belongs in another bug.

Could you move it? I guess it is also the change that causes the existing hover-active-drag test to change. I would feel more comfortable accepting the patch if it limits its side-effects.

[Aivo Paas]

(In reply to comment #69)
> Could you move it? I guess it is also the change that causes the existing hover-active-drag test to change. I would feel more comfortable accepting the patch if it limits its side-effects.

Oh, I forgot that one while I was away.

The thing is, if this is moved away, I can no longer check for if the element that had style changed is hovered or not. With that change in the patch it is performs a lot better, it does not even try to update cursor if the style change didn't happen under the cursor.

What do you men by "it is also the change that causes the existing hover-active-drag test to change"? The change makes hover working as it does in the other browsers - hover state is updated even when a mouse button is hold down while old code does not do that.

I couldn't find a single reason for the mustBeInActiveChain check. As noted, it has been there from 2005. The patch back then did not give any reason for that check nor did it touch any tests.

I have added David Hyatt to CC list (the one who introduced that check there). Maybe he can share some insight on that.

[Allan Sandfeld Jensen]

(In reply to comment #70)
> I couldn't find a single reason for the mustBeInActiveChain check. As noted, it has been there from 2005. The patch back then did not give any reason for that check nor did it touch any tests.
> 
The biggest problem I have with it is that it affects both Active and Hover since they are set using the same nodesToAddToChain vector. We could use two different vectors, but that could get confusing.

[Aivo Paas]

(In reply to comment #71)
> (In reply to comment #70)
> > I couldn't find a single reason for the mustBeInActiveChain check. As noted, it has been there from 2005. The patch back then did not give any reason for that check nor did it touch any tests.
> > 
> The biggest problem I have with it is that it affects both Active and Hover since they are set using the same nodesToAddToChain vector. We could use two different vectors, but that could get confusing.

I see your concern. Form what I read from the source, active is only touched when some new element got activated (controlled through bool allowActiveChanges = !oldActiveElement && activeElement();). My manual test also confirms that removing this check did not make active element to change on mouse move, but hovered element changed as it should. Have a look at http://jsbin.com/onomul/3/edit

Maybe a new unit test could help us out here? But I'm not sure what or how exactly it should test because this behavior doesn't seem to have any tests, does it?

[Darin Adler]

The reason this change was originally made is that computer user interfaces typically do a sort of “lock” when you click and drag on something. Giving hover feedback to indicate what a click would do when you are in the process of dragging something over that item you could click doesn’t make sense. That’s what drove the change.

The fact that this is now different from other browsers is unfortunate.

It’s too bad that it’s hard to make a website that will work properly. If I am dragging some text over a link, the cursor should not change into the “hand” cursor nor should the link light up with its hover feedback.

[Aivo Paas]

(In reply to comment #73)
> The reason this change was originally made is that computer user interfaces typically do a sort of “lock” when you click and drag on something. Giving hover feedback to indicate what a click would do when you are in the process of dragging something over that item you could click doesn’t make sense. That’s what drove the change.
> 
> The fact that this is now different from other browsers is unfortunate.
> 
> It’s too bad that it’s hard to make a website that will work properly. If I am dragging some text over a link, the cursor should not change into the “hand” cursor nor should the link light up with its hover feedback.

W3C: "The :hover pseudo-class applies while the user designates an element with a pointing device, but does not necessarily activate it."

There's no such limit defined in the spec that forbids applying :hover when there's an element activated. Hover state should be applied to the element that is under mouse pointer, but current code freezes hover state whenever mouse button is pressed down. Doesn't that conflict with the spec?

[Aivo Paas]

Ping for comments.
I would very much like to get that patch landed and getting no feedback doesn't really help.

[Alexey Proskuryakov]

Doesn't comment 73 provide sufficient feedback? No spec can possibly validate such an obvious UI deficiency.

Getting rid of the fake event seems worthwhile, but we shouldn't degrade UI when doing so.

[Aivo Paas]

(In reply to comment #76)
> Doesn't comment 73 provide sufficient feedback? No spec can possibly validate such an obvious UI deficiency.
> 
> Getting rid of the fake event seems worthwhile, but we shouldn't degrade UI when doing so.

What exactly do you call an obvious UI deficiency?

If hover state is locked on mousedown, there should be another fast and reliable way for checking which element is actually under the mouse cursor (or in the chain of parents of that element).I did not find another way, feel free to point me in the right direction if there is one.

Hover state would be perfect for that if only it represented the actual hovered state, not the locked one.
Any suggestions how to do that?
I think it would be pointless to add another flag for the real hover state just because there already is a broken one. Why not fix the current hover state flag? As I already pointed out, it does make hover state consistent with other browsers.

[Allan Sandfeld Jensen]

Just to keep things simple. Would anyone object to something closer to the original patch (modulo changes from comments) without the optimization that requires better knowledge of hover?

Second and third, we could then look at the more complicated issues of UI and how to best fast determine/remember if the mouse is over a node.

[Aivo Paas]

Created attachment 187844 [details]
Patch

[Aivo Paas]

Removed hover check before calling EventHandler::updateCursor()
And also removed the change in UX of :active and :hover states.

[Allan Sandfeld Jensen]

Comment on attachment 187844 [details]
Patch

LGTM, though a follow up patch should be opened for the optimization opportunity.

[Allan Sandfeld Jensen]

(In reply to comment #81)
> (From update of attachment 187844 [details])
> LGTM, though a follow up patch should be opened for the optimization opportunity.

Ehr, make that 'follow up bug'.

[Aivo Paas]

(In reply to comment #82)
> (In reply to comment #81)
> > (From update of attachment 187844 [details] [details])
> > LGTM, though a follow up patch should be opened for the optimization opportunity.
> 
> Ehr, make that 'follow up bug'.

Opened follow-up bug 109583

[Aivo Paas]

land, anyone?

[WebKit Review Bot]

Comment on attachment 187844 [details]
Patch

Clearing flags on attachment: 187844

Committed r142861: <http://trac.webkit.org/changeset/142861>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

This patch is fundamentally wrong, and a huge performance regression.

It's wrong because you're do hit testing (via updateCursor()) inside of style recalc. Hit testing relies on geometry, but since style recalc happens before layout, the geometry is not up-to-date.

Secondly, it's a huge perf hit. I profiled the web inspector while dragging a separator around, and see this:

Running Time	Self		Symbol Name
7864.0ms   96.6%	0.0	 	                           WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
7849.0ms   96.4%	0.0	 	                            WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
7842.0ms   96.3%	0.0	 	                             WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
5233.0ms   64.3%	1.0	 	                              WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
4534.0ms   55.7%	0.0	 	                               WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
1932.0ms   23.7%	1.0	 	                                WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
1614.0ms   19.8%	0.0	 	                                WebCore::Text::recalcTextStyle(WebCore::Node::StyleChange)
1612.0ms   19.8%	1.0	 	                                 WebCore::RenderObject::setStyle(WTF::PassRefPtr<WebCore::RenderStyle>)
1607.0ms   19.7%	0.0	 	                                  WebCore::EventHandler::updateCursor()
1583.0ms   19.4%	0.0	 	                                   WebCore::RenderView::hitTest(WebCore::HitTestRequest const&, WebCore::HitTestResult&)
1583.0ms   19.4%	0.0	 	                                    WebCore::RenderLayer::hitTest(WebCore::HitTestRequest const&, WebCore::HitTestLocation const&, WebCore::HitTestResult&)
1580.0ms   19.4%	1.0	 	                                     WebCore::RenderLayer::hitTestLayer(WebCore::RenderLayer*, WebCore::RenderLayer*, WebCore::HitTestRequest const&, WebCore::HitTestResult&, WebCore::LayoutRect const&, WebCore::HitTestLocation const&, bool, WebCore::HitTestingTransformState const*, double*)
1578.0ms   19.3%	0.0	 	                                      WebCore::RenderLayer::hitTestList(WTF::Vector<WebCore::RenderLayer*, 0ul>*, WebCore::RenderLayer*, WebCore::HitTestRequest const&, WebCore::HitTestResult&, WebCore::LayoutRect const&, WebCore::HitTestLocation const&, WebCore::HitTestingTransformState const*, double*, double*, WebCore::HitTestingTransformState const*, bool)
1578.0ms   19.3%	0.0	 	                                       WebCore::RenderLayer::hitTestLayer(WebCore::RenderLayer*, WebCore::RenderLayer*, WebCore::HitTestRequest const&, WebCore::HitTestResult&, WebCore::LayoutRect const&, WebCore::HitTestLocation const&, bool, WebCore::HitTestingTransformState const*, double*)
1578.0ms   19.3%	0.0	 	                                        WebCore::RenderLayer::hitTestList(WTF::Vector<WebCore::RenderLayer*, 0ul>*, WebCore::RenderLayer*, WebCore::HitTestRequest const&, WebCore::HitTestResult&, WebCore::LayoutRect const&, WebCore::HitTestLocation const&, WebCore::HitTestingTransformState const*, double*, double*, WebCore::HitTestingTransformState const*, bool)
1577.0ms   19.3%	0.0	 	                                         WebCore::RenderLayer::hitTestLayer(WebCore::RenderLayer*, WebCore::RenderLayer*, WebCore::HitTestRequest const&, WebCore::HitTestResult&, WebCore::LayoutRect const&, WebCore::HitTestLocation const&, bool, WebCore::HitTestingTransformState const*, double*)
1572.0ms   19.3%	4.0	 	                                          WebCore::RenderLayer::hitTestList(WTF::Vector<WebCore::RenderLayer*, 0ul>*, WebCore::RenderLayer*, WebCore::HitTestRequest const&, WebCore::HitTestResult&, WebCore::LayoutRect const&, WebCore::HitTestLocation const&, WebCore::HitTestingTransformState const*, double*, double*, WebCore::HitTestingTransformState const*, bool)
1567.0ms   19.2%	5.0	 	                                           WebCore::RenderLayer::hitTestLayer(WebCore::RenderLayer*, WebCore::RenderLayer*, WebCore::HitTestRequest const&, WebCore::HitTestResult&, WebCore::LayoutRect const&, WebCore::HitTestLocation const&, bool, WebCore::HitTestingTransformState const*, double*)
1528.0ms   18.7%	122.0	 	                                            WebCore::RenderLayer::hitTestList(WTF::Vector<WebCore::RenderLayer*, 0ul>*, WebCore::RenderLayer*, WebCore::HitTestRequest const&, WebCore::HitTestResult&, WebCore::LayoutRect const&, WebCore::HitTestLocation const&, WebCore::HitTestingTransformState const*, double*, double*, WebCore::HitTestingTransformState const*, bool)
1352.0ms   16.6%	182.0	 	                                             WebCore::RenderLayer::hitTestLayer(WebCore::RenderLayer*, WebCore::RenderLayer*, WebCore::HitTestRequest const&, WebCore::HitTestResult&, WebCore::LayoutRect const&, WebCore::HitTestLocation const&, bool, WebCore::HitTestingTransformState const*, double*)

[Simon Fraser (smfr)]

I am going to revert this.

[Simon Fraser (smfr)]

The patch is also bad because you're hit testing for every style change that involves a change in cursor style, but using the last known mouse position each time. You should only hit test once with that position, not every time you see a cursor change in the style.

Reverted in http://trac.webkit.org/changeset/142956

[Aivo Paas]

I had a fear for that, will have to use a timer just like the fake mousemove has.

[Aivo Paas]

Created attachment 188520 [details]
Patch

[Aivo Paas]

Created attachment 188522 [details]
Patch

[Aivo Paas]

Style cursor change is now calling EventHandler::scheduleCursorUpdate() which is set to call updateCursor after 20ms (50Hz).
This should give plenty of time for style and layout changes to happen.
The timer is also stopped when cursor is changed through mousemove, which calls selectCursor directly.

This should be fine to iron out the performance hit?

I'm also curious about how "dragging a separator around" could have caused a cursor change in style. Might it have been a false positive in RenderObject::areCursorsEqual()?

[Aivo Paas]

Created attachment 188525 [details]
Patch

[WebKit Review Bot]

Comment on attachment 188525 [details]
Patch

Attachment 188525 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16584162

New failing tests:
fast/events/mouse-cursor-change.html
fast/events/mouse-cursor-no-mousemove.html

[Aivo Paas]

Now that's unfortunate, I completely forgot about those cursor tests being in near-sync.
Will have to use longer timeouts I guess, which is not nice. I don't think there are other options. Any thoughts on that?

[Simon Fraser (smfr)]

Comment on attachment 188525 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:1245
> +    m_cursorTimer.stop();

You don't need to stop the timer here.

> Source/WebCore/page/EventHandler.h:283
> +    void cursorTimerFired(Timer<EventHandler>*);

cursorUpdateTimerFired.

> Source/WebCore/page/EventHandler.h:411
> +    Timer<EventHandler> m_cursorTimer;

This should be called m_cursorUpdateTimer

> Source/WebCore/rendering/RenderObject.cpp:1813
> +    if (oldStyle.get() && !areCursorsEqual(oldStyle.get(), this->style())) {
> +        if (Frame* frame = this->frame())
> +            frame->eventHandler()->scheduleCursorUpdate();
> +    }

Why not leave this in styleDidChange like the old code? setStyle() should not be polluted with  stuff like this. The hunk above would also then not be required.

I'm still not convinced that this is the correct place to do this. I think cursor updates should really happen after layout, and a zero-delay timer after a style change doesn't necessarily mean that layout has happened. Some style changes won't result in layout (just painting), so it's OK to update the cursor then. But others do, and the layout may be delayed by layout timers in FrameView, in which case we should also delay the cursor update.

[Aivo Paas]

I'll do the suggested changes.

I think we should go for a simple solution here just to get the 2 annoying bugs fixed and investigate a better solution under a new bug. It's just not that easy to find the correct place for a timer-less cursor update. I am far too unfamiliar with the code base at the moment so it would require a lot of digging around. I am just trying my best at fixing a long standing bug.

[Simon Fraser (smfr)]

I'm OK with the current patch if the RenderObject changes are cleaned up.

[Aivo Paas]

Created attachment 188633 [details]
Patch

[Aivo Paas]

Created attachment 188638 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 188638 [details]
Patch

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

> Source/WebCore/rendering/RenderObject.cpp:1971
> -            frame->eventHandler()->dispatchFakeMouseMoveEventSoon();
> +            frame->eventHandler()->scheduleCursorUpdate();

Why isn't dispatchFakeMouseMoveEventSoon() entirely removed?

[Simon Fraser (smfr)]

Also, does this patch cause a behavior change where the cursor is changed when the mouse is down?

[Antonio Gomes]

Comment on attachment 187844 [details]
Patch

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

> Source/WebCore/rendering/RenderObject.cpp:1810
> +    if (oldStyle.get() && !areCursorsEqual(oldStyle.get(), this->style())) {

is .get() needed?

[Aivo Paas]

(In reply to comment #104)
> (From update of attachment 187844 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187844&action=review
> 
> > Source/WebCore/rendering/RenderObject.cpp:1810
> > +    if (oldStyle.get() && !areCursorsEqual(oldStyle.get(), this->style())) {
> 
> is .get() needed?

It was there, but it doesn't matter, latest patch doesn't have that line any more.

[Aivo Paas]

(In reply to comment #102)
> (From update of attachment 188638 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188638&action=review
> 
> > Source/WebCore/rendering/RenderObject.cpp:1971
> > -            frame->eventHandler()->dispatchFakeMouseMoveEventSoon();
> > +            frame->eventHandler()->scheduleCursorUpdate();
> 
> Why isn't dispatchFakeMouseMoveEventSoon() entirely removed?

Last time I checked it was still used in scrolling.

> Also, does this patch cause a behavior change where the cursor is changed when the mouse is down?

There is no behavior change except that it respects the cursor changes initiated by user code (cursor value change, class change causing cursor change etc.).

[Aivo Paas]

Any more concerns or is it ok for landing?

Will this patch change the cursor when an element is set to "display: none"?

I've got some code that pulls a transparent blocking window over the page while an AJAX call is in progress. Once the ajax call returns, the blocking window is removed, but the cursor remains the same until I move the mouse.

On Firefox, the cursor automatically changes back without having to move the mouse.

[Aivo Paas]

(In reply to comment #108)
> Will this patch change the cursor when an element is set to "display: none"?
> 
> I've got some code that pulls a transparent blocking window over the page while an AJAX call is in progress. Once the ajax call returns, the blocking window is removed, but the cursor remains the same until I move the mouse.
> 
> On Firefox, the cursor automatically changes back without having to move the mouse.

This patch does not handle updating cursor on layout change. It only handled the case when cursor value is changed in styles. There should already be a separate bug for layout changes not updating cursor.

[Aivo Paas]

Created attachment 190475 [details]
Patch

[Aivo Paas]

Rebased just to please all the EWS'es, no idea what happened last time.
Still waiting for re-review.

[Aivo Paas]

Simon Fraser, could you please review?
I have tried to catch you on #webkit but I guess time zone difference does it's job too good these days.

[Simon Fraser (smfr)]

Comment on attachment 190475 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:149
> +const double cursorUpdateInterval = 0.02;

Calling this "internal" makes it sounds like the timer repeats. It would be better as "delay".

> Source/WebCore/page/EventHandler.cpp:1254
> +    HitTestRequest request(HitTestRequest::ReadOnly);
> +    HitTestResult result(view->windowToContents(m_lastKnownMousePosition));
> +    m_frame->document()->renderView()->hitTest(request, result);

I think you should call  m_frame->document()->updateLayoutIgnorePendingStylesheets(); before this code. Layout can be delayed on timers, and you need to ensure that layout is up to date before hit testing.

Alternatively (perhaps preferably), you could just bail if FrameView::needsLayout() returns true, butI wonder if that would result in cases where the cursor update would be postponed forever.

[Aivo Paas]

(In reply to comment #113)
> Calling this "internal" makes it sounds like the timer repeats. It would be better as "delay".

I'd say it's still interval, because it doesn't get infinitely delayed, but cursor will indeed be changed with that interval when requested frequently enough.

> > Source/WebCore/page/EventHandler.cpp:1254
> > +    HitTestRequest request(HitTestRequest::ReadOnly);
> > +    HitTestResult result(view->windowToContents(m_lastKnownMousePosition));
> > +    m_frame->document()->renderView()->hitTest(request, result);
> 
> I think you should call  m_frame->document()->updateLayoutIgnorePendingStylesheets(); before this code. Layout can be delayed on timers, and you need to ensure that layout is up to date before hit testing.
> 
> Alternatively (perhaps preferably), you could just bail if FrameView::needsLayout() returns true, butI wonder if that would result in cases where the cursor update would be postponed forever.

Will add m_frame->document()->updateLayoutIgnorePendingStylesheets();
Bailing would be practically the same as doing nothing about it, because there won't be another EventHandler::cupdateCursor() call for the same same change and another change may never come.

[Aivo Paas]

Created attachment 191550 [details]
Patch

[Darin Adler]

Comment on attachment 191550 [details]
Patch

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

Idea looks pretty good, but patch still needs work.

Not good that the test fails on Mac, by the way, and we should find out why before landing.

> Source/WebCore/ChangeLog:27
> +        (WebCore):

Please remove this bogus line.

> Source/WebCore/ChangeLog:36
> +        (EventHandler):

Please remove this bogus line.

> Source/WebCore/page/EventHandler.cpp:149
> +// The amount of time to wait for a cursor update on style and layout changes
> +// Set to 50Hz, no need to be faster than common screen refresh rate
> +const double cursorUpdateInterval = 0.02;

It’s a real shame that all our various timers such as the layout timer, repaint timer, and cursor update timer are independent. Some day I’d prefer to see a common scheme that linked them all since they are all about updating what’s visible on-screen.

> Source/WebCore/page/EventHandler.cpp:1238
> +    Settings* settings = m_frame->settings();
> +    if (settings && !settings->deviceSupportsMouse())
> +        return;

Why don’t I see deletion of old code that checked deviceSupportsMouse? If this was copied and pasted from somewhere, we need to refactor so we share that code instead of copying it.

Not new to this patch, but it’s crazy that this is a “setting”. That’s not the right way to expose something like that to WebKit.

> Source/WebCore/page/EventHandler.cpp:1245
> +    if (!m_frame->page() || !m_frame->page()->isOnscreen() || !m_frame->page()->focusController()->isActive())
> +        return;

Again, was this rule copied and pasted from somewhere? We don’t just want another copy.

I’d like to see these checks about when it’s OK to set the cursor bundled up into a FrameView member function called shouldSetCursor or something like that. I think I’d include the deviceSupportsMouse check there along with these. I hope this would cut down on code duplication; not sure where else these checks already exist and I am not easily able to grep the code at this moment.

> Source/WebCore/page/EventHandler.cpp:1251
> +    bool shiftKey;
> +    bool ctrlKey;
> +    bool altKey;
> +    bool metaKey;
> +    PlatformKeyboardEvent::getCurrentModifierState(shiftKey, ctrlKey, altKey, metaKey);

It’s not correct to combine the shift key state from the time of cursor adjustment with the mouse position from m_lastKnownMousePosition. Since we need shift key state to select the cursor, then we need to store the shift key state along with the mouse position at the time the mouse position is stored.

> Source/WebCore/page/EventHandler.cpp:1253
> +    m_frame->document()->updateLayoutIgnorePendingStylesheets();

This is wrong. A plain updateLayout would be OK, but there’s no reason for us to force a flash of unstyled content here, which is what updateLayoutIgnorePendingStylesheets does.

> Source/WebCore/page/EventHandler.cpp:1257
> +    m_frame->document()->renderView()->hitTest(request, result);

Better to call view->renderView() rather than m_frame->document()->renderView(). Also, the RenderView can be zero, so we should do a null check.

> Source/WebCore/page/EventHandler.cpp:1269
> +    if (m_cursorUpdateTimer.isActive())
> +        m_cursorUpdateTimer.stop();

We can just call stop unconditionally. No real reason to check isActive. Some other call sites might do it, but they are either incorrect, or trying to optimize.

This is not the correct place to stop the timer. We want to stop the timer when we set the cursor, not when we decide what to do set it to. Adding a side effect to a function that is supposed to tell us what cursor to use is not good design. This might require a little refactoring of the selectCursor call sites; maybe we can combine the selectCursor, setCursor, setting m_currentMouseCursor, and stopping the m_cursorUpdateTimer and use that function in all the appropriate places.

> Source/WebCore/page/EventHandler.cpp:1291
> -    Node* node = event.targetNode();
> -    RenderObject* renderer = node ? node->renderer() : 0;
> +    Node* node = result.targetNode();
> +    if (!node)
> +        return NoCursorChange;
> +    bool originalIsText = node->isTextNode();
> +    if (node && originalIsText)
> +        node = node->parentNode();
> +    if (!node)
> +        return NoCursorChange;
> +
> +    RenderObject* renderer = node->renderer();

We added a check for a text node. Does this fix a bug? Where is the test case for the bug it fixes? This fix should go in separately rather than being coupled with the rest of the change, or rolled out. The change log makes no mention of this.

> Source/WebCore/page/EventHandler.cpp:1354
> +        bool editable = (node->rendererIsEditable());

Please remove the parentheses.

> Source/WebCore/page/EventHandler.cpp:1356
> -        if (useHandCursor(node, event.isOverLink(), event.event().shiftKey()))
> +        if (useHandCursor(node, result.URLElement() && result.URLElement()->isLink(), shiftKey))

It’s not good that to copy the body of the isOverLink function from MouseEventWithHitTestResults here, even though it’s simple. We don’t just want to copy code; we want to reuse it. If we have to make a fix we don’t want to have to fix it in two places. We could move the isOverLink function into HitTestResult, then could use it here.

> Source/WebCore/page/EventHandler.cpp:1366
> -        if ((editable || (renderer && renderer->isText() && node->canStartSelection())) && !inResizer && !scrollbar)
> +        if ((editable || (originalIsText && node->canStartSelection())) && !inResizer && !result.scrollbar())

Again, does this fix a bug? Please don’t mix this in with your patch unless it’s really new to your patch.

> LayoutTests/fast/events/mouse-cursor-no-mousemove.html:17
> +var CURSOR_UPDATE_DELAY = 50;

Not good that this test races the cursor updating code. We should find a more reliable way to test this.

[Aivo Paas]

Comment on attachment 191550 [details]
Patch

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

>> Source/WebCore/ChangeLog:27
>> +        (WebCore):
> 
> Please remove this bogus line.

Come on, those are added by the script and there's tons of lines like that committed.
Will remove, but that's just trolling.

>> Source/WebCore/page/EventHandler.cpp:1238
>> +        return;
> 
> Why don’t I see deletion of old code that checked deviceSupportsMouse? If this was copied and pasted from somewhere, we need to refactor so we share that code instead of copying it.
> 
> Not new to this patch, but it’s crazy that this is a “setting”. That’s not the right way to expose something like that to WebKit.

Will remove that. It came from the fake mousemove event path. I'm not sure, but I think it was used in other places too, that's why I chose to keep it.

>> Source/WebCore/page/EventHandler.cpp:1245
>> +        return;
> 
> Again, was this rule copied and pasted from somewhere? We don’t just want another copy.
> 
> I’d like to see these checks about when it’s OK to set the cursor bundled up into a FrameView member function called shouldSetCursor or something like that. I think I’d include the deviceSupportsMouse check there along with these. I hope this would cut down on code duplication; not sure where else these checks already exist and I am not easily able to grep the code at this moment.

That was also in the fake mousemove event and there it is not only about setting the cursor, but it will also prevent from dispatching the mousemove event. It would be wrong calling it shouldSetCursor in that context.
That line can be simplified to: if (!m_frame->isActive() || !m_frame->page()->isOnscreen())

>> Source/WebCore/page/EventHandler.cpp:1251
>> +    PlatformKeyboardEvent::getCurrentModifierState(shiftKey, ctrlKey, altKey, metaKey);
> 
> It’s not correct to combine the shift key state from the time of cursor adjustment with the mouse position from m_lastKnownMousePosition. Since we need shift key state to select the cursor, then we need to store the shift key state along with the mouse position at the time the mouse position is stored.

You are wrong here.
m_lastKnownMousePosition is changed whenever mouse moves. But updateCursor() is called even when mouse didn't move. Using old shift state would be wrong there, because the state might have changed even when mouse didn't move. So we have to use the last known mouse position in combination with the last known shift state.

>> Source/WebCore/page/EventHandler.cpp:1253
>> +    m_frame->document()->updateLayoutIgnorePendingStylesheets();
> 
> This is wrong. A plain updateLayout would be OK, but there’s no reason for us to force a flash of unstyled content here, which is what updateLayoutIgnorePendingStylesheets does.

Then I'd say the idea of updating layout before cursor change should belong in another bug. Because mousemove also doesn't care about layout being pending before calling selectCursor.
Let's try to focus on the main idea of this bug/patch, OK?

>> Source/WebCore/page/EventHandler.cpp:1269
>> +        m_cursorUpdateTimer.stop();
> 
> We can just call stop unconditionally. No real reason to check isActive. Some other call sites might do it, but they are either incorrect, or trying to optimize.
> 
> This is not the correct place to stop the timer. We want to stop the timer when we set the cursor, not when we decide what to do set it to. Adding a side effect to a function that is supposed to tell us what cursor to use is not good design. This might require a little refactoring of the selectCursor call sites; maybe we can combine the selectCursor, setCursor, setting m_currentMouseCursor, and stopping the m_cursorUpdateTimer and use that function in all the appropriate places.

Will move it to handleMouseMoveEvent() right where hover and fake mousemove timers are also stopped.

>> Source/WebCore/page/EventHandler.cpp:1291
>> +    RenderObject* renderer = node->renderer();
> 
> We added a check for a text node. Does this fix a bug? Where is the test case for the bug it fixes? This fix should go in separately rather than being coupled with the rest of the change, or rolled out. The change log makes no mention of this.

It wasn't added, it's just moved up here as part of refactoring the method to not take a full MouseEvent, but work on HitTestResult instead.

>> Source/WebCore/page/EventHandler.cpp:1356
>> +        if (useHandCursor(node, result.URLElement() && result.URLElement()->isLink(), shiftKey))
> 
> It’s not good that to copy the body of the isOverLink function from MouseEventWithHitTestResults here, even though it’s simple. We don’t just want to copy code; we want to reuse it. If we have to make a fix we don’t want to have to fix it in two places. We could move the isOverLink function into HitTestResult, then could use it here.

Will move to HitTestResult.

>> Source/WebCore/page/EventHandler.cpp:1366
>> +        if ((editable || (originalIsText && node->canStartSelection())) && !inResizer && !result.scrollbar())
> 
> Again, does this fix a bug? Please don’t mix this in with your patch unless it’s really new to your patch.

It's also just part of the refactor to work with HitTestResult. There's no change to the logic.
"renderer && renderer->isText()" is moved to the top of method and result.scrollbar() was previously passed in as a parameter to selectCursor()

>> LayoutTests/fast/events/mouse-cursor-no-mousemove.html:17
>> +var CURSOR_UPDATE_DELAY = 50;
> 
> Not good that this test races the cursor updating code. We should find a more reliable way to test this.

I don't think there is a better way with current cursor testing framework.
Cursor update could not be made synchronous and there's no event for cursor changes.

[Aivo Paas]

Created attachment 191721 [details]
Patch

[Aivo Paas]

Made the requested changes.
Also removed entry from test expectations. Maybe things have changed, if not, someone with a Mac should investigate it. Or will just have to add the expectation back before landing.

[Darin Adler]

(In reply to comment #117)
> >> Source/WebCore/ChangeLog:27
> >> +        (WebCore):
> > 
> > Please remove this bogus line.
> 
> Come on, those are added by the script and there's tons of lines like that committed.
> Will remove, but that's just trolling.

Absolutely not. Please don’t join in with low quality work just because other people have done it. The script is buggy, but the ChangeLog is communication with other real people.

I don’t appreciate you saying "just trolling" when I am taking time out of my day to help you with your patch.

[Darin Adler]

Comment on attachment 191550 [details]
Patch

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

>>> Source/WebCore/page/EventHandler.cpp:1238
>>> +        return;
>> 
>> Why don’t I see deletion of old code that checked deviceSupportsMouse? If this was copied and pasted from somewhere, we need to refactor so we share that code instead of copying it.
>> 
>> Not new to this patch, but it’s crazy that this is a “setting”. That’s not the right way to expose something like that to WebKit.
> 
> Will remove that. It came from the fake mousemove event path. I'm not sure, but I think it was used in other places too, that's why I chose to keep it.

Removing it is not the right way to respond to my comment. We should refactor so the code is shared, not create differences.

>>> Source/WebCore/page/EventHandler.cpp:1245
>>> +        return;
>> 
>> Again, was this rule copied and pasted from somewhere? We don’t just want another copy.
>> 
>> I’d like to see these checks about when it’s OK to set the cursor bundled up into a FrameView member function called shouldSetCursor or something like that. I think I’d include the deviceSupportsMouse check there along with these. I hope this would cut down on code duplication; not sure where else these checks already exist and I am not easily able to grep the code at this moment.
> 
> That was also in the fake mousemove event and there it is not only about setting the cursor, but it will also prevent from dispatching the mousemove event. It would be wrong calling it shouldSetCursor in that context.
> That line can be simplified to: if (!m_frame->isActive() || !m_frame->page()->isOnscreen())

Find the right name and make the code shared, please.

>>> Source/WebCore/page/EventHandler.cpp:1251
>>> +    PlatformKeyboardEvent::getCurrentModifierState(shiftKey, ctrlKey, altKey, metaKey);
>> 
>> It’s not correct to combine the shift key state from the time of cursor adjustment with the mouse position from m_lastKnownMousePosition. Since we need shift key state to select the cursor, then we need to store the shift key state along with the mouse position at the time the mouse position is stored.
> 
> You are wrong here.
> m_lastKnownMousePosition is changed whenever mouse moves. But updateCursor() is called even when mouse didn't move. Using old shift state would be wrong there, because the state might have changed even when mouse didn't move. So we have to use the last known mouse position in combination with the last known shift state.

You say that I am wrong, but this could cause us to pair an incorrect combination of mouse position and modifier state. PlatformKeyboardEvent::getCurrentModifierState should be deprecated. We get modifier state from the event stream, not by querying explicitly at various times.

On the Mac platform, at least, we go through the same “fake mouse move” code path when modifier keys like shift change as well as when mouse position changes. Please take my suggestion and keep the two in sync.

>>> Source/WebCore/page/EventHandler.cpp:1253
>>> +    m_frame->document()->updateLayoutIgnorePendingStylesheets();
>> 
>> This is wrong. A plain updateLayout would be OK, but there’s no reason for us to force a flash of unstyled content here, which is what updateLayoutIgnorePendingStylesheets does.
> 
> Then I'd say the idea of updating layout before cursor change should belong in another bug. Because mousemove also doesn't care about layout being pending before calling selectCursor.
> Let's try to focus on the main idea of this bug/patch, OK?

Put in the correct function call, not the incorrect one. Please call updateLayout, not updateLayoutIgnorePendingStylesheets. You just used the wrong function,

Lets try to focus on getting good code in WebKit, OK?

Aivo, please improve your attitude! You keep telling me what to do.

>>> Source/WebCore/page/EventHandler.cpp:1366
>>> +        if ((editable || (originalIsText && node->canStartSelection())) && !inResizer && !result.scrollbar())
>> 
>> Again, does this fix a bug? Please don’t mix this in with your patch unless it’s really new to your patch.
> 
> It's also just part of the refactor to work with HitTestResult. There's no change to the logic.
> "renderer && renderer->isText()" is moved to the top of method and result.scrollbar() was previously passed in as a parameter to selectCursor()

You say no change to the logic, but the function now calls parentNode, and there is no call into parentNode in the old function. That’s a change to the logic. Perhaps a correct one, but you need to explain that it’s OK.

[Darin Adler]

Comment on attachment 191721 [details]
Patch

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

Seems OK. A few things that I still think need improvement, but good enough to land.

> Source/WebCore/page/EventHandler.cpp:1254
> +    bool shiftKey;
> +    bool ctrlKey;
> +    bool altKey;
> +    bool metaKey;
> +    PlatformKeyboardEvent::getCurrentModifierState(shiftKey, ctrlKey, altKey, metaKey);
> +
> +    HitTestRequest request(HitTestRequest::ReadOnly);
> +    HitTestResult result(view->windowToContents(m_lastKnownMousePosition));

As I mentioned in my previous comment, conceptually both mouse position changes and modifier key states come in to the event handler as a stream. It’s a mistake to pair a saved mouse position with a freshly obtained shift key state.

> Source/WebCore/page/EventHandler.cpp:1280
> +    bool originalIsText = node->isTextNode();

I think a better name would be originalNodeWasText.

> Source/WebCore/page/EventHandler.cpp:1282
> +    if (node && originalIsText)
> +        node = node->parentNode();

This call to parentNode is new code. Why does this change need to be made? Does it improve something?

> Source/WebCore/page/FrameView.h:343
> +    virtual bool shouldSetCursor() const;

Why virtual?

[Aivo Paas]

Sorry, I did not mean to offend you. It's just that many of the weaknesses you pointed out, were not my original ideas, but suggestions by others or based on the code that was already there. Not the warmest welcome a first time helping hand would hope for.

> > Source/WebCore/page/EventHandler.cpp:1282
> > +    if (node && originalIsText)
> > +        node = node->parentNode();
> 
> This call to parentNode is new code. Why does this change need to be made? Does it improve something?

I completely forgot about that. It was added when the timer was not used and instead hovered state was checked to only call updateCursor() on hovered nodes. Now that this is reverted and timer still used, this check is no longer needed. I will clean it up and then the refactored code should look almost exactly like the original.

I'll also go over the other other bits that were pointed out.

[Aivo Paas]

Created attachment 191828 [details]
Patch

[Build Bot]

Comment on attachment 191828 [details]
Patch

Attachment 191828 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17087088

New failing tests:
fast/events/mouse-cursor-change.html
editing/selection/selection-modify-crash.html

[Aivo Paas]

Created attachment 192249 [details]
Patch

[Aivo Paas]

Mac still fails the new cursor change test.
Added back the test expectation, no other changes compared to the previous upload. It should now pass all EWS.
Have tried to find someone to look at the test on a mac but no luck so far and I simply don't have the time to keep on begging for help.

I think I'm done for now, unless there's still something to fix before landing the patch.

[Aivo Paas]

Ping for review + land

[Aivo Paas]

What should be done to get some attention?
it's been 3 weeks...

[Alexey Proskuryakov]

*** Bug 113920 has been marked as a duplicate of this bug. ***

[Allan Sandfeld Jensen]

Comment on attachment 192249 [details]
Patch

Based on Darin Addler's comment of being good enough to land, and my own review. I will dare r+ it once more.

[WebKit Commit Bot]

Comment on attachment 192249 [details]
Patch

Rejecting attachment 192249 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 192249, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
or-change.html
patching file LayoutTests/fast/events/mouse-cursor-no-mousemove-expected.txt
patching file LayoutTests/fast/events/mouse-cursor-no-mousemove.html
patching file LayoutTests/platform/mac/TestExpectations
Hunk #1 FAILED at 1460.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/mac/TestExpectations.rej

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Allan Sandfeld Jensen']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-commit-queue.appspot.com/results/17420171

[Aivo Paas]

Created attachment 196614 [details]
Patch

[Aivo Paas]

Just rebased TestExpectations.
Probably should wait for green EWS because it's been so long...

[WebKit Commit Bot]

Comment on attachment 196614 [details]
Patch

Clearing flags on attachment: 196614

Committed r147739: <http://trac.webkit.org/changeset/147739>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

Comment on attachment 196614 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:1390
> +                    inResizer = layer->isPointInResizeControl(view->windowToContents(roundedIntPoint(result.localPoint())));

This was wrong. Bug 210778.