748961 - Ranges added with addRange to the current Selection (getSelection) are not immediately available in getSelection.toString()

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Blair]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:12.0) Gecko/20100101 Firefox/12.0
Build ID: 20120420145725

Steps to reproduce:

Create a range from a node, then add it to the current selection.

This data URL will demonstrate the issue:

data:text/html,<p id="sel">Selection</p><script>var range = document.createRange(); range.selectNode(document.getElementById('sel')); getSelection().addRange(range); console.log('inline: ', getSelection().toString()); setTimeout(function() {console.log('after: ', getSelection().toString())},2)</script>

Notice in Firefox 10 / 9  versus Firefox 11 / 12


Actual results:

In Firefox 11 + the node is only available in getSelection().toString() some unknown time after the function completes (in the setTimeout call), but this can be inconsistent on Firefox 12 where the selected node still may be missing.


Expected results:

In Firefox 10 - getSelection().toString() returns the text in the selected nodes including the immediately appended range.

Comment 1

[Alice0775 White]

Regression window(m-c)
Works:
http://hg.mozilla.org/mozilla-central/rev/c7101dec8deb
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111220 Firefox/11.0a1 ID:20111220083550
Fails:
http://hg.mozilla.org/mozilla-central/rev/a8506ab2c654
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111220 Firefox/11.0a1 ID:20111220085450
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c7101dec8deb&tochange=a8506ab2c654


Regression window(m-c)
Works:
http://hg.mozilla.org/integration/mozilla-inbound/rev/feaccb6a4c35
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111219 Firefox/11.0a1 ID:20111219235256
Fails:
http://hg.mozilla.org/integration/mozilla-inbound/rev/0aa9c3a5b7e1
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111219 Firefox/11.0a1 ID:20111220011653
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=feaccb6a4c35&tochange=0aa9c3a5b7e1

Regressed by: Bug 619273

Comment 2

[Mats Palmgren (inactive)]

Yeah, I guess frames haven't been created yet at the point the script runs.
Adding "document.body.offsetHeight" before the first .toString() fixes it.
We need to add a Flush somewhere... I can take a look.

Comment 3

[Mats Palmgren (inactive)]

Attachment: fix+test

Add a Flush to make sure frames are created.

Comment 4

[Mats Palmgren (inactive)]

(filed a separate bug 750070 to outparamdel GetPresShell)

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 619401 [details] [diff] [review]
fix+test

Review of attachment 619401 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsSelection.cpp
@@ +1317,5 @@
> +  // the selected content.
> +  nsCOMPtr<nsIPresShell> shell;
> +  nsresult rv = GetPresShell(getter_AddRefs(shell));
> +  if (NS_FAILED(rv) || !shell) {
> +    return NS_OK;

Surely you should return something here.

Comment 6

[Mats Palmgren (inactive)]

Attachment: fix+test, v2

Oops, sorry about that.  Here's the intradiff:

  nsTypedSelection::ToString(PRUnichar **aReturn)
  {
++  if (!aReturn) {
++    return NS_ERROR_NULL_POINTER;
++  }
 +  // We need Flush_Style here to make sure frames have been created for
 +  // the selected content.
 +  nsCOMPtr<nsIPresShell> shell;
 +  nsresult rv = GetPresShell(getter_AddRefs(shell));
 +  if (NS_FAILED(rv) || !shell) {
++    *aReturn = ToNewUnicode(EmptyString());
 +    return NS_OK;
 +  }
 +  shell->FlushPendingNotifications(Flush_Style);
 +

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 619423 [details] [diff] [review]
fix+test, v2

Review of attachment 619423 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Hmm, so XPConnect doesn't do the right thing without EmptyString() ?

Comment 9

[Mats Palmgren (inactive)]

I'm not sure what you're asking, the method is expected to return a newly
allocated string.  If I don't assign *aReturn and return NS_OK then the
result in JS is null, which is wrong.

Comment 10

[Mats Palmgren (inactive)]

I guess I could return an error code there to avoid the whole issue
with the out param, but I think toString() methods in general is
expected to not throw and instead return an empty string.
(I see ToStringWithFormat doesn't do that, but that seems wrong...)

Ms2ger?

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

I think the current patch is ok. I'm just a bit surprised how PRUnichar result behaves.
(We should change the API to use DOMString)

Comment 12

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb125cc30252

Comment 13

[Mats Palmgren (inactive)]

(In reply to Olli Pettay [:smaug] from comment #11)
> (We should change the API to use DOMString)

filed bug 751785

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/eb125cc30252