Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in mozilla15

Status

()

Core
Selection
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Blair, Assigned: Mats Palmgren (vacation - back in August))

Tracking

({regression})

11 Branch
mozilla15
x86
All
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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

5 years ago
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
Blocks: 619273
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
OS: Mac OS X → All

Updated

5 years ago
Component: Untriaged → Selection
Product: Firefox → Core
QA Contact: untriaged → selection
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.
Assignee: nobody → matspal
Created attachment 619401 [details] [diff] [review]
fix+test

Add a Flush to make sure frames are created.
Attachment #619401 - Flags: review?(bugs)
(filed a separate bug 750070 to outparamdel GetPresShell)

Updated

5 years ago
Attachment #619401 - Flags: review?(bugs) → review+
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.
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);
 +
Attachment #619401 - Attachment is obsolete: true
Attachment #619423 - Flags: review?(Ms2ger)
Comment on attachment 619423 [details] [diff] [review]
fix+test, v2

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

lgtm
Attachment #619423 - Flags: review?(Ms2ger) → review+

Comment 8

5 years ago
Hmm, so XPConnect doesn't do the right thing without EmptyString() ?
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.
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?
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)
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb125cc30252
Flags: in-testsuite+
Target Milestone: --- → mozilla15
(In reply to Olli Pettay [:smaug] from comment #11)
> (We should change the API to use DOMString)

filed bug 751785
https://hg.mozilla.org/mozilla-central/rev/eb125cc30252
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 758589
You need to log in before you can comment on or make changes to this bug.