Bug 51228 – WebKit2: Clicking in a selected text field does not clear the selection and set the caret (because WebKit2 doesn't set PlatformMouseEvent::m_activatedWebView?)

• Component: WebKit2
• Status: RESOLVED FIXED

[Adam Roben (:aroben)]

To reproduce:

1. Go to data:text/html,<input value=abcdefghijkl>
2. Select the text in the text field
3. Click in the middle of the selected text

The text remains selected, and no caret is inserted.

[Adam Roben (:aroben)]

I believe this is because WebKit2 isn't setting PlatformMouseEvent::m_activatedWebView.

Classic WebKit sets PlatformMouseEvent::m_activatedWebView to true when this is the first mouse event since we received WM_MOUSEACTIVATE, and to false otherwise. WebKit2 doesn't set this flag at all; it just uses PlatformMouseEvent's default constructor (which leaves this member completely uninitialized!). This can lead to selection problems, as this member is used to decide whether to reset selection or not.

[Adam Roben (:aroben)]

<rdar://problem/8780985>

[Jeff Miller]

Created attachment 77897 [details]
Patch to plumb through whether the click activated the WebView to match WebKit1 behavior.

Patch to plumb through whether the click activated the WebView to match WebKit1 behavior is attached.

[WebKit Review Bot]

Attachment 77897 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/platform/PlatformMouseEvent.h', u'WebKit2/ChangeLog', u'WebKit2/Shared/WebEvent.h', u'WebKit2/Shared/WebEventConversion.cpp', u'WebKit2/Shared/WebMouseEvent.cpp', u'WebKit2/Shared/win/WebEventFactory.cpp', u'WebKit2/Shared/win/WebEventFactory.h', u'WebKit2/UIProcess/win/WebView.cpp', u'WebKit2/UIProcess/win/WebView.h']" exit_code: 1
WebKit2/Shared/win/WebEventFactory.h:38:  The parameter name "hwnd" adds no information, so it should be removed.  [readability/parameter_name] [5]
WebKit2/Shared/win/WebEventFactory.h:38:  The parameter name "wparam" adds no information, so it should be removed.  [readability/parameter_name] [5]
WebKit2/Shared/win/WebEventFactory.h:38:  The parameter name "lparam" adds no information, so it should be removed.  [readability/parameter_name] [5]
WebCore/platform/PlatformMouseEvent.h:89:  Missing space after ,  [whitespace/comma] [3]
WebCore/platform/PlatformMouseEvent.h:111:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 5 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Roben (:aroben)]

Comment on attachment 77897 [details]
Patch to plumb through whether the click activated the WebView to match WebKit1 behavior.

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

It's unfortunate to have so many Windows-specific #ifdefs in here. Maybe if we unified the way the various platforms represent this information in WebCore the patch would be cleaner. Surprisingly, it looks like only Mac and Windows implement EventHandler::eventActivatedView.

It would be good to fix PlatformMouseEvent's default constructor to initialize m_activatedWebView, too, but that could be done in a separate patch.

I think Sam or Anders should give this a once-over.

> WebCore/platform/PlatformMouseEvent.h:89
> +            ,m_eventNumber(0)

Missing space here.

> WebCore/platform/PlatformMouseEvent.h:91
> +#endif
> +#if PLATFORM(WIN)

Maybe #elif would be better.

> WebCore/platform/PlatformMouseEvent.h:113
> +#if PLATFORM(MAC)
> +            ,m_eventNumber(0)
> +#endif
> +#if PLATFORM(WIN)

Same comments here.

[Jeff Miller]

Created attachment 77906 [details]
Patch to plumb through whether the click activated the WebView to match WebKit1 behavior.

Attached new patch to fix WebKit code style issues.

[Jeff Miller]

(In reply to comment #5)
> (From update of attachment 77897 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77897&action=review
> 
> It's unfortunate to have so many Windows-specific #ifdefs in here. Maybe if we unified the way the various platforms represent this information in WebCore the patch would be cleaner. Surprisingly, it looks like only Mac and Windows implement EventHandler::eventActivatedView.

I wasn't sure what the design philosophy should be here.  Obviously, it wouldn't hurt to have the platform-specific member variables be in all platforms, their values just wouldn't be used.

> 
> It would be good to fix PlatformMouseEvent's default constructor to initialize m_activatedWebView, too, but that could be done in a separate patch.
> 
> I think Sam or Anders should give this a once-over.
> 
> > WebCore/platform/PlatformMouseEvent.h:89
> > +            ,m_eventNumber(0)
> 
> Missing space here.

I uploaded a new patch to fix the code-style warnings.

> 
> > WebCore/platform/PlatformMouseEvent.h:91
> > +#endif
> > +#if PLATFORM(WIN)
> 
> Maybe #elif would be better.
> 
> > WebCore/platform/PlatformMouseEvent.h:113
> > +#if PLATFORM(MAC)
> > +            ,m_eventNumber(0)
> > +#endif
> > +#if PLATFORM(WIN)
> 
> Same comments here.

OK, I'll attach a new patch with these changes soon.

[Jeff Miller]

(In reply to comment #5)
> It would be good to fix PlatformMouseEvent's default constructor to initialize m_activatedWebView, too, but that could be done in a separate patch.

Could you be more specific?  I did change the default constructor to initialize m_activatedWebView, unless you're talking about a different method.

[Adam Roben (:aroben)]

(In reply to comment #8)
> (In reply to comment #5)
> > It would be good to fix PlatformMouseEvent's default constructor to initialize m_activatedWebView, too, but that could be done in a separate patch.
> 
> Could you be more specific?  I did change the default constructor to initialize m_activatedWebView, unless you're talking about a different method.

Sorry, I got confused. Thanks for fixing it!

[Jeff Miller]

Created attachment 77911 [details]
Patch to plumb through whether the click activated the WebView to match WebKit1 behavior.

New patch attached with fixes from Adam's review.

[Darin Adler]

Comment on attachment 77911 [details]
Patch to plumb through whether the click activated the WebView to match WebKit1 behavior.

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

The substance of this looks great. All my comments are about naming and minor style issues. So r=me but you might want to do some renames based on my suggestion; they are optional.

> WebKit2/Shared/WebEvent.h:115
>      WebMouseEvent(Type, Button, const WebCore::IntPoint& position, const WebCore::IntPoint& globalPosition, float deltaX, float deltaY, float deltaZ, int clickCount, Modifiers, double timestamp);
> +#if PLATFORM(WIN)
> +    WebMouseEvent(Type, Button, const WebCore::IntPoint& position, const WebCore::IntPoint& globalPosition, float deltaX, float deltaY, float deltaZ, int clickCount, Modifiers, double timestamp, bool activatedWebView);
> +#endif

It’s a little annoying to have all these #ifdefs. Is there any way to make this a platform-independent concept even if it’s not needed elsewhere? Is this really about HWND activation? Is there a similar concept on other platforms?

> WebKit2/Shared/WebEvent.h:142
> +    bool m_activatedWebView;

This name sounds like it could be a web view, rather than a boolean. Is there a less ambiguous alternate name we could come up with? I noticed the name comes from existing code, but I still don’t really like it.

Given the other code below, maybe the name for this is m_isMouseActivateEvent? Is there any need to mention WebView?

> WebKit2/Shared/win/WebEventFactory.h:40
> -    static WebMouseEvent createWebMouseEvent(HWND hwnd, UINT message, WPARAM wparam, LPARAM lparam);
> -    static WebWheelEvent createWebWheelEvent(HWND hwnd, UINT message, WPARAM wparam, LPARAM lparam);
> -    static WebKeyboardEvent createWebKeyboardEvent(HWND hwnd, UINT message, WPARAM wparam, LPARAM lparam);
> +    static WebMouseEvent createWebMouseEvent(HWND, UINT, WPARAM, LPARAM, bool);
> +    static WebWheelEvent createWebWheelEvent(HWND, UINT, WPARAM, LPARAM);
> +    static WebKeyboardEvent createWebKeyboardEvent(HWND, UINT, WPARAM, LPARAM);

While we do leave off argument names when the say nothing, I think the name “message” here added value and should not be removed, and the new bool definitely needs an argument name.

> WebKit2/UIProcess/win/WebView.h:149
> +    bool m_mouseActivated;

For booleans we try to use names that finish a noun or adjective phrase, like “web view [xxx]”. This one doesn’t. Because “web view mouse activated” doesn’t. Maybe the name m_wasActivatedByMouseEvent? Or m_isHandlingMouseActivateEvent?

[Jeff Miller]

(In reply to comment #11)
> It’s a little annoying to have all these #ifdefs. Is there any way to make this a platform-independent concept even if it’s not needed elsewhere? Is this really about HWND activation? Is there a similar concept on other platforms?

In comments to me outside of this bug, Anders indicated that he was OK with this being platform specific.  However, both you and Adam brought this up, and Adam did mention in comments above that both Mac and Windows implement EventHandler::eventActivatedView.  However, I haven't investigated this further, this is probably better looked at in a separate patch.

> 
> > WebKit2/Shared/WebEvent.h:142
> > +    bool m_activatedWebView;
> 
> This name sounds like it could be a web view, rather than a boolean. Is there a less ambiguous alternate name we could come up with? I noticed the name comes from existing code, but I still don’t really like it.
> 
> Given the other code below, maybe the name for this is m_isMouseActivateEvent? Is there any need to mention WebView?

Anders suggested m_didActivateWebView, which is what I'm going to use.
> 
> > WebKit2/Shared/win/WebEventFactory.h:40
> > -    static WebMouseEvent createWebMouseEvent(HWND hwnd, UINT message, WPARAM wparam, LPARAM lparam);
> > -    static WebWheelEvent createWebWheelEvent(HWND hwnd, UINT message, WPARAM wparam, LPARAM lparam);
> > -    static WebKeyboardEvent createWebKeyboardEvent(HWND hwnd, UINT message, WPARAM wparam, LPARAM lparam);
> > +    static WebMouseEvent createWebMouseEvent(HWND, UINT, WPARAM, LPARAM, bool);
> > +    static WebWheelEvent createWebWheelEvent(HWND, UINT, WPARAM, LPARAM);
> > +    static WebKeyboardEvent createWebKeyboardEvent(HWND, UINT, WPARAM, LPARAM);
> 
> While we do leave off argument names when the say nothing, I think the name “message” here added value and should not be removed, and the new bool definitely needs an argument name.

I will fix this.

> 
> > WebKit2/UIProcess/win/WebView.h:149
> > +    bool m_mouseActivated;
> 
> For booleans we try to use names that finish a noun or adjective phrase, like “web view [xxx]”. This one doesn’t. Because “web view mouse activated” doesn’t. Maybe the name m_wasActivatedByMouseEvent? Or m_isHandlingMouseActivateEvent?

I was copying the existing name used in WK1, but I agree.  I will change this m_wasActivatedByMouseEvent and setMouseActivated() to setWasActivatedByMouseEvent() in both the WK1 and WK2 versions.

New patch coming soon.

[Jeff Miller]

Created attachment 77931 [details]
Patch to plumb through whether the click activated the WebView to match WebKit1 behavior.

Updates based on Darin's comments.

[WebKit Commit Bot]

Comment on attachment 77931 [details]
Patch to plumb through whether the click activated the WebView to match WebKit1 behavior.

Clearing flags on attachment: 77931

Committed r75040: <http://trac.webkit.org/changeset/75040>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.