DOMSelection.getRangeAt() returns a different range than the selection Attached is the same test as bug 23600, except the text node is not empty. It's clear that our behavior is wrong here: comparing point with <text>, offset 0: FAILED: result = 1 expected 0 range: startContainer: [object HTMLDivElement] startOffset: 0 endContainer: [object HTMLDivElement] endOffset: 0 selection: anchorNode: [object Text] anchorOffset: 0 focusNode: [object Text] focusOffset: 0 asd
Created attachment 27122 [details] test case (similar to 23600)
I think that this general issue is filed elsewhere let me see if I can find it...
Related but not the same bug: https://bugs.webkit.org/show_bug.cgi?id=6350
Sigh. Well this is part of the problem: http://trac.webkit.org/browser/trunk/WebCore/editing/Selection.cpp#L141 Why oh why would toRange chose to modify the Selection instead of just returning the range? It seems like that modification code is just in the wrong place.
It seems the proper way to fix this is to write a version of toRange() which just returns the raw range (w/o modification) and then check all the callers to toRange() to see which behavior they want, and then maybe find a better name for the existing toRange logic.
(In reply to comment #5) > It seems the proper way to fix this is to write a version of toRange() which > just returns the raw range (w/o modification) and then check all the callers to > toRange() to see which behavior they want, and then maybe find a better name > for the existing toRange logic. Notice also that selection endpoints may change at creation time, inside validate().
(In reply to comment #4) > Why oh why would toRange chose to modify the Selection instead of just > returning the range? I think the reason is that Selection positions may be invalid in DOM sense, so calling rangeCompliantEquivalent() is necessary. As Justin says, the validate() call during selection creation is the real culprit. See also: bug 15256 and the many related ones.
(In reply to comment #7) > (In reply to comment #4) > > Why oh why would toRange chose to modify the Selection instead of just > > returning the range? > > I think the reason is that Selection positions may be invalid in DOM sense, so > calling rangeCompliantEquivalent() is necessary. As Justin says, the validate() > call during selection creation is the real culprit. > > See also: bug 15256 and the many related ones. The real culprit for this bug is definitely toRange(). Looking at the test output, the selection is as expected, when it's converted to a range, since it's a "caret" it's pushed out of the empty text node by explicit code in toRange().
Sorry, I was looking at another bug's test case.
I have a local fix, will post shortly.
Created attachment 27372 [details] Slightly clearer diff, and added documentation of ASSERTS ojan and I kept hitting in this code. WebCore/ChangeLog | 36 ++++++++++++++++++++++ WebCore/editing/Editor.cpp | 16 +++++----- WebCore/editing/Selection.cpp | 52 +++++++++++++++++++------------- WebCore/editing/Selection.h | 36 ++++++++++++---------- WebCore/editing/SelectionController.h | 2 +- WebCore/editing/TypingCommand.cpp | 16 +++++----- WebCore/page/Frame.cpp | 16 ++++------ 7 files changed, 110 insertions(+), 64 deletions(-)
Created attachment 27373 [details] Rename toRange to toNormalizedRange and add new firstRange which returns an unmodified range LayoutTests/ChangeLog | 13 ++++ LayoutTests/fast/dom/Selection/getRangeAt.html | 13 ++++ .../fast/dom/Selection/resources/TEMPLATE.html | 13 ++++ .../fast/dom/Selection/resources/getRangeAt.js | 33 +++++++++ WebCore/ChangeLog | 77 ++++++++++++++++++++ WebCore/WebCore.base.exp | 2 +- WebCore/dom/InputElement.cpp | 2 +- WebCore/editing/DeleteButtonController.cpp | 2 +- WebCore/editing/Editor.cpp | 24 +++--- WebCore/editing/EditorCommand.cpp | 8 +- WebCore/editing/RemoveFormatCommand.cpp | 2 +- WebCore/editing/ReplaceSelectionCommand.cpp | 4 +- WebCore/editing/Selection.cpp | 25 +++---- WebCore/editing/Selection.h | 8 ++- WebCore/editing/SelectionController.h | 2 +- WebCore/editing/TypingCommand.cpp | 4 +- WebCore/editing/markup.cpp | 2 +- WebCore/loader/archive/cf/LegacyWebArchive.cpp | 2 +- WebCore/page/AccessibilityRenderObject.cpp | 2 +- WebCore/page/ContextMenuController.cpp | 4 +- WebCore/page/DOMSelection.cpp | 13 ++-- WebCore/page/DragController.cpp | 8 +- WebCore/page/EventHandler.cpp | 2 +- WebCore/page/Frame.cpp | 16 ++-- WebCore/platform/ContextMenu.cpp | 2 +- WebKit/mac/ChangeLog | 22 ++++++ WebKit/mac/WebView/WebFrame.mm | 8 +- WebKit/mac/WebView/WebHTMLView.mm | 6 +- WebKit/mac/WebView/WebView.mm | 4 +- 29 files changed, 250 insertions(+), 73 deletions(-)
Comment on attachment 27372 [details] Slightly clearer diff, and added documentation of ASSERTS ojan and I kept hitting in this code. Ooops attached the wrong patch. :)
Comment on attachment 27373 [details] Rename toRange to toNormalizedRange and add new firstRange which returns an unmodified range Looks great although simply "normalizedRange" might be better. r=me
Ap also reviewed and suggested that I add rangeCompliantEquivalent calls to firstRange(). I'm not sure that adding rangeCompliantEquivalent is correct in all cases (it seems to avoid having positions in tables!?) but it will fix any invalid (img, 1) positions (which I don't know how to generate), which sounds like a good thing. I'm going to leave it as toNormalizedRange() because I feel it better implies that the function does work. :) firstRange doesn't really need to do work, if we stored start/end as a Range. I'd like to discourage people from calling toNormalizedRange,cause I don't think it's what most callers actually need/want. I think the logic is misplaced.
Created attachment 27402 [details] With ap's suggested addition of rangeCompliantEquivalent and some documentation of that function. WebCore/editing/Selection.cpp | 4 +++- WebCore/editing/htmlediting.cpp | 15 ++++++++++----- 2 files changed, 13 insertions(+), 6 deletions(-)
Comment on attachment 27402 [details] With ap's suggested addition of rangeCompliantEquivalent and some documentation of that function. Going to land the reviewed patch and then start removing this code entirely by starting to separate the concepts of Selection and VisibleSelection.
Committing to http://svn.webkit.org/repository/webkit/trunk ... M LayoutTests/ChangeLog A LayoutTests/fast/dom/Selection/getRangeAt.html A LayoutTests/fast/dom/Selection/resources/TEMPLATE.html A LayoutTests/fast/dom/Selection/resources/getRangeAt.js M WebCore/ChangeLog M WebCore/WebCore.base.exp M WebCore/dom/InputElement.cpp M WebCore/editing/DeleteButtonController.cpp M WebCore/editing/Editor.cpp M WebCore/editing/EditorCommand.cpp M WebCore/editing/RemoveFormatCommand.cpp M WebCore/editing/ReplaceSelectionCommand.cpp M WebCore/editing/Selection.cpp M WebCore/editing/Selection.h M WebCore/editing/SelectionController.h M WebCore/editing/TypingCommand.cpp M WebCore/editing/htmlediting.cpp M WebCore/editing/markup.cpp M WebCore/loader/archive/cf/LegacyWebArchive.cpp M WebCore/page/AccessibilityRenderObject.cpp M WebCore/page/ContextMenuController.cpp M WebCore/page/DOMSelection.cpp M WebCore/page/DragController.cpp M WebCore/page/EventHandler.cpp M WebCore/page/Frame.cpp M WebCore/platform/ContextMenu.cpp M WebKit/mac/ChangeLog M WebKit/mac/WebView/WebFrame.mm M WebKit/mac/WebView/WebHTMLView.mm M WebKit/mac/WebView/WebView.mm Committed r40746 Yay! Of course this code is about to get re-written...