Bug 23601 – DOMSelection.getRangeAt() returns a different range than the selection

• Component: HTML Editing
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

Created attachment 27122 [details]
test case (similar to 23600)

[Justin Garcia]

I think that this general issue is filed elsewhere let me see if I can find it...

[Justin Garcia]

Related but not the same bug:

https://bugs.webkit.org/show_bug.cgi?id=6350

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Justin Garcia]

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

[Alexey Proskuryakov]

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

[Eric Seidel (no email)]

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

[Alexey Proskuryakov]

Sorry, I was looking at another bug's test case.

[Eric Seidel (no email)]

I have a local fix, will post shortly.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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. :)

[Justin Garcia]

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

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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