Deep tree traversal in HTMLOptionElement.prototype.text ends up displaying <script> tag contents

VERIFIED FIXED in Firefox 18

Status

()

defect
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: collares, Assigned: Ms2ger)

Tracking

({regression})

18 Branch
mozilla19
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox17 unaffected, firefox18+ fixed, firefox19+ fixed)

Details

()

Attachments

(2 attachments, 2 obsolete attachments)

Posted file Testcase
Bug 787412 changed HTMLOptionElement.prototype.text so that it now displays every text node inside the <option> tag, but that also includes JavaScript code inside <script> tags; see attached HTML testcase for a demonstration. This differs from Chrome and Opera's behavior, although I don't know what the spec says.

I found this bug while visiting the main page of a major Brazilian airline (see URL above), so I would bet IE also behaves as Chrome and Opera (but I don't have a Windows machine near me to be sure).
Reporter

Updated

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

Comment 1

7 years ago
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?
Assignee

Comment 3

7 years ago
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.
Assignee

Updated

7 years ago
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
Assignee

Comment 5

7 years ago
Posted patch Patch v1 (obsolete) — Splinter Review
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+
Assignee

Comment 7

7 years ago
Posted patch Patch v2 (obsolete) — Splinter Review
Attachment #671808 - Attachment is obsolete: true
Attachment #672488 - Flags: review?(bzbarsky)
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-
Assignee

Comment 9

7 years ago
Posted patch Patch v3Splinter Review
Like this, then
Attachment #672488 - Attachment is obsolete: true
Attachment #672518 - Flags: review?(bzbarsky)
Comment on attachment 672518 [details] [diff] [review]
Patch v3

Yes, just like that.
Attachment #672518 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/16bf5abecb5b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
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?
Reporter

Comment 13

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