644 bytes, text/html
603 bytes, text/html
11.02 KB, text/plain
3.45 KB, patch
|Details | Diff | Splinter Review|
Looks like this regressed in this range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=54184cfa6f0e&tochange=9f412256da4c Bisecting now.
Status: UNCONFIRMED → NEW
Component: Layout: Form Controls → Graphics
Ever confirmed: true
QA Contact: layout.form-controls → thebes
Hg bisect says: The first bad revision is: changeset: 60477:b6384e736df2 user: Mats Palmgren <email@example.com> date: Fri Jan 14 01:22:26 2011 +0100 summary: Bug 602331 - selection addRange cannot select nodes that are being dynamically appended to the DOM. r=roc a=blocking2.0:final
At a guess, we're now flushing frames at some point when things don't work happily for some reason....
Component: Graphics → Layout: Form Controls
OS: Windows 7 → All
See Also: → bug 642977
(In reply to comment #5) > At a guess, we're now flushing frames at some point when things don't work > happily for some reason.... Could that be the flush added in that patch? ;-)
Well, that addition is what the guess was based on, yes.
Created attachment 520365 [details] nested nsTextEditorState::PrepareEditor The flush leads to a nested nsTextEditorState::PrepareEditor which protects against that (added in bug 577518). http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsTextEditorState.cpp#1161
Created attachment 520366 [details] [diff] [review] wip1 (wdiff) It's the script runner at the end that restores the selection state that causes it. Would it be safe to allow that part to nest?
(In reply to comment #9) > Created attachment 520366 [details] [diff] [review] > wip1 (wdiff) > > It's the script runner at the end that restores the selection state > that causes it. Would it be safe to allow that part to nest? No. Is the problem solved by adding a script blocker inside RestoreSelectionState::Run before calling AddRange?
Created attachment 520492 [details] [diff] [review] fix v2, with reftest
Hardware: x86_64 → All
Version: unspecified → Trunk
Comment on attachment 520492 [details] [diff] [review] fix v2, with reftest The code change looks good, but please add a comment on why the script blocker is necessary. I don't think you need the spellcheck="false" in the test, though. Please remove it.
Attachment #520492 - Flags: review?(ehsan) → review+
Also, it would be great if you could move the reftest to layout/reftests/editor, which would make it easier for me to run this reftest on editor changes (since it's really about the editor).
Added a code comment, removed the @spellcheck, and moved the tests to layout/reftests/editor FTR, that spellcheck=false thing is to avoid misspelled word underlines which used to be added asynchronously (at least on some platforms) which could cause oranges in reftests. I'll just have to spell correctly for tests to pass now I guess... :-) Fixed in Cedar: http://hg.mozilla.org/projects/cedar/rev/974a47b48836
Flags: in-testsuite? → in-testsuite+
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.2
Comment on attachment 520492 [details] [diff] [review] fix v2, with reftest Low-risk regression fix for mozilla-2.0 branch.
Attachment #520492 - Flags: approval2.0?
Is anyone triaging approval2.0? flags? did I use the wrong flag?
Attachment #520492 - Flags: approval2.0? → approval2.0-
You need to log in before you can comment on or make changes to this bug.