Last Comment Bug 748961 - Ranges added with addRange to the current Selection (getSelection) are not immediately available in getSelection.toString()
: Ranges added with addRange to the current Selection (getSelection) are not i...
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Selection (show other bugs)
: 11 Branch
: x86 All
: -- normal with 1 vote (vote)
: mozilla15
Assigned To: Mats Palmgren (:mats)
:
Mentors:
Depends on: 758589
Blocks: 619273
  Show dependency treegraph
 
Reported: 2012-04-25 14:12 PDT by Blair
Modified: 2012-05-25 05:50 PDT (History)
5 users (show)
mats: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix+test (3.44 KB, patch)
2012-04-29 08:06 PDT, Mats Palmgren (:mats)
bugs: review+
Details | Diff | Review
fix+test, v2 (3.55 KB, patch)
2012-04-29 10:39 PDT, Mats Palmgren (:mats)
Ms2ger: review+
Details | Diff | Review

Description Blair 2012-04-25 14:12:40 PDT
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 2012-04-25 19:12:27 PDT
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 (:mats) 2012-04-25 19:46:40 PDT
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 (:mats) 2012-04-29 08:06:54 PDT
Created attachment 619401 [details] [diff] [review]
fix+test

Add a Flush to make sure frames are created.
Comment 4 Mats Palmgren (:mats) 2012-04-29 08:12:28 PDT
(filed a separate bug 750070 to outparamdel GetPresShell)
Comment 5 :Ms2ger 2012-04-29 09:56:25 PDT
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 (:mats) 2012-04-29 10:39:02 PDT
Created attachment 619423 [details] [diff] [review]
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 2012-04-29 10:40:27 PDT
Comment on attachment 619423 [details] [diff] [review]
fix+test, v2

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

lgtm
Comment 8 Olli Pettay [:smaug] 2012-04-29 11:04:59 PDT
Hmm, so XPConnect doesn't do the right thing without EmptyString() ?
Comment 9 Mats Palmgren (:mats) 2012-04-29 16:01:52 PDT
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 (:mats) 2012-04-29 16:10:23 PDT
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] 2012-04-29 23:23:30 PDT
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 13 Mats Palmgren (:mats) 2012-05-03 17:29:33 PDT
(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] 2012-05-04 10:55:02 PDT
https://hg.mozilla.org/mozilla-central/rev/eb125cc30252

Note You need to log in before you can comment on or make changes to this bug.