Bug 42922 – [chromium] WebViewClient should have an elementClicked() method

• Component: WebKit Misc.
• Status: RESOLVED WONTFIX

[Jay Civelli]

In order to factor out the autofill client from WebKit to Chromium, Chromium needs an easy way of knowing an input element has been clicked and whether or not that element had focus before it was clicked.
This is so autofill suggestion popups can be shown when an input element is clicked.

[Jay Civelli]

Created attachment 62480 [details]
Initial patch

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/public/WebViewClient.h:153:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
WebKit/chromium/public/WebViewClient.h:153:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/chromium/public/WebViewClient.h:154:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/chromium/public/WebViewClient.h:155:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5 in 3 files


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

[Jay Civelli]

Created attachment 62481 [details]
Fixed style.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/public/WebViewClient.h:153:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 2 in 3 files


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

[James Hawkins]

LGTM, minus style fixes.

[Jay Civelli]

Created attachment 62525 [details]
Really fixing style

[Darin Fisher (:fishd, Google)]

Comment on attachment 62525 [details]
Really fixing style

WebKit/chromium/src/WebViewImpl.cpp:386
 +      if (!clickedElement.isNull()) {
are you sure you want to run this callback right in the middle of
the mouseDown handler?  could that result in some interesting
re-entrancy bugs?  suppose the inputElementClicked method did
something to change focus.  could that be bad?  i see the code
following this checking focusedWebCoreNode.

WebKit/chromium/public/WebViewClient.h:155
 +      virtual void inputElementClicked(const WebInputElement& element, bool alreadyFocused) { }
style-nit: no need for the "element" parameter name since it does not add
extra info.



WebKit/chromium/public/WebViewClient.h:155
 +      virtual void inputElementClicked(const WebInputElement& element, bool alreadyFocused) { }
this name is a bit confusing.  based on the implementation, it seems that
this will happen whenever a mousedown event happens to result in the
focused node being an <input> element.  it does not necessarily mean
that the mousedown event was dispatched to an <input> element.  couldn't
i write an event handler that focused an <input> element during event
processing?  that would screw up this implementation, right?

if you really want to know if a "click" event was dispatched to an
<input> element, then I think you need to somehow hook into the
defaultEventHandler for the HTMLInputElement.  also, there is a
difference between the "click" event and the "mousedown" event.
i can't tell if that distinction matters to you.

[Darin Fisher (:fishd, Google)]

For reference, the "click" event is defined here:
http://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-MouseEvent

Notice that a "click" corresponds to a mousedown followed by a mouseup.

Have you considered just using an event listener?

[Jay Civelli]

@fishd
Sorry my patch was wrong. I meant to have clickedElement be the clickedNode, not the focused element.

As you suggest, I could probably use an event listener from the Chromium side instead.
I could add an event listener for mousedown on the document in render_view.cc
I have to use mousedown instead of click because by the time I get notified of the click, the mousedown event already focused the field and I have no way of knowing if the input field was focused before it was clicked (I need to know that so we can have the autofill behavior where clicking a focused input shows the autofill popup).

I am not sure there is a reentrency problem in that case, but I can see how some other listeners might change the focus. In that case we probably would not want to notify for autofill purposes. I could post a task so to process the notification and check which element is focused after all listeners have returned (once the stack has unwinded).

Do you think it makes sense?

[Jay Civelli]

Addressed on the Chromium side.