Bug 23601 - DOMSelection.getRangeAt() returns a different range than the selection
Summary: DOMSelection.getRangeAt() returns a different range than the selection
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 23600
  Show dependency treegraph
 
Reported: 2009-01-28 13:56 PST by Eric Seidel (no email)
Modified: 2009-02-06 17:29 PST (History)
5 users (show)

See Also:


Attachments
test case (similar to 23600) (1.07 KB, text/html)
2009-01-28 13:57 PST, Eric Seidel (no email)
no flags Details
Slightly clearer diff, and added documentation of ASSERTS ojan and I kept hitting in this code. (14.83 KB, patch)
2009-02-05 18:00 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Rename toRange to toNormalizedRange and add new firstRange which returns an unmodified range (40.60 KB, patch)
2009-02-05 18:00 PST, Eric Seidel (no email)
justin.garcia: review+
Details | Formatted Diff | Diff
With ap's suggested addition of rangeCompliantEquivalent and some documentation of that function. (2.13 KB, patch)
2009-02-06 10:29 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2009-01-28 13:56:50 PST
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
Comment 1 Eric Seidel (no email) 2009-01-28 13:57:27 PST
Created attachment 27122 [details]
test case (similar to 23600)
Comment 2 Justin Garcia 2009-01-28 14:53:07 PST
I think that this general issue is filed elsewhere let me see if I can find it...
Comment 3 Justin Garcia 2009-01-28 15:11:50 PST
Related but not the same bug:

https://bugs.webkit.org/show_bug.cgi?id=6350
Comment 4 Eric Seidel (no email) 2009-02-05 13:58:30 PST
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.
Comment 5 Eric Seidel (no email) 2009-02-05 14:00:19 PST
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.
Comment 6 Justin Garcia 2009-02-05 14:20:04 PST
(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().
Comment 7 Alexey Proskuryakov 2009-02-05 14:30:41 PST
(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.
Comment 8 Eric Seidel (no email) 2009-02-05 14:36:23 PST
(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().
Comment 9 Alexey Proskuryakov 2009-02-05 15:03:54 PST
Sorry, I was looking at another bug's test case.
Comment 10 Eric Seidel (no email) 2009-02-05 17:55:00 PST
I have a local fix, will post shortly.
Comment 11 Eric Seidel (no email) 2009-02-05 18:00:27 PST
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(-)
Comment 12 Eric Seidel (no email) 2009-02-05 18:00:30 PST
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 13 Eric Seidel (no email) 2009-02-05 18:00:58 PST
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 14 Justin Garcia 2009-02-05 18:41:37 PST
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
Comment 15 Eric Seidel (no email) 2009-02-06 10:27:08 PST
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.
Comment 16 Eric Seidel (no email) 2009-02-06 10:29:26 PST
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 17 Eric Seidel (no email) 2009-02-06 16:21:13 PST
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.
Comment 18 Eric Seidel (no email) 2009-02-06 17:29:23 PST
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...