| Summary: | [chromium] Upstream Android changes to WebFrameImpl::selectRange | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Iain Merrick <husky> | ||||||||||||||||||||||||
| Component: | WebKit API | Assignee: | Iain Merrick <husky> | ||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
| Severity: | Normal | CC: | abarth, dglazkov, hans, husky, japhet, mifenton, peter, rniwa, webkit.review.bot | ||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||
| Hardware: | Other | ||||||||||||||||||||||||||
| OS: | Android | ||||||||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||||||||
| Bug Blocks: | 66687, 93998 | ||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||
|
Description
Iain Merrick
2012-07-27 09:10:02 PDT
Created attachment 154971 [details]
Patch
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.
Created attachment 155229 [details]
Patch
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? 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. Created attachment 155518 [details]
Patch
Please make sure your patches block the meta bug: bug 66687 :). Thanks! 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. +rniwa, as this is about selection/editing. 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. Created attachment 155547 [details]
Patch
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. My comments above are just trivialities. @rniwa: Would you be willing to give this code a real review? 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. 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. 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. Thanks Iain. 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? Created attachment 156075 [details]
Patch
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). 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. (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? 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. (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. (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. (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. (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? (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. (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. See https://developer.mozilla.org/en-US/docs/DOM/Selection. "base"="anchor" and "extent"="focus" for all practical purposes (they are technically different). > > 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?
(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. 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? (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. 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. See EventHandler::updateSelectionForMouseDrag at http://trac.webkit.org/browser/trunk/Source/WebCore/page/EventHandler.cpp?124774#L780 for example. 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? (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. Great, thanks for your patience! Created attachment 156931 [details]
Patch
This code seems to work. Needs a test -- I'm looking at https://bugs.webkit.org/show_bug.cgi?id=93108 now. 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. 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. (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. Created attachment 157198 [details]
Patch
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). 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 > +<span id='target'>some test text for testing</span> 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. 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 >> +<span id='target'>some test text for testing</span> > > 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. (In reply to comment #48) > > >> Source/WebKit/chromium/tests/data/select_range_basic.html:1 > >> +<span id='target'>some test text for testing</span> > > > > 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. (In reply to comment #49) > (In reply to comment #48) > > > > >> Source/WebKit/chromium/tests/data/select_range_basic.html:1 > > >> +<span id='target'>some test text for testing</span> > > > > > > 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. Created attachment 157465 [details]
Patch
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. (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. 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. 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. (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 > >> +<span id='target'>some test text for testing</span> > > > > 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. 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? (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. 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 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
> 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.
@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. Created attachment 157785 [details]
cleaned up patch, but no test
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. Created attachment 157787 [details]
fixed ChangeLog
Comment on attachment 157787 [details] fixed ChangeLog Clearing flags on attachment: 157787 Committed r125329: <http://trac.webkit.org/changeset/125329> All reviewed patches have been landed. Closing bug. 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(). |