92513 – [chromium] Upstream Android changes to WebFrameImpl::selectRange

Iain Merrick

Reported 2012-07-27 09:10:02 PDT

Android-specific changes made to WebFrameImpl methods selectRange and setCaretSelectionAtWindowPoint need to be upstreamed back into WebKit. See related Chromium bug http://code.google.com/p/chromium/issues/detail?id=138939

Iain Merrick

Comment 1 2012-07-27 09:15:42 PDT

Created attachment 154971 [details] Patch

WebKit Review Bot

Comment 2 2012-07-27 09:29:22 PDT

Attachment 154971 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1 Source/WebKit/chromium/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] Source/WebKit/chromium/ChangeLog:9: Line contains tab character. [whitespace/tab] [5] Source/WebKit/chromium/ChangeLog:10: Line contains tab character. [whitespace/tab] [5] Total errors found: 3 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.

Iain Merrick

Comment 3 2012-07-30 01:16:04 PDT

Created attachment 155229 [details] Patch

Hans Wennborg

Comment 4 2012-07-31 04:11:54 PDT

Comment on attachment 155229 [details] Patch Just a couple of drive-by style nits; I can't comment on the actual functionality here. View in context: https://bugs.webkit.org/attachment.cgi?id=155229&action=review > Source/WebKit/chromium/src/WebFrameImpl.cpp:1463 > + // Ensure startPosition is before endPosition nit: finish sentence with period; applies for comments below as well. > Source/WebKit/chromium/src/WebFrameImpl.cpp:1467 > + endPosition = tmp; maybe just std::swap(startPosition, endPosition) > Source/WebKit/chromium/src/WebFrameImpl.cpp:1499 > + WebCore::EUserTriggered userTriggered = static_cast<WebCore::EUserTriggered>(true); the cast looks funny how about "const WebCore::EUserTriggered userTriggered = WebCore::UserTriggered;" ? or maybe just passing in WebCore::UserTriggered in the function calls where it is used below?

Iain Merrick

Comment 5 2012-07-31 06:22:51 PDT

Comment on attachment 155229 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155229&action=review >> Source/WebKit/chromium/src/WebFrameImpl.cpp:1463 >> + // Ensure startPosition is before endPosition > > nit: finish sentence with period; applies for comments below as well. Done >> Source/WebKit/chromium/src/WebFrameImpl.cpp:1467 >> + endPosition = tmp; > > maybe just std::swap(startPosition, endPosition) Good call, done. >> Source/WebKit/chromium/src/WebFrameImpl.cpp:1499 >> + WebCore::EUserTriggered userTriggered = static_cast<WebCore::EUserTriggered>(true); > > the cast looks funny > how about "const WebCore::EUserTriggered userTriggered = WebCore::UserTriggered;" ? > or maybe just passing in WebCore::UserTriggered in the function calls where it is used below? Huh, you're right, that is a bit odd. It does seem correct for it always to be true, as this method is specifically intended to convert touch coordinates into a selection range. I'll just pass WebCore::UserTriggered directly.

Iain Merrick

Comment 6 2012-07-31 06:39:23 PDT

Created attachment 155518 [details] Patch

Peter Beverloo

Comment 7 2012-07-31 07:22:39 PDT

Please make sure your patches block the meta bug: bug 66687 :). Thanks!

Peter Beverloo

Comment 8 2012-07-31 07:57:00 PDT

Comment on attachment 155518 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155518&action=review Could we write layout or unit tests to verify correct behavior of this code? It's a lot of logic on the WebKit API side.. Do you have an example/explanation of what is broken right now? At least on desktop, selections in content editable elements seems to be clamped to the editable root already. > Source/WebKit/chromium/src/WebFrameImpl.cpp:1501 > + // New selection is outside input box - move cursor towards selection point. s/input box/editable element? > Source/WebKit/chromium/src/WebFrameImpl.h:287 > + void setCaretSelectionAtWindowPoint(const WebPoint&); Is this method called from outside WebFrameImpl? Otherwise it should be private.

Peter Beverloo

Comment 9 2012-07-31 07:57:34 PDT

+rniwa, as this is about selection/editing.

Iain Merrick

Comment 10 2012-07-31 08:21:11 PDT

Comment on attachment 155518 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155518&action=review >> Source/WebKit/chromium/src/WebFrameImpl.cpp:1501 >> + // New selection is outside input box - move cursor towards selection point. > > s/input box/editable element? Done >> Source/WebKit/chromium/src/WebFrameImpl.h:287 >> + void setCaretSelectionAtWindowPoint(const WebPoint&); > > Is this method called from outside WebFrameImpl? Otherwise it should be private. Nope, it isn't. Done.

Iain Merrick

Comment 11 2012-07-31 08:30:25 PDT

Created attachment 155547 [details] Patch

Adam Barth

Comment 12 2012-07-31 09:19:25 PDT

Comment on attachment 155547 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155547&action=review > Source/WebKit/chromium/src/WebFrameImpl.cpp:1458 > + VisiblePosition startPosition(visiblePositionForWindowPoint(start)); > + VisiblePosition endPosition(visiblePositionForWindowPoint(end)); nit: WebKit generally prefers the assignment form of the constructor: VisiblePosition startPosition = visiblePositionForWindowPoint(start); > Source/WebKit/chromium/src/WebFrameImpl.cpp:1463 > + // Ensure startPosition is before endPosition. This comment just states what the code does. We generally frown on these sorts of comments. When we include comments, we prefer comments that explain why the code does something rather than re-stating what the code does. > Source/WebKit/chromium/src/WebFrameImpl.cpp:1474 > + // Ensure startPosition is not at end of block. ditto > Source/WebKit/chromium/src/WebFrameImpl.cpp:1480 > + // Ensure endPosition is not at start of block. ditto > Source/WebKit/chromium/src/WebFrameImpl.cpp:1496 > + if (frame()->selection()->isContentEditable()) { Prefer early return.

Adam Barth

Comment 13 2012-07-31 09:20:05 PDT

My comments above are just trivialities. @rniwa: Would you be willing to give this code a real review?

Ryosuke Niwa

Comment 14 2012-07-31 11:54:36 PDT

Comment on attachment 155547 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155547&action=review > Source/WebKit/chromium/src/WebFrameImpl.cpp:1460 > + if (startPosition.isNull() || endPosition.isNull() || startPosition == endPosition) We never set a collapsed selection by this function? Why? > Source/WebKit/chromium/src/WebFrameImpl.cpp:1472 > + if (frame()->selection()->isContentEditable()) { > + startPosition = frame()->selection()->selection().visibleStart().honorEditingBoundaryAtOrAfter(startPosition); > + endPosition = frame()->selection()->selection().visibleEnd().honorEditingBoundaryAtOrBefore(endPosition); > + if (startPosition.isNull() || endPosition.isNull()) > + return; > + } This is done in setSelection. > Source/WebKit/chromium/src/WebFrameImpl.cpp:1479 > + if (startPosition != endPosition && isEndOfBlock(startPosition)) { > + VisiblePosition nextStartPosition(startPosition.next()); > + if (!nextStartPosition.isNull()) > + startPosition = nextStartPosition; > + } Why are we doing this? > Source/WebKit/chromium/src/WebFrameImpl.cpp:1535 > + if (currentCaretRect.x() > point.x) { > + // Cursor should move left unless that would result in a line change or would result in the cursor > + // moving past the selection point. > + movePosition = currentPosition.previous(); > + shouldMove = frame()->view()->contentsToWindow(movePosition.absoluteCaretBounds()).x() > point.x; > + moveDirection = DirectionLeft; > + } else if (currentCaretRect.x() < point.x) { > + // Cursor should move right unless that would result in a line change or would result in the cursor > + // moving past the selection point. > + movePosition = currentPosition.next(); > + shouldMove = frame()->view()->contentsToWindow(movePosition.absoluteCaretBounds()).x() < point.x; > + moveDirection = DirectionRight; > + } > + > + if (shouldMove && inSameLine(currentPosition, movePosition)) > + frame()->selection()->modify(FrameSelection::AlterationMove, moveDirection, CharacterGranularity, UserTriggered); > + > + shouldMove = false; > + if (currentCaretRect.y() > point.y) { > + // Cursor should move up unless that would not result in a line change > + // as that means it's already at the top. > + shouldMove = !inSameLine(currentPosition, previousLinePosition(currentPosition, 0)); > + moveDirection = DirectionBackward; > + } else if (currentCaretRect.y() < point.y) { > + // Cursor should move down unless that would not result in a line change > + // as that means it's already at the bottom. > + shouldMove = !inSameLine(currentPosition, nextLinePosition(currentPosition, 0)); > + moveDirection = DirectionForward; > + } What? Why do we need all of this code. This clearly doesn't work at all for bidirectional text. See what EventHandler::handleMousePressEventSingleClick and other functions in EventHandler.cpp do. We should be re-using the code there.

Iain Merrick

Comment 15 2012-08-01 04:04:26 PDT

Comment on attachment 155547 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155547&action=review >> Source/WebKit/chromium/src/WebFrameImpl.cpp:1460 >> + if (startPosition.isNull() || endPosition.isNull() || startPosition == endPosition) > > We never set a collapsed selection by this function? Why? This method is called when dragging one of the selection handles. We don't allow a collapsed selection because that would cause us to exit selection mode. However, this method is *also* called when we specifically want to set a single insertion point (lines 1452-5). That's not good -- maybe selectRange() is too general and we should have separate methods for those two use cases. >> Source/WebKit/chromium/src/WebFrameImpl.cpp:1463 >> + // Ensure startPosition is before endPosition. > > This comment just states what the code does. We generally frown on these sorts of comments. When we include comments, we prefer comments that explain why the code does something rather than re-stating what the code does. Yeah, we haven't been aggressive enough at policing these in the Android fork. I'll remove all these comments. >> Source/WebKit/chromium/src/WebFrameImpl.cpp:1535 >> + } > > What? Why do we need all of this code. This clearly doesn't work at all for bidirectional text. > See what EventHandler::handleMousePressEventSingleClick and other functions in EventHandler.cpp do. > We should be re-using the code there. I'll refactor all this stuff.

Iain Merrick

Comment 16 2012-08-01 04:05:54 PDT

Thanks for the comments! This is the code we're currently shipping with, so I wanted to start with that patch, but I'm happy to make all the requested changes.

Adam Barth

Comment 17 2012-08-01 11:33:44 PDT

Thanks Iain.

Iain Merrick

Comment 18 2012-08-02 06:44:58 PDT

Comment on attachment 155547 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155547&action=review >> Source/WebKit/chromium/src/WebFrameImpl.cpp:1472 >> + } > > This is done in setSelection. I think this is incorrect. Assuming you mean FrameSelection::setSelection, that method doesn't seem to check for editable boundaries (both by code inspection, and by experimentally removing this code from the Android port). FrameSelection::modifyExtendingRight and similar methods *do* clip at the editable boundaries, but setSelection doesn't. Is that correct, or am I misreading the code? I don't think setSelection should truncate to the editable boundaries, because then there would be no way to select an entire different part of the page. Is there another suitable method call? Possibly FrameSelection::setSelectedRange?

Iain Merrick

Comment 19 2012-08-02 07:10:38 PDT

Created attachment 156075 [details] Patch

Iain Merrick

Comment 20 2012-08-02 07:12:35 PDT

I've removed setCaretSelectionAtWindowPoint as that needs heavy rewriting. I'll address that in a later patch. I've trimmed selectRange down to what I think is the minimal set of changes needed (apart from special handling for caret selections).

Ryosuke Niwa

Comment 21 2012-08-02 09:47:31 PDT

Comment on attachment 155547 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155547&action=review >>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1472 >>> + } >> >> This is done in setSelection. > > I think this is incorrect. Assuming you mean FrameSelection::setSelection, that method doesn't seem to check for editable boundaries (both by code inspection, and by experimentally removing this code from the Android port). > > FrameSelection::modifyExtendingRight and similar methods *do* clip at the editable boundaries, but setSelection doesn't. Is that correct, or am I misreading the code? > > I don't think setSelection should truncate to the editable boundaries, because then there would be no way to select an entire different part of the page. Is there another suitable method call? Possibly FrameSelection::setSelectedRange? I misspoke. This is done in VisibleSelection::VisibleSelection.

Iain Merrick

Comment 22 2012-08-02 09:53:32 PDT

(In reply to comment #21) > I misspoke. This is done in VisibleSelection::VisibleSelection. Hmm, does that happen via VisibleSelection::validate()? (which the constructors generally end up calling) Various things happen in there but I don't see any specific logic to check the editable boundaries, and the code behaves differently when we do it manally (as in this patch). Maybe I'm just looking in the wrong place?

Ryosuke Niwa

Comment 23 2012-08-02 09:54:05 PDT

Comment on attachment 156075 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156075&action=review > Source/WebKit/chromium/src/WebFrameImpl.cpp:1466 > + if (frame()->selection()->isContentEditable()) { > + startPosition = frame()->selection()->selection().visibleStart().honorEditingBoundaryAtOrAfter(startPosition); > + endPosition = frame()->selection()->selection().visibleEnd().honorEditingBoundaryAtOrBefore(endPosition); > + if (startPosition.isNull() || endPosition.isNull()) > + return; > + } Adjustment around editing boundary is done in VisibleSelection::VisibleSelection > VisibleSelection::validate > VisibleSelection::adjustSelectionToAvoidCrossingEditingBoundaries.

Iain Merrick

Comment 24 2012-08-02 09:55:48 PDT

(In reply to comment #23) > (From update of attachment 156075 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=156075&action=review > > > Source/WebKit/chromium/src/WebFrameImpl.cpp:1466 > > + if (frame()->selection()->isContentEditable()) { > > + startPosition = frame()->selection()->selection().visibleStart().honorEditingBoundaryAtOrAfter(startPosition); > > + endPosition = frame()->selection()->selection().visibleEnd().honorEditingBoundaryAtOrBefore(endPosition); > > + if (startPosition.isNull() || endPosition.isNull()) > > + return; > > + } > > Adjustment around editing boundary is done in VisibleSelection::VisibleSelection > VisibleSelection::validate > VisibleSelection::adjustSelectionToAvoidCrossingEditingBoundaries. Aha, doh! Now I see it. Thanks! I suspect I just have to be more careful about handling all four of start/end/base/extent, rather than just start and end.

Ryosuke Niwa

Comment 25 2012-08-02 09:57:14 PDT

(In reply to comment #24) > Aha, doh! Now I see it. Thanks! > > I suspect I just have to be more careful about handling all four of start/end/base/extent, rather than just start and end. Please don't manually adjust selection end points in WebKit code. That's extremely error prone.

Iain Merrick

Comment 26 2012-08-02 10:03:48 PDT

(In reply to comment #25) > (In reply to comment #24) > > Aha, doh! Now I see it. Thanks! > > > > I suspect I just have to be more careful about handling all four of start/end/base/extent, rather than just start and end. > > Please don't manually adjust selection end points in WebKit code. That's extremely error prone. Sorry, I'll clarify. Reading adjustSelectionToAvoidCrossingEditingBoundaries, it looks like when the selection starts in non-editable text and ends in editable text, the endpoint is adjusted rather than the start point. That's not what the Android port wants in this situation.

Ryosuke Niwa

Comment 27 2012-08-02 10:22:46 PDT

(In reply to comment #26) > Sorry, I'll clarify. Reading adjustSelectionToAvoidCrossingEditingBoundaries, it looks like when the selection starts in non-editable text and ends in editable text, the endpoint is adjusted rather than the start point. That's not what the Android port wants in this situation. If you're adding code only for Android port, then why are you changing WebFrameImpl::selectRange? Also, are you saying that when a user selects text from point A to point B and they're not in the same editable region, you want point A to be adjusted instead of point B?

Iain Merrick

Comment 28 2012-08-06 06:07:19 PDT

(In reply to comment #27) > (In reply to comment #26) > > Sorry, I'll clarify. Reading adjustSelectionToAvoidCrossingEditingBoundaries, it looks like when the selection starts in non-editable text and ends in editable text, the endpoint is adjusted rather than the start point. That's not what the Android port wants in this situation. > > If you're adding code only for Android port, then why are you changing WebFrameImpl::selectRange? Well, where's a better place to put it? This is part of the WebKit API and it needs to behave sensibly for all touch-based UIs. > Also, are you saying that when a user selects text from point A to point B and they're not in the same editable region, you want point A to be adjusted instead of point B? No, this call is used when adjusting an existing selection. If points A and B are both inside an editable region, and point A is then moved before the start of the region, what *should* happen is that point A gets pinned at the start. What happens instead is that point B gets pinned to the end of the previous region -- not what the user wants. Unfortunately the API doesn't include any information about whether the start or end is being adjusted. I suspect we need to either extend the API, or use a heuristic (for example, if one endpoint changes and the other doesn't, that's the endpoint that needs to be clipped against the editing boundaries). I'll try pushing this logic into FrameSelection and/or Editor, which already have some similar code.

Ryosuke Niwa

Comment 29 2012-08-06 07:39:48 PDT

(In reply to comment #28) > > Also, are you saying that when a user selects text from point A to point B and they're not in the same editable region, you want point A to be adjusted instead of point B? > > No, this call is used when adjusting an existing selection. If points A and B are both inside an editable region, and point A is then moved before the start of the region, what *should* happen is that point A gets pinned at the start. What happens instead is that point B gets pinned to the end of the previous region -- not what the user wants. What you need to do is to treat point A as the "extent" of the selection, not the "base". Selection has this concept of "base" and "extent" (similar to what we expose to JavaScript as "anchor" and "focus") and end points are adjusted to be inside the editable region of the "base". If either point is adjusted, then that end should be treated as the extent. > Unfortunately the API doesn't include any information about whether the start or end is being adjusted. Why can't we expose that information? Given what you want to do, it's insane not to expose that information.

Ryosuke Niwa

Comment 30 2012-08-06 07:43:53 PDT

See https://developer.mozilla.org/en-US/docs/DOM/Selection. "base"="anchor" and "extent"="focus" for all practical purposes (they are technically different).

Iain Merrick

Comment 31 2012-08-06 08:01:30 PDT

> > Unfortunately the API doesn't include any information about whether the start or end is being adjusted. > > Why can't we expose that information? Given what you want to do, it's insane not to expose that information. We already have several redundant-looking APIs in WebFrame: virtual void setSelectionToRange(const WebRange&); virtual void selectRange(const WebPoint& start, const WebPoint& end); virtual void selectRange(const WebRange&); As there are no comments, the semantics are very unclear. I think we should clean those up before adding even more APIs -- do you agree?

Ryosuke Niwa

Comment 32 2012-08-06 08:02:55 PDT

(In reply to comment #31) > We already have several redundant-looking APIs in WebFrame: > > virtual void setSelectionToRange(const WebRange&); > virtual void selectRange(const WebPoint& start, const WebPoint& end); > virtual void selectRange(const WebRange&); > > As there are no comments, the semantics are very unclear. I think we should clean those up before adding even more APIs -- do you agree? Definitely. Also, the intent of each function should be clear from function names, not comments.

Iain Merrick

Comment 33 2012-08-06 08:13:01 PDT

FrameSelection::setBase and setExtent don't have the semantics discussed above (honoring the boundaries of the current element if it's editable). Manually testing with the desktop UI confirms this: if I have some editable text selected, shift-clicking in a non-editable part of the page does a deselect-all. That's correct for desktop, not correct for a touch UI. I'm thinking of extending FrameSelection::SetSelectionOption with a new "HonorEditingBoundaries" flag. How does that sound?

Ryosuke Niwa

Comment 34 2012-08-06 08:43:09 PDT

(In reply to comment #33) > FrameSelection::setBase and setExtent don't have the semantics discussed above (honoring the boundaries of the current element if it's editable). Manually testing with the desktop UI confirms this: if I have some editable text selected, shift-clicking in a non-editable part of the page does a deselect-all. That's correct for desktop, not correct for a touch UI. I've never said that you should be using setBase and setExtent.

Ryosuke Niwa

Comment 35 2012-08-06 08:45:18 PDT

Please observe the behavior of selection by a mouse drag. When you start a mouse drag inside an editable region and extent mouse moves out of the editable region, selection extends until the end of the editable region. You can read the code that does this in Source/page/EventHandler.cpp.

Ryosuke Niwa

Comment 36 2012-08-06 09:00:38 PDT

See EventHandler::updateSelectionForMouseDrag at http://trac.webkit.org/browser/trunk/Source/WebCore/page/EventHandler.cpp?124774#L780 for example.

Iain Merrick

Comment 37 2012-08-06 09:03:54 PDT

I guess this gets back to your original comment about reusing that code. Just to be clear, do you want WebFrameImpl::selectRange to call through to EventHandler (either an existing method or a new method)? Or to somewhere else?

Ryosuke Niwa

Comment 38 2012-08-06 09:05:55 PDT

(In reply to comment #37) > I guess this gets back to your original comment about reusing that code. Just to be clear, do you want WebFrameImpl::selectRange to call through to EventHandler (either an existing method or a new method)? Or to somewhere else? Yeah, that'll be good. Alternatively, you can extract some code from EventHandler and move it to FrameSelection as well.

Iain Merrick

Comment 39 2012-08-06 09:08:34 PDT

Great, thanks for your patience!

Iain Merrick

Comment 40 2012-08-07 06:53:30 PDT

Created attachment 156931 [details] Patch

Iain Merrick

Comment 41 2012-08-07 07:09:52 PDT

This code seems to work. Needs a test -- I'm looking at https://bugs.webkit.org/show_bug.cgi?id=93108 now.

Ryosuke Niwa

Comment 42 2012-08-07 08:05:47 PDT

Comment on attachment 156931 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156931&action=review > Source/WebCore/page/EventHandler.cpp:760 > -static VisiblePosition selectionExtentRespectingEditingBoundary(const VisibleSelection& selection, const LayoutPoint& localPoint, Node* targetNode) > +VisiblePosition EventHandler::visiblePositionRespectingEditingBoundary(const HitTestResult& hitTestResult) Why are we renaming this function? This function is about adjusting extent, not either base or extent. > Source/WebCore/page/EventHandler.cpp:762 > + LayoutPoint layoutPoint = hitTestResult.localPoint(); Why are we renaming this variable to layoutPoint? That doesn't explain anything. It's like naming a variable int or boolean. > Source/WebCore/page/EventHandler.cpp:3714 > + VisiblePosition startPosition = visiblePositionRespectingEditingBoundary(hitTestStart); > + VisiblePosition endPosition = visiblePositionRespectingEditingBoundary(hitTestEnd); This is not right. Either end point needs to be base, and that's the one we should be adjusting, not both. > Source/WebCore/page/EventHandler.cpp:3716 > + VisibleSelection newSelection(startPosition, endPosition); We need to know which point is base & extent here to create a selection properly. > Source/WebKit/chromium/src/WebFrameImpl.cpp:1461 > + frame()->eventHandler()->handleSelectRange(IntPoint(start.x, start.y), IntPoint(end.x, end.y)); We should definitely not be calling into eventHandler here since we're not handling an event.

Iain Merrick

Comment 43 2012-08-07 08:16:08 PDT

Comment on attachment 156931 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156931&action=review >> Source/WebCore/page/EventHandler.cpp:760 >> +VisiblePosition EventHandler::visiblePositionRespectingEditingBoundary(const HitTestResult& hitTestResult) > > Why are we renaming this function? This function is about adjusting extent, not either base or extent. There's nothing specific to extent in this function. >> Source/WebCore/page/EventHandler.cpp:762 >> + LayoutPoint layoutPoint = hitTestResult.localPoint(); > > Why are we renaming this variable to layoutPoint? That doesn't explain anything. > It's like naming a variable int or boolean. Oops, I think I meant localPoint (as that's what the parameter that I removed was called). I'll switch it back to selectionEndPoint, though. >> Source/WebCore/page/EventHandler.cpp:3716 >> + VisibleSelection newSelection(startPosition, endPosition); > > We need to know which point is base & extent here to create a selection properly. There is no such distinction in the Android text selection model (and I believe touch-based ChromeOS too). There's a selection handle at each end of the selection, and either one can be moved freely. What I could maybe do is have WebFrameImpl figure out which endpoint is moving, and define that one to be the extent. Does that sound plausible? >> Source/WebKit/chromium/src/WebFrameImpl.cpp:1461 >> + frame()->eventHandler()->handleSelectRange(IntPoint(start.x, start.y), IntPoint(end.x, end.y)); > > We should definitely not be calling into eventHandler here since we're not handling an event. OK, I'll move the shared bits of logic into FrameSelection.

Ryosuke Niwa

Comment 44 2012-08-07 08:21:20 PDT

(In reply to comment #43) > >> Source/WebCore/page/EventHandler.cpp:3716 > >> + VisibleSelection newSelection(startPosition, endPosition); > > > > We need to know which point is base & extent here to create a selection properly. > > There is no such distinction in the Android text selection model (and I believe touch-based ChromeOS too). There's a selection handle at each end of the selection, and either one can be moved freely. > > What I could maybe do is have WebFrameImpl figure out which endpoint is moving, and define that one to be the extent. Does that sound plausible? Yes, that's exactly what we need here.

Iain Merrick

Comment 45 2012-08-08 05:58:06 PDT

Created attachment 157198 [details] Patch

Iain Merrick

Comment 46 2012-08-08 06:06:35 PDT

I was trying to do too many things at once, so I'm now aiming for a minimal refactoring: - Extract EventHandler::selectionExtentRespectingEditingBoundary into VisibleSelection. - Call that from WebFrameImpl::visiblePositionForWindowPoint. I have re-added the unit test from https://bugs.webkit.org/show_bug.cgi?id=93108, which got rolled back due to Windows failures. I'll extend it to cover the editable element case and check that it works on all platforms. I think it would be good to move the targetNode() function into HitTestResult. That would remove some duplicate code, and we could also simplify the parameter list of selectionExtentRespectingEditingBoundary. Not essential for this patch, so I added a FIXME comment. I have NOT addressed the base/extent confusion that Ryosuke identified. I think that should be done in a separate patch, by an API change (the SelectRange message should specify which endpoint is moving).

Ryosuke Niwa

Comment 47 2012-08-08 10:47:43 PDT

Comment on attachment 157198 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157198&action=review > Source/WebCore/ChangeLog:4 > + Move selectionExtentRespectingEditingBoundary() into VisibleSelection > + https://bugs.webkit.org/show_bug.cgi?id=92513 Please file a new bug and post the patch there. We don't typically one patch per each bug. > Source/WebKit/chromium/tests/data/select_range_basic.html:1 > +some test text for testing Missing DOCTYPE. > Source/WebKit/chromium/tests/data/select_range_iframe.html:1 > +<html> Ditto. > Source/WebKit/chromium/tests/data/select_range_scroll.html:1 > +<div style='height:3000px'> Ditto.

Iain Merrick

Comment 48 2012-08-09 05:33:32 PDT

Comment on attachment 157198 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157198&action=review >> Source/WebCore/ChangeLog:4 >> + https://bugs.webkit.org/show_bug.cgi?id=92513 > > Please file a new bug and post the patch there. > We don't typically one patch per each bug. This is the same bug! Did you make this comment because the ChangeLog entry is different? I intended that WebCore/ChangeLog should explain what was happening in WebCore, but to minimize confusion I'll use the same text in WebCore/ChangeLog and WebKit/ChangeLog. Fixed. >> Source/WebKit/chromium/tests/data/select_range_basic.html:1 >> +some test text for testing > > Missing DOCTYPE. I had a look at the other files in Source/WebKit/chromium/tests/data/. Out of 54 .html files, only 6 have doctype tags; and those 6 are just "<!DOCTYPE html>". If we think doctypes are important, we should add them to all the test files (but that's outside the scope of this bug). I'm inclined to leave out the doctype, for consistency with the surrounding code. But I can add "<!DOCTYPE html>" to these three files if you really think it's a blocker for this bug -- let me know. Not changed.

Peter Beverloo

Comment 49 2012-08-09 05:48:42 PDT

(In reply to comment #48) > > >> Source/WebKit/chromium/tests/data/select_range_basic.html:1 > >> +some test text for testing > > > > Missing DOCTYPE. > > I had a look at the other files in Source/WebKit/chromium/tests/data/. Out of 54 .html files, only 6 have doctype tags; and those 6 are just "<!DOCTYPE html>". > > If we think doctypes are important, we should add them to all the test files (but that's outside the scope of this bug). > > I'm inclined to leave out the doctype, for consistency with the surrounding code. But I can add "<!DOCTYPE html>" to these three files if you really think it's a blocker for this bug -- let me know. > > Not changed. Adding a doctype will mean the test is ran in standards mode, something that generally is a good practice, unless the test will rely on specific quirks behavior. This also implies that the tests will be much easier to port to other browsers, may they be interested, as standards mode is much more interoperable than quirks mode. WebKit has thousands of tests imported from other browsers and official test-suites. Going forth, I agree with Ryosuke that using standards mode when new tests are being added is the way to go.

Iain Merrick

Comment 50 2012-08-09 05:50:19 PDT

(In reply to comment #49) > (In reply to comment #48) > > > > >> Source/WebKit/chromium/tests/data/select_range_basic.html:1 > > >> +some test text for testing > > > > > > Missing DOCTYPE. > > > > I had a look at the other files in Source/WebKit/chromium/tests/data/. Out of 54 .html files, only 6 have doctype tags; and those 6 are just "<!DOCTYPE html>". > > > > If we think doctypes are important, we should add them to all the test files (but that's outside the scope of this bug). > > > > I'm inclined to leave out the doctype, for consistency with the surrounding code. But I can add "<!DOCTYPE html>" to these three files if you really think it's a blocker for this bug -- let me know. > > > > Not changed. > > Adding a doctype will mean the test is ran in standards mode, something that generally is a good practice, unless the test will rely on specific quirks behavior. This also implies that the tests will be much easier to port to other browsers, may they be interested, as standards mode is much more interoperable than quirks mode. WebKit has thousands of tests imported from other browsers and official test-suites. > > Going forth, I agree with Ryosuke that using standards mode when new tests are being added is the way to go. OK, will fix.

Iain Merrick

Comment 51 2012-08-09 09:04:18 PDT

Created attachment 157465 [details] Patch

Iain Merrick

Comment 52 2012-08-09 09:09:54 PDT

Work-in-progress patch. No review needed but any comments appreciated. Doctypes fixed. I made the tests a bit stricter, which revealed an annoying off-by-one error (I assume this is why it failed previously Windows): hitTest() and selectionBounds() aren't round-trip robust. Working on a fix or workaround for that. VisibleSelection::validate() and EventHandler::selectionExtentRespectingEditingBoundary() also have subtly different endpoint behaviour (VisibleSelection clips your endpoint to the end of the editable element, EventHandler clips it to the start of the next element), so I'm not calling selectionExtentRespectingEditingBoundary refactoring. I added doctypes to the test pages, and added a new one for editable element selection.

Iain Merrick

Comment 53 2012-08-09 09:11:10 PDT

(In reply to comment #52) > so I'm not calling selectionExtentRespectingEditingBoundary refactoring. Er, mangled prose there, sorry. I'm not reusing that method any more, so I also removed the refactoring.

Iain Merrick

Comment 54 2012-08-09 10:32:29 PDT

Hmm, unit tests seem to be using tiny font -- less than 1 pixel square per glyph. That makes it impossible to select text accurately with integer coordinates. Trying to figure out where that font gets set.

Iain Merrick

Comment 55 2012-08-09 10:54:46 PDT

Setting font-size explicitly in the test pages gives *different* selection errors. It consistently misses some chars at the end, or selects too many. Very strange.

Ryosuke Niwa

Comment 56 2012-08-09 10:56:19 PDT

(In reply to comment #48) > (From update of attachment 157198 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=157198&action=review > > >> Source/WebCore/ChangeLog:4 > >> + https://bugs.webkit.org/show_bug.cgi?id=92513 > > > > Please file a new bug and post the patch there. > > We don't typically one patch per each bug. > > This is the same bug! Did you make this comment because the ChangeLog entry is different? Is this the only patch you're going to post to this bug? (i.e. once this patch is landed, there's nothing else to do?) We try to land exactly one patch per bug. If you're intending to split the original patch into 2-3 pieces, and then those split pieces should be posted on new bugs that blocks this one. Also, the text above the bug URL should be the bug title. If you think the current bug title does not reflect what the patch does or the problem you're solving, then you should also update the bug title. > >> Source/WebKit/chromium/tests/data/select_range_basic.html:1 > >> +some test text for testing > > > > Missing DOCTYPE. > > I had a look at the other files in Source/WebKit/chromium/tests/data/. Out of 54 .html files, only 6 have doctype tags; and those 6 are just "<!DOCTYPE html>". > > If we think doctypes are important, we should add them to all the test files (but that's outside the scope of this bug). Yes. We should have DOCTYPE. Otherwise we'll be in quirks mode, which isn't spec'ed anywhere.

Iain Merrick

Comment 57 2012-08-09 10:58:22 PDT

Any idea why WebViewImpl::selectionBounds would give strange results? (much too small, it seems to me) Is there something odd about the rendering context that WebFrameTest sets up?

Iain Merrick

Comment 58 2012-08-09 11:08:54 PDT

(In reply to comment #56) [...] > If you're intending to split the original patch into 2-3 pieces, and then those split pieces should be posted on new bugs that blocks this one. > > Also, the text above the bug URL should be the bug title. If you think the current bug title does not reflect what the patch does or the problem you're solving, then you should also update the bug title. I think there are an indefinite number of improvements that could be made to all of this code, so I'll file separate bugs.

WebKit Review Bot

Comment 59 2012-08-09 12:46:59 PDT

Comment on attachment 157465 [details] Patch Attachment 157465 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13454953 New failing tests: WebFrameTest.SelectRange

WebKit Review Bot

Comment 60 2012-08-09 12:47:23 PDT

Created attachment 157518 [details] Archive of layout-test-results from gce-cr-linux-05 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-05 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Adam Barth

Comment 61 2012-08-10 10:17:02 PDT

> Any idea why WebViewImpl::selectionBounds would give strange results? (much too small, it seems to me) Is there something odd about the rendering context that WebFrameTest sets up? I don't know how many WebFrameTest actually render things. It's possible that things aren't set up in a reasonable way in the testing framework because we tend to use the LayoutTest framework for things related to rendering. It's possible to change your test into a LayoutTest, but it's unclear to me if that's the right thing to do here. I get the sense that you're getting a bit frustrated by the back-and-forth with rniwa. I'll try building your patch and seeing if I can figure out the issue.

Adam Barth

Comment 62 2012-08-10 11:42:13 PDT

@Iain: I'm not sure I understand these tests. The selectionBounds functions gives you a bounding box around the selection. If you then set the selection to that bounding box, it's not surprising that you're catching some additional text in the selection. The bounding box is an upper bound, after all.

Adam Barth

Comment 63 2012-08-10 12:17:43 PDT

Created attachment 157785 [details] cleaned up patch, but no test

Adam Barth

Comment 64 2012-08-10 12:19:31 PDT

Given that there are currently zero tests for WebFrame::selectRange, I don't think we should block this patch on lacking a test. It makes me sad, but that's the current state of affairs.

Adam Barth

Comment 65 2012-08-10 12:22:22 PDT

Created attachment 157787 [details] fixed ChangeLog

WebKit Review Bot

Comment 66 2012-08-10 14:45:58 PDT

Comment on attachment 157787 [details] fixed ChangeLog Clearing flags on attachment: 157787 Committed r125329: <http://trac.webkit.org/changeset/125329>

WebKit Review Bot

Comment 67 2012-08-10 14:46:08 PDT

All reviewed patches have been landed. Closing bug.

Iain Merrick

Comment 68 2012-08-13 03:45:11 PDT

Adam, thanks for following up! I didn't write that test originally, but the code looks sensible to me, so I'll continue to investigate and see if I can get it working reliably. I think it's still a better approach than a layout test, as that would require adding a SelectRange callback to the test runner. I'll also open new bugs for the refactoring changes I mentioned, like moving targetNode().

Attachments
Patch (7.67 KB, patch) 2012-07-27 09:15 PDT, Iain Merrick	no flags	Details Formatted Diff Diff
Patch (7.69 KB, patch) 2012-07-30 01:16 PDT, Iain Merrick	no flags	Details Formatted Diff Diff
Patch (7.57 KB, patch) 2012-07-31 06:39 PDT, Iain Merrick	no flags	Details Formatted Diff Diff
Patch (7.56 KB, patch) 2012-07-31 08:30 PDT, Iain Merrick	no flags	Details Formatted Diff Diff
Patch (2.89 KB, patch) 2012-08-02 07:10 PDT, Iain Merrick	no flags	Details Formatted Diff Diff
Patch (7.86 KB, patch) 2012-08-07 06:53 PDT, Iain Merrick	no flags	Details Formatted Diff Diff
Patch (13.00 KB, patch) 2012-08-08 05:58 PDT, Iain Merrick	no flags	Details Formatted Diff Diff
Patch (11.53 KB, patch) 2012-08-09 09:04 PDT, Iain Merrick	no flags	Details Formatted Diff Diff
Archive of layout-test-results from gce-cr-linux-05 (304.99 KB, application/zip) 2012-08-09 12:47 PDT, WebKit Review Bot	no flags	Details
cleaned up patch, but no test (4.73 KB, patch) 2012-08-10 12:17 PDT, Adam Barth	no flags	Details Formatted Diff Diff
fixed ChangeLog (4.73 KB, patch) 2012-08-10 12:22 PDT, Adam Barth	no flags	Details Formatted Diff Diff
Show Obsolete (9) View All Add attachment proposed patch, testcase, etc.