Bug 111120 – [chromium] Add Webkit API to comminicate the state of touch editing handles with the embedder.

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[Varun Jain]

[chromium] Add Webkit API to comminicate the state of touch editing handles with the embedder.

[Varun Jain]

Created attachment 190821 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[James Robinson]

Could you put the motivation for this patch in the ChangeLog?

[Varun Jain]

Created attachment 190825 [details]
Patch

[Varun Jain]

(In reply to comment #3)
> Could you put the motivation for this patch in the ChangeLog?

Done.

[James Robinson]

Comment on attachment 190825 [details]
Patch

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

> Source/WebKit/chromium/ChangeLog:9
> +        1. WebView::updateTouchEditing: In the current form, WebCore notifies the embedder (through
> +        WebViewClient::didChangeSelection) of any changes in selection on the webpage. Since we intend
> +        to draw the selection handles in WebKit, we need this API that the embedder can use to inform
> +        WebKit of this selection change so that the handles can be updated.

Are you saying that currently the notion of the current selection can be out of sync between WebKit and the embedder?  In what circumstances is this the case?  Is this touch-specific?

> Source/WebKit/chromium/ChangeLog:12
> +        2. WebViewClient::didChangeTouchEditingHandleVisibility: This is to notify the embedder when
> +        touch editing handles are shown/hidden to give it a chance to update its state and any ui
> +        components it may be using for touch editing.

Could you be more specific?  What sorts of things would the embedder do differently when the handles are added or removed?

[Varun Jain]

Comment on attachment 190825 [details]
Patch

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

>> Source/WebKit/chromium/ChangeLog:9
>> +        WebKit of this selection change so that the handles can be updated.
> 
> Are you saying that currently the notion of the current selection can be out of sync between WebKit and the embedder?  In what circumstances is this the case?  Is this touch-specific?

I think I found a way to avoid this API!! It is very simply really and I dont know why I did not see it before :(
the didChangeSelection is called by EditorClientImpl and I can just hook the updateTouchEditing call there without exposing it in the API.

>> Source/WebKit/chromium/ChangeLog:12
>> +        components it may be using for touch editing.
> 
> Could you be more specific?  What sorts of things would the embedder do differently when the handles are added or removed?

In the current UI spec I have, the embedder shows a small (Cut|Copy|Paste|...) widget when the handles are visible and removes it when they go away.

[Varun Jain]

Created attachment 191274 [details]
Patch

[Varun Jain]

Hi James, I have removed the WebView API that I realized was not required. I am still waiting on your thoughts about my reply to your question about the second API.

[James Robinson]

Comment on attachment 191274 [details]
Patch

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

Maybe Darin would be able to give some advice on the placement of this.  I can't quite wrap my head around the intended placement of selection/editing related APIs.  There are several selection/caret related APIs on WebWidget, WebView, WebViewClient, and WebFrame.  It's not clear to me, for instance, if we support selections on any sort of WebWidget or just WebViews.

It also seems a bit strange to have UI that varies based on whether touch handles are present or not, but I guess that's more of a UI question.

> Source/WebKit/chromium/ChangeLog:12
> +        [chromium] Add Webkit API to comminicate the state of touch editing handles with the embedder.
> +        This is part of the effort to draw touch editing handles in WebKit so that they can be in
> +        sync with the rest of the webpage. To be able to do this, we need the following API:
> +        WebViewClient::didChangeTouchEditingHandleVisibility: This is to notify the embedder when
> +        touch editing handles are shown/hidden to give it a chance to update its state and any ui
> +        components it may be using for touch editing (such as Cut|Copy|Paste menus that may be
> +        displayed when the user is selecting text using the touch handles).
> +        https://bugs.webkit.org/show_bug.cgi?id=111120
> +
> +        Reviewed by NOBODY (OOPS!).

The way this is normally formatted is:

[chromium] One-line title
https://link.to.bug/
<blank line>
Reviewed by ...
<blank line>
Long description of patch that spans
multiple lines


Please follow that here.

[Varun Jain]

Created attachment 191278 [details]
Patch

[Varun Jain]

(In reply to comment #10)
> (From update of attachment 191274 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191274&action=review
> 
> Maybe Darin would be able to give some advice on the placement of this.  I can't quite wrap my head around the intended placement of selection/editing related APIs.  There are several selection/caret related APIs on WebWidget, WebView, WebViewClient, and WebFrame.  It's not clear to me, for instance, if we support selections on any sort of WebWidget or just WebViews.
> 

I think this is no different (in terms of placement) than the WebViewClient::didChangeSelection method. It is also very similar in behavior (unfortunately not exactly the same). I would argue that since this API is only needed for widgets that draw their own selection handles it should be limited to WebView and not WebWidget since only WebViewImpl currently has (will have after my patches are all done) ability to draw touch handles. A Widget holding a plugin or a popup menu will, for instance, have no use for this API.

> It also seems a bit strange to have UI that varies based on whether touch handles are present or not, but I guess that's more of a UI question.
> 

this is the spec I got from UX. But I think this is pretty common UX for touch selection on many tablet devices

> > Source/WebKit/chromium/ChangeLog:12
> > +        [chromium] Add Webkit API to comminicate the state of touch editing handles with the embedder.
> > +        This is part of the effort to draw touch editing handles in WebKit so that they can be in
> > +        sync with the rest of the webpage. To be able to do this, we need the following API:
> > +        WebViewClient::didChangeTouchEditingHandleVisibility: This is to notify the embedder when
> > +        touch editing handles are shown/hidden to give it a chance to update its state and any ui
> > +        components it may be using for touch editing (such as Cut|Copy|Paste menus that may be
> > +        displayed when the user is selecting text using the touch handles).
> > +        https://bugs.webkit.org/show_bug.cgi?id=111120
> > +
> > +        Reviewed by NOBODY (OOPS!).
> 
> The way this is normally formatted is:
> 
> [chromium] One-line title
> https://link.to.bug/
> <blank line>
> Reviewed by ...
> <blank line>
> Long description of patch that spans
> multiple lines
> 
> 
> Please follow that here.

Done.

[James Robinson]

(In reply to comment #12)
> (In reply to comment #10)
> > (From update of attachment 191274 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=191274&action=review
> > 
> > Maybe Darin would be able to give some advice on the placement of this.  I can't quite wrap my head around the intended placement of selection/editing related APIs.  There are several selection/caret related APIs on WebWidget, WebView, WebViewClient, and WebFrame.  It's not clear to me, for instance, if we support selections on any sort of WebWidget or just WebViews.
> > 
> 
> I think this is no different (in terms of placement) than the WebViewClient::didChangeSelection method. It is also very similar in behavior (unfortunately not exactly the same). I would argue that since this API is only needed for widgets that draw their own selection handles it should be limited to WebView and not WebWidget since only WebViewImpl currently has (will have after my patches are all done) ability to draw touch handles. A Widget holding a plugin or a popup menu will, for instance, have no use for this API.

OK, so your assertion is we only support selection on WebViews and not other WebWidgets?  We do support text input on other Widgets (or at least it appears we do given the composition and selection APIs on WebWidget), so it seems they would need this as well.  But I'm not sure on this point.

[Varun Jain]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #10)
> > > (From update of attachment 191274 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=191274&action=review
> > > 
> > > Maybe Darin would be able to give some advice on the placement of this.  I can't quite wrap my head around the intended placement of selection/editing related APIs.  There are several selection/caret related APIs on WebWidget, WebView, WebViewClient, and WebFrame.  It's not clear to me, for instance, if we support selections on any sort of WebWidget or just WebViews.
> > > 
> > 
> > I think this is no different (in terms of placement) than the WebViewClient::didChangeSelection method. It is also very similar in behavior (unfortunately not exactly the same). I would argue that since this API is only needed for widgets that draw their own selection handles it should be limited to WebView and not WebWidget since only WebViewImpl currently has (will have after my patches are all done) ability to draw touch handles. A Widget holding a plugin or a popup menu will, for instance, have no use for this API.
> 
> OK, so your assertion is we only support selection on WebViews and not other WebWidgets?  We do support text input on other Widgets (or at least it appears we do given the composition and selection APIs on WebWidget), so it seems they would need this as well.  But I'm not sure on this point.

Sorry for not being clear. I only asserted that only a WebView currently draws its own selection handles and hence needs this API which is specific to the visibility of the handles and not the selection.
I do see the plethora of selection APIs on WebWidget, although they seem to be implemented only in WebViewImpl (ignoring no-op implementations).

[Varun Jain]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > (In reply to comment #10)
> > > > (From update of attachment 191274 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=191274&action=review
> > > > 
> > > > Maybe Darin would be able to give some advice on the placement of this.  I can't quite wrap my head around the intended placement of selection/editing related APIs.  There are several selection/caret related APIs on WebWidget, WebView, WebViewClient, and WebFrame.  It's not clear to me, for instance, if we support selections on any sort of WebWidget or just WebViews.
> > > > 
> > > 
> > > I think this is no different (in terms of placement) than the WebViewClient::didChangeSelection method. It is also very similar in behavior (unfortunately not exactly the same). I would argue that since this API is only needed for widgets that draw their own selection handles it should be limited to WebView and not WebWidget since only WebViewImpl currently has (will have after my patches are all done) ability to draw touch handles. A Widget holding a plugin or a popup menu will, for instance, have no use for this API.
> > 
> > OK, so your assertion is we only support selection on WebViews and not other WebWidgets?  We do support text input on other Widgets (or at least it appears we do given the composition and selection APIs on WebWidget), so it seems they would need this as well.  But I'm not sure on this point.
> 
> Sorry for not being clear. I only asserted that only a WebView currently draws its own selection handles and hence needs this API which is specific to the visibility of the handles and not the selection.
> I do see the plethora of selection APIs on WebWidget, although they seem to be implemented only in WebViewImpl (ignoring no-op implementations).

That said, if you are more comfortable with moving this method to WebWidgetClient I would be happy to do so.

[Darin Fisher (:fishd, Google)]

Comment on attachment 191278 [details]
Patch

What does the caller of this code look like?

[Varun Jain]

Created attachment 192066 [details]
Patch

[Varun Jain]

(In reply to comment #16)
> (From update of attachment 191278 [details])
> What does the caller of this code look like?

Hi Darin... I have changed the API as we had discussed over IM. PTAL.