Bug 96508 – [Chromium] Merge moveSelectionStart, moveSelectionEnd, and moveCaret into selectRange

• Component: New Bugs
• Status: RESOLVED FIXED

[Iain Merrick]

[chromium] WebViewImpl::selectionRange should return start/end on Android

[Iain Merrick]

Created attachment 163617 [details]
[e-mail]

[Iain Merrick]

There's an unfortunate conflict between https://bugs.webkit.org/show_bug.cgi?id=66973 (WebViewImpl::selectionBounds) and my recent patch https://bugs.webkit.org/show_bug.cgi?id=93998 (WebFrameImpl::selectRange).

66973 makes selectionBounds return base/extent rather than start/end. But on Android we do want start/end, in order to draw the selection handles correctly. Why do we need base/extent on other platforms?

93998 interacts badly because it tries to handle base/extent correctly on Android: "extent" is always the selection handle currently being moved, "base" is the stationary one. This means isBaseFirst() can flip back and forth as the user adjusts the selection -- that's correct, but it means the values returned by selectionBounds will flip back and forth too.

I've uploaded a patch that just skips the isBaseFirst check on OS(ANDROID). That obviously isn't ideal. Varun, is there anything else we could do here? Have I just taken the wrong approach in 93998?

[Iain Merrick]

Created attachment 163621 [details]
[e-mail]

[Ryosuke Niwa]

Comment on attachment 163621 [details]
[e-mail]

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:2266
> +// "Start" always precedes "end". "Base" is where the user's selection gesture
> +// started, "end" is where it finished. Most platforms want base/extent here.
> +// Android wants start/end even if base/extent are reversed.
> +// FIXME: This shouldn't be OS-specific. http://webkit.org/b/96508

This is very confusing. Why are arguments called start and end, and yet they expect it to be base and extent? Furthermore, they aren't base and extent. They're anchor and focus.
The terminology here is all messed up. Please fix them.

Also, we should probably add a new enum like ShouldUseAnchorAndFocus with values UseAnchorAndFocus and UseStartAndEnd instead of hard-coding if-endif like this.

[Iain Merrick]

Comment on attachment 163621 [details]
[e-mail]

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

>> Source/WebKit/chromium/src/WebViewImpl.cpp:2266
>> +// FIXME: This shouldn't be OS-specific. http://webkit.org/b/96508
> 
> This is very confusing. Why are arguments called start and end, and yet they expect it to be base and extent? Furthermore, they aren't base and extent. They're anchor and focus.
> The terminology here is all messed up. Please fix them.
> 
> Also, we should probably add a new enum like ShouldUseAnchorAndFocus with values UseAnchorAndFocus and UseStartAndEnd instead of hard-coding if-endif like this.

I could just remove this swizzle completely, instead of #ifdef'ing it, but I assume there was some good reason for doing it in the first place. Any idea what that might be? Do you think I could just go ahead and delete these 4 lines?

[Iain Merrick]

I would also appreciate any comments you have on https://bugs.webkit.org/show_bug.cgi?id=93998, which was submitted in your absence.

[Iain Merrick]

Just had a chat offline about this with olilan (who wrote our text selection code for Android). There's a similar start/end swizzle in our Java code as well.

I think we could use *either* start/end or base/extent, we just need to be much clearer about which it is, and make sure we do the same thing consistently in all the APIs. The smallest change would be to use base/extent, as that's what most of the code already does (despite parameter names to the contrary). That means:

WebFrameImpl::selectRange() should take base and extent parameters
WebViewImpl::selectionBounds() should return base and extent

My new moveSelectionStart / moveSelection end methods wouldn't be needed.

I can go ahead and make a patch for that. Ryosuke, does that sound like a reasonable approach?

Also, can you explain the distinction between base/extent and anchor/focus? It looks to me like those are just the WebKit and HTML names for the same concepts, but maybe there's an important difference.

[Ryosuke Niwa]

(In reply to comment #7)
> I think we could use *either* start/end or base/extent, we just need to be much clearer about which it is, and make sure we do the same thing consistently in all the APIs.

Yes.

> The smallest change would be to use base/extent, as that's what most of the code already does (despite parameter names to the contrary). That means:
> 
> WebFrameImpl::selectRange() should take base and extent parameters
> WebViewImpl::selectionBounds() should return base and extent

Okay but from the code, it appears that you're using anchor/focus instead.

> My new moveSelectionStart / moveSelection end methods wouldn't be needed.
> 
> I can go ahead and make a patch for that. Ryosuke, does that sound like a reasonable approach?

Sure. I'm happy as long as there isn't obnoxious if-endif depending on Android.

> Also, can you explain the distinction between base/extent and anchor/focus? It looks to me like those are just the WebKit and HTML names for the same concepts, but maybe there's an important difference.

Base/extent are different from anchor/focus when the user double or triple clicks on a page and creates a selection with granularity other than character. In such case, base/extent are at where the user clicked whereas anchor/focus will be extended to where the selection ends are.

[Iain Merrick]

Created attachment 164414 [details]
[e-mail]

[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.

[Ryosuke Niwa]

Comment on attachment 164414 [details]
[e-mail]

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

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1477
> +        TextGranularity granularity = frameSelection->isNone() ? CharacterGranularity : frameSelection->granularity();

This doesn't look right. Why do we want to select a word just because a word was selected elsewhere before?

[Ryosuke Niwa]

Comment on attachment 164414 [details]
[e-mail]

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

> Source/WebKit/chromium/ChangeLog:4


We also need to rename the bug as these patches do much more than renaming argument variables.

[Iain Merrick]

Comment on attachment 164414 [details]
[e-mail]

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

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1477
>> +        TextGranularity granularity = frameSelection->isNone() ? CharacterGranularity : frameSelection->granularity();
> 
> This doesn't look right. Why do we want to select a word just because a word was selected elsewhere before?

Hmm, that felt like the right way to handle an existing selection, but you're right, there isn't really a use case for this and it isn't being tested.

Will just use character granularity.

[Iain Merrick]

Created attachment 164541 [details]
[e-mail]

[Ryosuke Niwa]

Comment on attachment 164541 [details]
[e-mail]

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

> Source/WebKit/chromium/ChangeLog:3
> +        [chromium] Fix parameter names selectionBounds and selectRange, remove moveSelectionStart/End/Caret

I've just updated the bug title to be something more appropriate. Please update this line.

[Ryosuke Niwa]

Comment on attachment 164541 [details]
[e-mail]

Ugh... I mean cq-.

[Iain Merrick]

Comment on attachment 164541 [details]
[e-mail]

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

>> Source/WebKit/chromium/ChangeLog:3
>> +        [chromium] Fix parameter names selectionBounds and selectRange, remove moveSelectionStart/End/Caret
> 
> I've just updated the bug title to be something more appropriate. Please update this line.

Done

[Iain Merrick]

Created attachment 164579 [details]
[e-mail]

[Iain Merrick]

Comment on attachment 164579 [details]
[e-mail]

Updated ChangeLog headline and "reviewed by" only

[WebKit Review Bot]

Comment on attachment 164579 [details]
[e-mail]

Clearing flags on attachment: 164579

Committed r128903: <http://trac.webkit.org/changeset/128903>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.