Bug 96508

Created attachment 163617 [details] husky@chromium.org

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?

Created attachment 163621 [details] husky@chromium.org

Comment on attachment 163621 [details] husky@chromium.org 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.

Comment on attachment 163621 [details] husky@chromium.org 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?

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

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.

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

Created attachment 164414 [details] husky@chromium.org

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

Comment on attachment 164414 [details] husky@chromium.org 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?

Comment on attachment 164414 [details] husky@chromium.org 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.

Comment on attachment 164414 [details] husky@chromium.org 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.

Created attachment 164541 [details] husky@chromium.org

Comment on attachment 164541 [details] husky@chromium.org 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.

Comment on attachment 164541 [details] husky@chromium.org Ugh... I mean cq-.

Comment on attachment 164541 [details] husky@chromium.org 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

Created attachment 164579 [details] husky@chromium.org

Comment on attachment 164579 [details] husky@chromium.org Updated ChangeLog headline and "reviewed by" only

Comment on attachment 164579 [details] husky@chromium.org Clearing flags on attachment: 164579 Committed r128903: <http://trac.webkit.org/changeset/128903>

All reviewed patches have been landed. Closing bug.