[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 #121 from Darin Adler <darin at apple.com>  2013-03-06 09:29:37 PST ---
(From update of attachment 191550)
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.

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