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.
Created attachment 68425 [details] Added required functions.
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.
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.
Created attachment 69420 [details] Fixed style error
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?
(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.
(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.
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?
Created attachment 70037 [details] Addressed reviewer's comments
(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.
(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
Created attachment 70306 [details] Minor changes to address reviewer's comments
(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
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>
All reviewed patches have been landed. Closing bug.