[Webkit-unassigned] [Bug 101857] Updating mouse cursor on style changes without emitting fake mousemove event

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=101857





--- Comment #117 from Aivo Paas <aivopaas at gmail.com>  2013-03-06 01:59:48 PST ---
(From update of attachment 191550)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.