Closed Bug 602331 Opened 14 years ago Closed 14 years ago

selection addRange cannot select nodes that are being dynamically appended to the DOM

Categories

(Core :: DOM: Selection, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla2.0b10
Tracking Status
blocking2.0 --- final+

People

(Reporter: blgroup, Assigned: MatsPalmgren_bugz)

References

Details

(Keywords: regression, testcase, Whiteboard: [hardblocker](?))

Attachments

(2 files)

User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.10) Gecko/20100914 Firefox/3.6.10 Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b6) Gecko/20100101 Firefox/4.0b6 Trying to select new nodes dynamically via window window.getSelection().addRange, the nodes will not be added to the current selection. If the nodes already exist in the dom, then they can be added to the current selection. This issue occurs in Firefox 4. It does not occur in Firefox 3.6.10 Reproducible: Always Steps to Reproduce: 1. Load the attached test page 2. Press "Select Newly Added" Actual Results: Newly added node 0 will not be selected Expected Results: Newly added node 0 will be selected Loading the test page in Firefox 3 will demonstrate the expected results
Attached file Test page to demonstrate the bug (deleted) —
Version: unspecified → Trunk
Ehsan, any idea what's up here? Are we missing a frame flush somewhere in selection code and losing due to lazy frame construction?
Status: UNCONFIRMED → NEW
blocking2.0: --- → ?
Ever confirmed: true
Looks very much like lazy frame construction is in play there. Try clicking the second button, and then click the third one. The only node which is not selected is the one which is just created (and wouldn't have frames unless selectNode flushes).
I don't think selectNode should flush; that's called for all sorts of internal programmatic stuff, right?. Perhaps addRange should?
(In reply to comment #4) > I don't think selectNode should flush; that's called for all sorts of internal > programmatic stuff, right?. Perhaps addRange should? What if the script tries to inspect the contents of the range without adding it to a selection?
Inspect in what way? The range is fine; it's just the visual selection display that's wrong, no? Which, btw, suggests that it would also be wrong if content in the middle of a selection range went from display:none to being visible....
(In reply to comment #6) > Inspect in what way? The range is fine; it's just the visual selection display > that's wrong, no? > > Which, btw, suggests that it would also be wrong if content in the middle of a > selection range went from display:none to being visible.... It is not just the visual selection. If you were to then try to access the contents of the current selection eg. window.getSelection().toString() you will see that the new node is not contained within the selection.
That's ... odd. Do the selection ranges look wrong too?
Hmm, maybe this is only a painting problem? Try this: load the test case, click "Select Newly Added", switch to another tab and switch back to this tab, and boom! The selection is there! (Note: all I'm doing here is speculating based on the evidence, I haven't really debugged this).
I also tried to modify the test case to just alert range.toString() before adding it to the selection, and indeed the selection seems to be fine.
Disabling lazy frame construction seems to fix the problem. (For reference that can be done easily by putting "return PR_FALSE;" at the very start of nsCSSFrameConstructor::MaybeConstructLazily.)
Yeah, that makes sesne; selectFrames can't find the frames and then we don't deal with them being created. See also comment 6 second paragraph. ;)
Hmm, so nsTypedSelection::selectFrames used to have a flush in it, but it was removed in bug 527306.
That wouldn't help comment 6 second paragraph, right?
Severity: major → critical
This bug seems at least as critical as bug 527306. There are a lot of sites on the web where copy and paste is now broken in FF4 because of this bug. eg. http://www.myspace.com/
Whoops, not myspace.com I meant http://sportsillustrated.cnn.com/
Do we maybe want to go with a less invasive fix that just fixes the regression wrt 3.6 instead of fixing all the problems here?
What are the steps to reproduce the problem on http://sportsillustrated.cnn.com/ ? I just tried and it _seemed_ to work fine for me.
> Do we maybe want to go with a less invasive fix that just fixes the regression Sure, though I'd like to see some evidence that there's actual breakage before we start messing with adding complexity here...
Well the visual problem is confirmed at least.
I just selected and then copied the text at the very bottom of sportsillustrated.cnn.com using the latest beta and when I tried to paste into Notepad or Wordpad nothing happened. Nothing got pasted. I did the exact same thing using 3.6.8 and it worked fine. Looks broken to me. Text copied: "SI.com is part of Turner - SI Digital, part of the Turner Sports & Entertainment Digital Network. Copyright © 2010 Time Inc. A Time Warner Company. All Rights Reserved"
I tried doing that in the current nightly build (2010-10-13) and it worked fine for me. Could you try the latest nightly build (you can get it from http://nightly.mozilla.org/) and see if the issue is still present there? It could have been fixed since the last beta was released.
Timothy: I just installed the nightly on Windows 7 and OS X 10.6.4 and you're right, it worked fine. The demo submitted with the initial bug report still doesn't work as expected though.
Ok, great, thanks for testing. I can reproduce the problem in the testcase attached to this bug. I was trying trying to determine the nature of the problem because what you were reporting at the sportsillustrated site was different from the problem in the testcase attached to this bug.
I think we should add a flush somewhere that doesn't regress 527306.
Assignee: nobody → matspal
blocking2.0: ? → final+
Setting selection state for arbitrary frame creation seems hard to do without regressing perf. To fix problems like paragraph 2 in comment #6, we should move selection state to content.
Ehsan might be a good person to work on comment #26, but obviously it's post-FF4.
(In reply to comment #26) > Setting selection state for arbitrary frame creation seems hard to do without > regressing perf. To fix problems like paragraph 2 in comment #6, we should move > selection state to content. Hmm, are you talking about the editor selection state? <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/nsSelectionState.h#71>
But the testcase is about selection of frames that aren't under an editor.
Or the stuff in nsTypedSelection::selectFrames?
I'm taking about the NS_FRAME_IS_SELECTED bit.
(In reply to comment #27) > Ehsan might be a good person to work on comment #26, but obviously it's > post-FF4. I filed bug 619273 for that.
Whiteboard: [hardblocker](?)
Probably caused by removing the flush in bug 527306.
Blocks: 527306
Attached patch fix + reftests (deleted) — Splinter Review
Try server results still pending...
Attachment #502775 - Flags: review?(roc)
Comment on attachment 502775 [details] [diff] [review] fix + reftests + if (presShell) + presShell->FlushPendingNotifications(Flush_Frames); {}
Attachment #502775 - Flags: review?(roc) → review+
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b10
Depends on: 626014
Depends on: 626288
Depends on: 642800
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: