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)

defect

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)

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:
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.
Assignee: anthonyd → kin
Blocks: 30838
Priority: -- → P3
Target Milestone: --- → Future
.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Any chance of this bug being fixed for Gecko 1.8?
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
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!
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.
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.
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.
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]
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.
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.
Flags: wanted1.9+
Whiteboard: [wanted-1.9]
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.
Attached patch part 1, update Range (deleted) — Splinter Review
This updates the Range boundaries as needed for splitText() and normalize().
Assignee: nobody → matspal
Attachment #549964 - Flags: review?(bzbarsky)
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)
Attached patch part 3, tests (obsolete) (deleted) — Splinter Review
Depends on: 619273
Keywords: helpwantedtestcase
OS: Linux → All
Blocks: 673108
I think we can skip the workaround.
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.
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+
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)
Attached patch part 1, rev 2, update Range (deleted) — Splinter Review
> Nit: I think we usually use 'e' as a prefix for enums. But I don't care that > strongly. Fixed.
Attachment #550898 - Flags: review+
Attached patch part 2, reftests (deleted) — Splinter Review
Added workaround for painting bug per comment 23.
Attachment #549971 - Attachment is obsolete: true
Attached patch part 3, mochitest (obsolete) (deleted) — Splinter Review
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.
Attached patch part 3, mochitest (obsolete) (deleted) — Splinter Review
> 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.
Attached patch part 3, mochitest (deleted) — Splinter Review
(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)
Could you please email public-webapps@w3.org with the suggested changes Jonas?
Flags: in-testsuite+
Whiteboard: [inbound][mochitest needs review:sicking]
Target Milestone: Future → mozilla8
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound][mochitest needs review:sicking] → [mochitest needs review:sicking]
Depends on: 679459
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.
Depends on: 684661
Depends on: 682463
I filed bug 684661 on backing out this change from aurora, for causing regressions.
Target Milestone: mozilla8 → mozilla9
Whiteboard: [mochitest needs review:sicking] → [mochitest needs review:sicking], [qa-]
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)
Whiteboard: [mochitest needs review:sicking], [qa-] → [mochitest needs review:smaug], [qa-]
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+
Whiteboard: [mochitest needs review:smaug], [qa-] → [qa-]
Did this re-land after the previous backing out? If so, on which channel?
(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.
Depends on: 803924
Component: DOM: Traversal-Range → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: