Bug 46296 – Add more functionality in WebView interface for currently focused elements

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Varun Jain]

Adding two methods to the WebView interface:
1. method to check if currently focused node is a textfield or
other types of input element. This is required by the renderer
to take appropriate action. For example, on touch devices, if the
focused node is a textfield, we may want to invoke an on-screen
keyboard.
2. method to inform the renderer to scroll the currently focused
element into view, for instance, when it is hidden due to window
resizing.

[Varun Jain]

Created attachment 68425 [details]
Added required functions.

[Varun Jain]

Created attachment 69397 [details]
Addressed reviewer's comments

As pointed out by the reviewer in an offline conversation, the method isInputElement that I had included in the WebView interface in my previous patch is redundant since the same information is available from the WebNode object. In this patch, I have removed that method and added the necessary methods to WebNode.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/src/WebNode.cpp:156:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 6 files


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

[Varun Jain]

Created attachment 69420 [details]
Fixed style error

[James Robinson]

First off, WebKit API reviews should go through Darin Fisher.  I noticed he wasn't even on the CC list which is probably why this hasn't been reviewed yet.

Second, what is this for?

[Darin Fisher (:fishd, Google)]

(In reply to comment #0)
> Adding two methods to the WebView interface:
> 1. method to check if currently focused node is a textfield or
> other types of input element. This is required by the renderer
> to take appropriate action. For example, on touch devices, if the
> focused node is a textfield, we may want to invoke an on-screen
> keyboard.

It sounds like this should be accomplished by observing a client notification from WebKit.  WebViewClient already has a textFieldDidBeginEditing notification.  Is that not sufficient?


> 2. method to inform the renderer to scroll the currently focused
> element into view, for instance, when it is hidden due to window
> resizing.

This seems like a reasonable addition.

[Varun Jain]

(In reply to comment #6)
> (In reply to comment #0)
> > Adding two methods to the WebView interface:
> > 1. method to check if currently focused node is a textfield or
> > other types of input element. This is required by the renderer
> > to take appropriate action. For example, on touch devices, if the
> > focused node is a textfield, we may want to invoke an on-screen
> > keyboard.
> 
> It sounds like this should be accomplished by observing a client notification from WebKit.  WebViewClient already has a textFieldDidBeginEditing notification.  Is that not sufficient?

In my latest patch, I am accomplishing this by observing client notification in RenderView::focusedNodeChanged. This change is in the related Chromium CL that you can see here: http://codereview.chromium.org/3474007

However, to take into account all text-input elements, I need the API extention to WebNode to determine if the newly focused node is a textfield, textarea or content editable div.

the notification that you have suggested (textFieldDidBeginEditing) seems to be textfield specific as it is defined to work with HTMLInputElement. It will require significant API changes to both WebInputElement and WebViewImpl to make it work for textareas and content editable divs. In contrast, my change adds a couple of methods to WebNode which expose a little more information about the underlying WebCore::Node (which should have been exposed anyways). Is there a specific reason why you would not want those two methods in WebNode?



> 
> 
> > 2. method to inform the renderer to scroll the currently focused
> > element into view, for instance, when it is hidden due to window
> > resizing.
> 
> This seems like a reasonable addition.

[Darin Fisher (:fishd, Google)]

Comment on attachment 69420 [details]
Fixed style error

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

> WebKit/chromium/src/WebNode.cpp:157
> +        return element && element->isTextFormControl();

i think you should move this method to WebElement.  then the code to
determine if a WebNode is a WebElement would move to the embedder.
see also WebElement::isFormControlElement.

i think this new method should be named isTextFormControlElement.

actually, we already have WebInputElement::isTextField.  can you
use that?

[Varun Jain]

Created attachment 70037 [details]
Addressed reviewer's comments

[Varun Jain]

(In reply to comment #8)
> (From update of attachment 69420 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69420&action=review
> 
> > WebKit/chromium/src/WebNode.cpp:157
> > +        return element && element->isTextFormControl();
> 
> i think you should move this method to WebElement.  then the code to
> determine if a WebNode is a WebElement would move to the embedder.
> see also WebElement::isFormControlElement.

agreed. moved the method to WebElement and changed the chrome CL to incorporate the check for WebNode vs. WebElement.
Also, I checked and looks like WebElement::isFormControlElement is too generic for my cause. I only need textfields and textareas which is served by isTextFormControl()

> 
> i think this new method should be named isTextFormControlElement.

The method in the underlying WebCore::Element is called isTextFormControl(). I think it would be better to keep the naming consistent in the wrapper.

> 
> actually, we already have WebInputElement::isTextField.  can you
> use that?

As I mentioned earlier, WebInputElement is textfield specific.

[Darin Fisher (:fishd, Google)]

(In reply to comment #10)
> (In reply to comment #8)
> > (From update of attachment 69420 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=69420&action=review
> > 
> > > WebKit/chromium/src/WebNode.cpp:157
> > > +        return element && element->isTextFormControl();
> > 
> > i think you should move this method to WebElement.  then the code to
> > determine if a WebNode is a WebElement would move to the embedder.
> > see also WebElement::isFormControlElement.
> 
> agreed. moved the method to WebElement and changed the chrome CL to incorporate the check for WebNode vs. WebElement.
> Also, I checked and looks like WebElement::isFormControlElement is too generic for my cause. I only need textfields and textareas which is served by isTextFormControl()

OK

> 
> > 
> > i think this new method should be named isTextFormControlElement.
> 
> The method in the underlying WebCore::Element is called isTextFormControl(). I think it would be better to keep the naming consistent in the wrapper.

Sorry to bikeshed on this point, but I think it is more important for the public facing API to be consistent.  That's why I said to use the Element suffix.


> 
> > 
> > actually, we already have WebInputElement::isTextField.  can you
> > use that?
> 
> As I mentioned earlier, WebInputElement is textfield specific.

OK

[Varun Jain]

Created attachment 70306 [details]
Minor changes to address reviewer's comments

[Varun Jain]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #8)
> > > (From update of attachment 69420 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=69420&action=review
> > > 
> > > > WebKit/chromium/src/WebNode.cpp:157
> > > > +        return element && element->isTextFormControl();
> > > 
> > > i think you should move this method to WebElement.  then the code to
> > > determine if a WebNode is a WebElement would move to the embedder.
> > > see also WebElement::isFormControlElement.
> > 
> > agreed. moved the method to WebElement and changed the chrome CL to incorporate the check for WebNode vs. WebElement.
> > Also, I checked and looks like WebElement::isFormControlElement is too generic for my cause. I only need textfields and textareas which is served by isTextFormControl()
> 
> OK
> 
> > 
> > > 
> > > i think this new method should be named isTextFormControlElement.
> > 
> > The method in the underlying WebCore::Element is called isTextFormControl(). I think it would be better to keep the naming consistent in the wrapper.
> 
> Sorry to bikeshed on this point, but I think it is more important for the public facing API to be consistent.  That's why I said to use the Element suffix.

Done.

> 
> 
> > 
> > > 
> > > actually, we already have WebInputElement::isTextField.  can you
> > > use that?
> > 
> > As I mentioned earlier, WebInputElement is textfield specific.
> 
> OK

[WebKit Commit Bot]

Comment on attachment 70306 [details]
Minor changes to address reviewer's comments

Clearing flags on attachment: 70306

Committed r69459: <http://trac.webkit.org/changeset/69459>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.