Bug 67569 – Rename confirmCompositionWithoutDisturbingSelection to cancelComposition

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

Per discussion on the bug 67377, we found that confirmCompositionWithoutDisturbingSelection is only used to cancel composition and the corresponding changeset http://trac.webkit.org/changeset/94497 made a change that enforces this notion.  We should rename this function to match the actual behavior now.

[Ryosuke Niwa]

Created attachment 106469 [details]
cleanup

[Kenneth Rohde Christiansen]

So what if I really want to confirm? We are depending a lot of this method in our slightly outdated branch as we commit the composition instead of cancelling it in many cases (ie. moving from one field to another etc)

[Antonio Gomes]

(In reply to comment #2)
> So what if I really want to confirm? We are depending a lot of this method in our slightly outdated branch as we commit the composition instead of cancelling it in many cases (ie. moving from one field to another etc)

r+ upon kenneth's concern is not an issue.

I was about to ask the same: if one want to cancel selection without actually disturbing selection.

[Kenneth Rohde Christiansen]

(In reply to comment #3)
> (In reply to comment #2)
> > So what if I really want to confirm? We are depending a lot of this method in our slightly outdated branch as we commit the composition instead of cancelling it in many cases (ie. moving from one field to another etc)
> 
> r+ upon kenneth's concern is not an issue.
> 
> I was about to ask the same: if one want to cancel selection without actually disturbing selection.

We kind of need that. That is how our mobile platform works. We commit when loosing focus (could be just moving the cursor position with a touch in the same input field) and before injecting compositions into our virtual keyboard (used when the user touches a word and it becomes a composition)

[Antonio Gomes]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > So what if I really want to confirm? We are depending a lot of this method in our slightly outdated branch as we commit the composition instead of cancelling it in many cases (ie. moving from one field to another etc)
> > 
> > r+ upon kenneth's concern is not an issue.
> > 
> > I was about to ask the same: if one want to cancel selection without actually disturbing selection.
> 
> We kind of need that. That is how our mobile platform works. We commit when loosing focus (could be just moving the cursor position with a touch in the same input field) and before injecting compositions into our virtual keyboard (used when the user touches a word and it becomes a composition)

Although the patch is not chaning any actual behavior, but cleaning it up instead. So if it is/was broken for us with the patch, it will keep beeing.

[Kenneth Rohde Christiansen]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (In reply to comment #2)
> > > > So what if I really want to confirm? We are depending a lot of this method in our slightly outdated branch as we commit the composition instead of cancelling it in many cases (ie. moving from one field to another etc)
> > > 
> > > r+ upon kenneth's concern is not an issue.
> > > 
> > > I was about to ask the same: if one want to cancel selection without actually disturbing selection.
> > 
> > We kind of need that. That is how our mobile platform works. We commit when loosing focus (could be just moving the cursor position with a touch in the same input field) and before injecting compositions into our virtual keyboard (used when the user touches a word and it becomes a composition)
> 
> Although the patch is not chaning any actual behavior, but cleaning it up instead. So if it is/was broken for us with the patch, it will keep beeing.

Sure, but the behavior was changed without me being aware of it. Examples of use:

http://gitorious.org/+qtwebkit-webkit2-dev/webkit/qtwebkit-webkit2-dev/blobs/master/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp

I think we are using it one other place, but I don't have the source handy right now.

[Ryosuke Niwa]

(In reply to comment #6)
> Sure, but the behavior was changed without me being aware of it. Examples of use:
> 
> http://gitorious.org/+qtwebkit-webkit2-dev/webkit/qtwebkit-webkit2-dev/blobs/master/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp
> 
> I think we are using it one other place, but I don't have the source handy right now.

Kenneth and I chatted over this and he's going to investigate further if he can work-around by using other API.

I'm going to land this patch for now because I don't want to leave the function name mismatching to its behavior. We can always add new API for Qt if it's necessary.

[Ryosuke Niwa]

Comment on attachment 106469 [details]
cleanup

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

> Source/WebCore/editing/Editor.cpp:1494
> -    if (preserveSelection)
> +    if (mode == ConfirmComposition)

Oops, this obviously needs to be "mode == CancelComposition". Will fix before landing the patch.

[Ryosuke Niwa]

Committed r94613: <http://trac.webkit.org/changeset/94613>