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

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Blair]

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

Comment 1

[Blair]

Attachment: Test page to demonstrate the bug

Comment 2

[Boris Zbarsky [:bzbarsky]]

Ehsan, any idea what's up here?  Are we missing a frame flush somewhere in selection code and losing due to lazy frame construction?

Comment 3

[(no longer active)]

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).

Comment 4

[Boris Zbarsky [:bzbarsky]]

I don't think selectNode should flush; that's called for all sorts of internal programmatic stuff, right?.  Perhaps addRange should?

Comment 5

[(no longer active)]

(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?

Comment 6

[Boris Zbarsky [:bzbarsky]]

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....

Comment 7

[Blair]

(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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

That's ... odd.  Do the selection ranges look wrong too?

Comment 9

[(no longer active)]

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).

Comment 10

[(no longer active)]

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.

Comment 11

[Timothy Nikkel (:tnikkel)]

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.)

Comment 12

[Boris Zbarsky [:bzbarsky]]

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.  ;)

Comment 13

[Timothy Nikkel (:tnikkel)]

Hmm, so nsTypedSelection::selectFrames used to have a flush in it, but it was removed in bug 527306.

Comment 14

[Boris Zbarsky [:bzbarsky]]

That wouldn't help comment 6 second paragraph, right?

Comment 15

[Blair]

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/

Comment 16

[Blair]

Whoops,  not myspace.com

I meant http://sportsillustrated.cnn.com/

Comment 17

[Timothy Nikkel (:tnikkel)]

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?

Comment 18

[Timothy Nikkel (:tnikkel)]

What are the steps to reproduce the problem on http://sportsillustrated.cnn.com/ ? I just tried and it _seemed_ to work fine for me.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> 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...

Comment 20

[Timothy Nikkel (:tnikkel)]

Well the visual problem is confirmed at least.

Comment 21

[Jim]

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"

Comment 22

[Timothy Nikkel (:tnikkel)]

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.

Comment 23

[Jim]

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.

Comment 24

[Timothy Nikkel (:tnikkel)]

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.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we should add a flush somewhere that doesn't regress 527306.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Ehsan might be a good person to work on comment #26, but obviously it's post-FF4.

Comment 28

[(no longer active)]

(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>

Comment 29

[Timothy Nikkel (:tnikkel)]

But the testcase is about selection of frames that aren't under an editor.

Comment 30

[(no longer active)]

Or the stuff in nsTypedSelection::selectFrames?

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm taking about the NS_FRAME_IS_SELECTED bit.

Comment 32

[(no longer active)]

(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.

Comment 33

[Mats Palmgren (inactive)]

Probably caused by removing the flush in bug 527306.

Comment 34

[Mats Palmgren (inactive)]

Attachment: fix + reftests

Try server results still pending...

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 502775 [details] [diff] [review]
fix + reftests

+  if (presShell)
+    presShell->FlushPendingNotifications(Flush_Frames);

{}

Comment 36

[Mats Palmgren (inactive)]

http://hg.mozilla.org/mozilla-central/rev/b6384e736df2