Last Comment Bug 636512 - document.getSelection() should return the same as getSelection(), not stringification
: document.getSelection() should return the same as getSelection(), not stringi...
Status: RESOLVED FIXED
[good first bug]
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- minor (vote)
: mozilla8
Assigned To: Vishnu S (:vishnus)
:
:
Mentors:
Depends on: 690997 692412
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-24 10:38 PST by Aryeh Gregor (:ayg) (away until October 25)
Modified: 2012-06-30 09:30 PDT (History)
11 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
will change DOMString to nsISelection (763 bytes, patch)
2011-07-06 09:00 PDT, Vishnu S (:vishnus)
no flags Details | Diff | Splinter Review
Changed implementation :) (2.40 KB, patch)
2011-07-06 09:28 PDT, Vishnu S (:vishnus)
bzbarsky: review-
Details | Diff | Splinter Review
Corrected the build issue, changed the UUID (3.19 KB, patch)
2011-07-07 09:44 PDT, Vishnu S (:vishnus)
bzbarsky: review+
Details | Diff | Splinter Review

Description Aryeh Gregor (:ayg) (away until October 25) 2011-02-24 10:38:09 PST
Per spec:

"For historical reasons, the getSelection() method on the Document interface must return the same Selection object as its defaultView's getSelection() method."
http://html5.org/specs/dom-range.html#dom-document-getselection

This is how WebKit (Chrome 10 dev) and IE 9 RC implement it.  Firefox 4b11 and Opera 11 return a string.  Test case:

data:text/html,<!doctype html>
<script>alert(typeof document.getSelection())</script>

Alerts "string" in Firefox and Opera, "object" in IE and WebKit.  The behavior of the original API was actually to return a string (see bug 32481), but the specced behavior seems simpler, more convenient, and more intuitive, unless it will cause compat problems.  If Gecko doesn't want to change this, the spec should probably change.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2011-02-24 10:44:53 PST
I think making this change should be fine (post-2.0).

Looks like just changing the idl and removing the stringification might be good enough.

Should we take out the warning too?
Comment 2 Aryeh Gregor (:ayg) (away until October 25) 2011-02-24 14:14:52 PST
I don't see any reason for the warning.
Comment 3 :Ms2ger (⌚ UTC+1/+2) 2011-04-16 08:06:31 PDT
toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_595934_message_categories.js includes a test for the warning that will need to be removed.
Comment 4 Aryeh Gregor (:ayg) (away until October 25) 2011-06-19 10:05:54 PDT
Fun gotcha I found that this behavior causes: if you use "getSelection()" to retrieve the Selection object (instead of "window.getSelection()"), it will work everywhere except in event handlers, where it will mean document.getSelection() instead of window.getSelection().  I just hit that in a test-case, and I'd have been scratching my head for an awfully long time over it if I didn't know about this quirk.
Comment 5 Vishnu S (:vishnus) 2011-07-06 09:00:17 PDT
Created attachment 544259 [details] [diff] [review]
will change DOMString to nsISelection

This is my first patch :) thanks to Ms2ger for the guidelines :) hope this is the right one :)
Comment 6 Vishnu S (:vishnus) 2011-07-06 09:28:27 PDT
Created attachment 544270 [details] [diff] [review]
Changed implementation :)

Implementation part changed. Again thanks to Ms2ger :)
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2011-07-06 09:44:45 PDT
Comment on attachment 544270 [details] [diff] [review]
Changed implementation :)

This needs a checkin comment and a User or From line, but more importantly needs to rev the nsIDOMHTMLDocument iid.
Comment 8 Vishnu S (:vishnus) 2011-07-07 09:44:05 PDT
Created attachment 544521 [details] [diff] [review]
Corrected the build issue, changed the UUID
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2011-07-07 10:03:54 PDT
Comment on attachment 544521 [details] [diff] [review]
Corrected the build issue, changed the UUID

r=me.  Thank you for the patch!
Comment 10 Vishnu S (:vishnus) 2011-07-07 10:08:40 PDT
so is that it? :) :)
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2011-07-07 10:16:21 PDT
That's it in terms of things you have to do.  The patch still needs to be landed, but I or one of the other folks managing mozilla-inbound will take care of that.
Comment 12 Vishnu S (:vishnus) 2011-07-07 10:18:13 PDT
oh ok :) thank you. :) and thank you Ms2ger :)
Comment 13 Josh Matthews [:jdm] 2011-07-07 10:42:21 PDT
I presume this will break a test if checked in as-is, according to http://mxr.mozilla.org/mozilla-central/source/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_595934_message_categories.js
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2011-07-07 10:47:51 PDT
Ah, indeed.  Good catch.  I'll remove that test before pushing this.  I checked the other consumers, and they're all working with windows, not documents.
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2011-07-07 12:45:33 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/764fbee6b335
Comment 16 Marco Bonardo [::mak] 2011-07-08 05:52:36 PDT
http://hg.mozilla.org/mozilla-central/rev/764fbee6b335
Comment 17 j.j. 2011-10-06 13:19:44 PDT
(In reply to Aryeh Gregor from comment #2)
> I don't see any reason for the warning.

Why not? Could have saved at least one bug report and probably a lot of unreported Developer's trouble.
Comment 18 Aryeh Gregor (:ayg) (away until October 25) 2011-10-06 13:23:26 PDT
IIRC, the warning previously said the feature was deprecated or similar, which isn't true.  So I don't see any reason for that warning.  It might be useful to have a warning for a couple of years saying that different browsers treat the attribute differently and you should use window.getSelection() instead if you want your code to work cross-browser.  But it seems like you could add warnings like that to lots of features, and usually Gecko doesn't, does it?
Comment 19 Eric Shepherd [:sheppy] 2011-10-17 10:03:49 PDT
This doc was already updated, and the change is listed on Firefox 8 for developers.
Comment 20 Robert Longson 2011-10-25 11:00:31 PDT
*** Bug 697184 has been marked as a duplicate of this bug. ***
Comment 21 :Ms2ger (⌚ UTC+1/+2) 2012-04-29 13:35:45 PDT
https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIDOMNSHTMLDocument gets it wrong
Comment 22 Florian Scholz [:fscholz] (MDN) 2012-06-30 09:30:12 PDT
(In reply to :Ms2ger from comment #21)
> https://developer.mozilla.org/en/XPCOM_Interface_Reference/
> nsIDOMNSHTMLDocument gets it wrong

Fixed.

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