document.getSelection() should return the same as getSelection(), not stringification

RESOLVED FIXED in mozilla8

Status

()

Core
DOM: Core & HTML
--
minor
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: ayg, Assigned: vishnus)

Tracking

({dev-doc-complete})

Trunk
mozilla8
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug])

Attachments

(3 attachments)

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.
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?
Component: DOM: Traversal-Range → DOM: Core & HTML
QA Contact: traversal-range → general
Whiteboard: [good first bug]
I don't see any reason for the warning.
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.
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.
(Assignee)

Comment 5

6 years ago
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 :)
(Assignee)

Comment 6

6 years ago
Created attachment 544270 [details] [diff] [review]
Changed implementation :)

Implementation part changed. Again thanks to Ms2ger :)
Attachment #544270 - Flags: review+

Updated

6 years ago
Attachment #544270 - Flags: review+ → review?(bzbarsky)
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.
Attachment #544270 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 8

6 years ago
Created attachment 544521 [details] [diff] [review]
Corrected the build issue, changed the UUID
Attachment #544521 - Flags: review?(bzbarsky)
Comment on attachment 544521 [details] [diff] [review]
Corrected the build issue, changed the UUID

r=me.  Thank you for the patch!
Attachment #544521 - Flags: review?(bzbarsky) → review+
Keywords: checkin-needed
(Assignee)

Comment 10

6 years ago
so is that it? :) :)
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.
(Assignee)

Comment 12

6 years ago
oh ok :) thank you. :) and thank you Ms2ger :)
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
Keywords: checkin-needed
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.
http://hg.mozilla.org/integration/mozilla-inbound/rev/764fbee6b335
Assignee: nobody → onlinevishnu
Flags: in-testsuite?
Target Milestone: --- → mozilla8
http://hg.mozilla.org/mozilla-central/rev/764fbee6b335
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Keywords: dev-doc-needed

Updated

6 years ago
Depends on: 692412

Comment 17

6 years ago
(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.
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?

Updated

6 years ago
Depends on: 690997
This doc was already updated, and the change is listed on Firefox 8 for developers.
Keywords: dev-doc-needed → dev-doc-complete

Updated

6 years ago
Duplicate of this bug: 697184
https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIDOMNSHTMLDocument gets it wrong
Keywords: dev-doc-complete → dev-doc-needed
(In reply to :Ms2ger from comment #21)
> https://developer.mozilla.org/en/XPCOM_Interface_Reference/
> nsIDOMNSHTMLDocument gets it wrong

Fixed.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.