Closed Bug 801486 Opened 9 years ago Closed 9 years ago
Deep tree traversal in HTMLOption
Element .prototype .text ends up displaying <script> tag contents
Summary: Deep tree traversal in HTMLOptionElement.prototype.text ends up displaying <script> tags → Deep tree traversal in HTMLOptionElement.prototype.text ends up displaying <script> tag contents
This may not be a DOM issue, as it looks like Firefox is doing the right thing with regards to the textContent value (http://dom.spec.whatwg.org/#dom-node-textcontent and a script tag is an Element, as far as I can tell). However, there definitely seems to be a bug here, even if we ignore how other browsers behave: The selected option text that gets displayed in the <option> widget is different from the text that appears in the drop-down list of options. This is visible in the testcase above.
Our behavior for .text is correct per spec, but the spec might well be wrong here. As far as comment 1 goes, the spec says that the display value should be the label. This is basically incompatible with Gecko's implementation of <option>, where we just render its contents. But again, the spec may well just be wrong here. And frankly, I don't care about the edge cases in this direction as much as I do about script content being included in the .text. Ms2ger, do you have time to look into this?
Looks like WebKit just excludes scripts. Do you think it would make sense to copy that?
Assignee: nobody → Ms2ger
Given what I remember of the per-spec content model for <option>, yes.
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
Ignoring both svg:script and html:script to match WebKit/Opera.
Attachment #671808 - Flags: review?(bzbarsky)
Comment on attachment 671808 [details] [diff] [review] Patch v1 How about doing this non-recursively using GetNextNode()/GetNextNonChildNode()? Also, instead of IsNodeOfType(), just test NodeType(), please. It's faster (e.g. non-virtual). r=me with those changes, but would like to see the non-recursive version before you land it.
Attachment #671808 - Flags: review?(bzbarsky) → review+
Comment on attachment 672488 [details] [diff] [review] Patch v2 This will fail if the DOM looks like so: <option> <span> Some <script></script> </span> Text </option> You really do want GetNextNonChildNode.
Attachment #672488 - Flags: review?(bzbarsky) → review-
Like this, then
Comment on attachment 672518 [details] [diff] [review] Patch v3 Yes, just like that.
Attachment #672518 - Flags: review?(bzbarsky) → review+
Comment on attachment 672518 [details] [diff] [review] Patch v3 [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 787412 User impact if declined: Some sites could display script contents. Testing completed (on m-c, etc.): on m-c, passes tests, reviewed by bz. Risk to taking this patch (and alternatives if risky): pretty low; the change is well-understood and localized to option elements. It aligns us with WebKit and Opera. Alternative is a backout. String or UUID changes made by this patch: None.
Attachment #672518 - Flags: approval-mozilla-aurora?
This is indeed fixed on today's nightly. Thanks a lot!
Status: RESOLVED → VERIFIED
Comment on attachment 672518 [details] [diff] [review] Patch v3 Approving as this is a tracked bug with a low risk fix . Also verified on nightly as per comment 13
Attachment #672518 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.