Closed
Bug 191864
Opened 22 years ago
Closed 13 years ago
Range and Selection broken with splitText() and normalize()
Categories
(Core :: DOM: Core & HTML, defect, P3)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla9
People
(Reporter: nate, Assigned: MatsPalmgren_bugz)
References
Details
(Keywords: dev-doc-complete, testcase, Whiteboard: [qa-])
Attachments
(5 files, 4 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
sicking
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3a) Gecko/20021207 Phoenix/0.5
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3a) Gecko/20021207 Phoenix/0.5
The DOM spec says that ranges should stay as constant as possible while the
document contents mutate
(http://www.w3.org/TR/DOM-Level-2-Traversal-Range/ranges.html#Level-2-Range-Mutation)
but range.splitText() and node.normalize() break this. And since the selection
object uses an underlying range, it too is broken.
After a splitText(), any part of the range or selection that should be in the
second (new) text node is not. After a normalize(), any part of the selection
or range not in (what was) the first text node is no longer selected or included.
Elaborate test case follows.
Elaborate test case follows.
Reproducible: Always
Steps to Reproduce:
Reporter | ||
Comment 1•22 years ago
|
||
Here's a test case with directions for reproducing the bug regarding the
interaction of range and selection objects with the range.splitText() and
node.normalize() methods.
anthonyd no longer works on range stuff.
Comment 4•20 years ago
|
||
Any chance of this bug being fixed for Gecko 1.8?
Comment 5•19 years ago
|
||
This testcase is partially invalid. Clicking on the buttons changes the
selection. You have to type in javascript:viewSelection() and friends manually
into the location bar.
With that taken into effect, this bug is WFM.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050811
SeaMonkey/1.0a
Reporter | ||
Comment 6•19 years ago
|
||
Thanks for looking at this. I'm the original reporter, but haven't really
thought about this since reporting it. I recall it was a showstopper for me at
the time, though.
Could you clarify exactly how it works for you, step by step in the directions?
You are saying that the range stays the same (as expected) when you type the
javascript: into the location bar, but changes when you use the buttons? Is
this necessary for all steps, or only some?
I still see the problem with Firefox 1.0, but don't have a current version to
try. And is there any reason that clicking on a button _should_ change the
selection? Because if so, that seems like a new bug that should be added and
cross-referenced. But if the problem has actually been fixed, great!
Updated•18 years ago
|
Keywords: helpwanted
FWIW, the way to fix this would be to add a notification to nsIMutationObserver that is sent out after the mutation is done that notifies ranges that a split or a normalize was just done and that the range needs to update itself.
Comment 8•17 years ago
|
||
Nominating for 1.9 since we've head from webdevs that this is a cause of issues for them.
Flags: blocking1.9?
Ideally we would get clarification from the w3c what the desired behavior is. The only thing that would indicate that this is an actual bug is the very fluffy paragraph:
There are two general principles which apply to Ranges under document mutation: The first is that all Ranges in a document will remain valid after any mutation operation and the second is that, as much as possible, all Ranges will select the same portion of the document after any mutation operation.
So it's not more than a "general pricipal". It's alway questionable what "any mutation" means. Is splitText() one or two mutations? We've implemented it on top of the normal DOM APIs which forces it to be two. Which is why this bug exists.
Comment 10•17 years ago
|
||
I know I'm not the w3c, but the quoted paragraph seems to pretty matter-of-factly explain why this is a bug, and the assertion of ambiguity provided by Jonas is not convincing to me.
"any mutation" means "any mutation operation provided by the DOM". Of course JS developers can write mutations that will move information outside of the view of the text-range code, and thus break range tracking. This is why it's so important for the built-in text range APIs to be comprehensive, and to have them retain ranges across operations. With this, JS developers can properly use the range APIs and avoid breaking selection or existing ranges.
The current splitText() is implemented as if an external JS developer had to force implement it without connection to the underlying ranges. In order to fit in with the "general principal" it should be made an operation which works in concert with the range-code and behaves as other range operations do.
Updated•17 years ago
|
Assignee: kinmoz → nobody
QA Contact: nobody → traversal-range
Not a blocker since this isn't a regression. But it'd certainly be nice to have fixed.
Flags: blocking1.9? → blocking1.9-
Whiteboard: [wanted-1.9]
Comment 12•17 years ago
|
||
Any ambiguity in the spec should be cleared up by the next 2 sections: Insertions and Deletion. The text says that:
"Any mutation of the document tree which affect Ranges can be considered to be a combination of basic deletion and insertion operations. In fact, it can be convenient to think of those operations as being accomplished using the deleteContents() and insertNode() Range methods and, in the case of Text mutations, the splitText() and normalize() methods."
so inserting an empty span, which consists of a splitText and an insertNode, should, as demonstrated by the examples leave the selection intact.
Comment 13•17 years ago
|
||
The spec doesn't actually define how to handle a splitText(). If a splitText() consists of a truncation of one text node and the insertion of another, then the selection should end up not containing the second part.
At least that's what I think the spec says. It seems dumb to me.
Updated•17 years ago
|
Flags: wanted1.9+
Whiteboard: [wanted-1.9]
Comment 14•17 years ago
|
||
Yes, it does:
There are two general principles which apply to Ranges under document mutation: The first is that all Ranges in a document will remain valid after any mutation operation and the second is that, as much as possible, all Ranges will select the same portion of the document after any mutation operation.
The text in the document doesn't change after a splitText, so neither should the selection.
Assignee | ||
Comment 15•13 years ago
|
||
This updates the Range boundaries as needed for splitText() and normalize().
Assignee: nobody → matspal
Attachment #549964 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 16•13 years ago
|
||
This is just a temporary workaround until we move the "selected bit" to
content (bug 619273). I guess we could skip this part until bug 619273
is fixed if we think we can live with the painting error until then...
It's needed for the reftest to pass though.
Attachment #549969 -
Flags: review?(roc)
Assignee | ||
Comment 17•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
I think we can skip the workaround.
Comment 19•13 years ago
|
||
Comment on attachment 549964 [details] [diff] [review]
part 1, update Range
Going to punt this to sicking...
Jonas, if you really want me to review this, let me know.
Attachment #549964 -
Flags: review?(bzbarsky) → review?(jonas)
I actually think that what we're currently doing is the correct behavior per spec.
However I agree that this new behavior is more useful and is what should be specced. Could someone make sure that the new DOM Core spec is updated to reflect this behavior.
Also, please write mochitests that work directly with the DOM that checks that a range has the correct state after a splitText and normalize call for all the various edge cases here.
Comment 21•13 years ago
|
||
I think the new range spec is specifying this new behavior.
Comment on attachment 549964 [details] [diff] [review]
part 1, update Range
Review of attachment 549964 [details] [diff] [review]:
-----------------------------------------------------------------
r=me, but I'd like to also review a mochitest that tests through the various edge cases.
::: content/base/public/nsIMutationObserver.h
@@ +96,5 @@
> +
> + struct Details {
> + enum {
> + kMerge, // two text nodes are merged as a result of normalize()
> + kSplit // a text node is split as a result of splitText()
Nit: I think we usually use 'e' as a prefix for enums. But I don't care that strongly.
Attachment #549964 -
Flags: review?(jonas) → review+
Assignee | ||
Comment 23•13 years ago
|
||
Comment on attachment 549969 [details] [diff] [review]
part 2, make newly created text frames draw as selected if they are
(In reply to comment #18)
> I think we can skip the workaround.
Ok, I think it should be easy enough to workaround in script, at least
for the common case of the primary selection. Eg. copy all the ranges
to an array, removeAllRanges(), add them back with addRange().
Attachment #549969 -
Attachment is obsolete: true
Attachment #549969 -
Flags: review?(roc)
Assignee | ||
Comment 24•13 years ago
|
||
> Nit: I think we usually use 'e' as a prefix for enums. But I don't care that
> strongly.
Fixed.
Attachment #550898 -
Flags: review+
Assignee | ||
Comment 25•13 years ago
|
||
Added workaround for painting bug per comment 23.
Attachment #549971 -
Attachment is obsolete: true
Assignee | ||
Comment 26•13 years ago
|
||
Brief overview:
1. save original text nodes in document.original_nodes
2. setup the selection
3. perform splitText() and/or normalize()
4. verify that most child nodes are the same as original_nodes, their
textContent is expected, and that the number of child nodes is
the expected
5. verify Selection range nodes/offsets
Attachment #550904 -
Flags: review?(jonas)
Comment on attachment 550904 [details] [diff] [review]
part 3, mochitest
Review of attachment 550904 [details] [diff] [review]:
-----------------------------------------------------------------
This test is a lot more complicated than it needs to be. First off, testing ranges doesn't need to involve selection. It feels much safer to me to use a plain range object that doesn't involve selection at all.
Second, why do you need to mess with frames and such?
Third, why not have a simple for loop that runs through all tests? There seems to be a lot of infrastructure there just to drive what is essentially just running through an array of test parameters.
Assignee | ||
Comment 28•13 years ago
|
||
> First off, testing
> ranges doesn't need to involve selection. It feels much safer to me to use a
> plain range object that doesn't involve selection at all.
Fixed - I removed all uses of Selection, just using Range now.
> Second, why do you need to mess with frames and such?
The mochitest was derived from the earlier reftest that needed separate
Selections to test the rendering of each case independently.
Fixed - I removed all use of IFRAMEs.
> Third, why not have a simple for loop that runs through all tests?
Fixed.
Attachment #550904 -
Attachment is obsolete: true
Attachment #552878 -
Flags: review?(jonas)
Attachment #550904 -
Flags: review?(jonas)
Comment on attachment 552878 [details] [diff] [review]
part 3, mochitest
Review of attachment 552878 [details] [diff] [review]:
-----------------------------------------------------------------
::: layout/generic/test/test_selection_splitText-normalize.html
@@ +49,5 @@
> + }
> +
> + // setup the range
> + let sel = t[0]
> + let startNode = sel.startNode === undefined ? p.firstChild : p.childNodes[sel.startNode];
None of the tests set startNode. Is that intentional?
@@ +59,5 @@
> +
> + // splitText
> + let split = t[1]
> + let c = p.childNodes[split[0]];
> + p.childNodes[split[0]].splitText(split[1])
c is unused
@@ +94,5 @@
> + [ {endNode:2}, [ [0, "0"], [1, "1234"], "5678" ]],
> + [ {endNode:3}, [ [0, "0"], [1, "1234"], "5", [2, "678"] ]],
> +/* test_split_merge */
> + [ {}, [ [0, "012345678" ] ]],
> + [ {startParent:true}, [ "012345678" ]], /* hmm, shouldn't this normalize back into the original child[0] ? */
If I understand this test correctly, this ends up making the original textnode be an empty textnode placed a newly created textnode which contains "012345678". normalize() is defined to toss out all empty textnodes and merge remaining adjecent textnodes.
Though I agree that that results in behavior which is somewhat inconsistent. Let me know if you want me to raise this on the webapps list.
Assignee | ||
Comment 30•13 years ago
|
||
(In reply to Jonas Sicking (:sicking) from comment #29)
> None of the tests set startNode. Is that intentional?
Yeah, it wasn't needed for any test. Fixed:
+ let startNode = p.firstChild;
> c is unused
Removed.
> If I understand this test correctly, this ends up making the original
> textnode be an empty textnode placed a newly created textnode which contains
> "012345678". normalize() is defined to toss out all empty textnodes and
> merge remaining adjecent textnodes.
Correct. I changed the comment to just document why it's different:
/* splitText() creates an empty first child which is removed by normalize() */
> Though I agree that that results in behavior which is somewhat inconsistent.
> Let me know if you want me to raise this on the webapps list.
I don't think we should change it at this point.
We're compatible with Chrome and Opera on this.
IE seems to always create a new text node when normalizing.
Attachment #552878 -
Attachment is obsolete: true
Attachment #553009 -
Flags: review?(jonas)
Attachment #552878 -
Flags: review?(jonas)
Comment 31•13 years ago
|
||
Could you please email public-webapps@w3.org with the suggested changes Jonas?
Assignee | ||
Comment 32•13 years ago
|
||
I pushed the fix + reftest to inbound to get it fixed for mozilla8:
http://hg.mozilla.org/integration/mozilla-inbound/rev/4d046f0cdcbd
http://hg.mozilla.org/integration/mozilla-inbound/rev/4c6dfeb5dc3a
Flags: in-testsuite+
Whiteboard: [inbound][mochitest needs review:sicking]
Target Milestone: Future → mozilla8
http://hg.mozilla.org/mozilla-central/rev/4d046f0cdcbd
http://hg.mozilla.org/mozilla-central/rev/4c6dfeb5dc3a
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound][mochitest needs review:sicking] → [mochitest needs review:sicking]
Updated•13 years ago
|
Keywords: dev-doc-needed
Comment 34•13 years ago
|
||
Since this appears to just be a bug fix, rather than a change to expected behavior, I've simply listed this fix on Firefox 8 for developers. If you feel that more is needed, please let me know.
Keywords: dev-doc-needed → dev-doc-complete
Assignee | ||
Comment 35•13 years ago
|
||
I filed bug 684661 on backing out this change from aurora, for causing regressions.
Assignee | ||
Updated•13 years ago
|
Target Milestone: mozilla8 → mozilla9
Updated•13 years ago
|
Whiteboard: [mochitest needs review:sicking] → [mochitest needs review:sicking], [qa-]
Assignee | ||
Comment 36•13 years ago
|
||
Comment on attachment 553009 [details] [diff] [review]
part 3, mochitest
Olli, perhaps you could review this one as well?
It passes with the latest patches in bug 682463.
Attachment #553009 -
Flags: review?(jonas) → review?(Olli.Pettay)
Assignee | ||
Updated•13 years ago
|
Whiteboard: [mochitest needs review:sicking], [qa-] → [mochitest needs review:smaug], [qa-]
Comment 37•13 years ago
|
||
Comment on attachment 553009 [details] [diff] [review]
part 3, mochitest
>+ if (sel.todo) {
>+ alert(r.startContainer + '\n' + r.startOffset + '\n' + r.endContainer + '\n' + r.endOffset + '\n')
>+ return;
>+ }
Remove or comment out this.
Kind of hard to review without comments.
rs=me
Attachment #553009 -
Flags: review?(Olli.Pettay) → review+
Assignee | ||
Comment 38•13 years ago
|
||
Landed the mochitest:
https://hg.mozilla.org/integration/mozilla-inbound/rev/55f620801bc7
Whiteboard: [mochitest needs review:smaug], [qa-] → [qa-]
Comment 39•13 years ago
|
||
Comment 40•13 years ago
|
||
Did this re-land after the previous backing out? If so, on which channel?
Comment 41•13 years ago
|
||
(In reply to Eric Shepherd [:sheppy] from comment #40)
> Did this re-land after the previous backing out? If so, on which channel?
This originally landed in Firefox 8, but it was backed out from Firefox 8 while Firefox 8 was on Aurora.
The patches are still in Firefox 9 (currently Aurora) and 10 (Nightly), because they were never backed out on the mozilla-central/nightly channel.
Updated•12 years ago
|
Component: DOM: Traversal-Range → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•