Closed Bug 801486 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: DOM: Core & HTML, defect)

18 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla19
Tracking Status
firefox17 --- unaffected
firefox18 + fixed
firefox19 + fixed

People

(Reporter: collares, Assigned: Ms2ger)

References

()

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

Attached 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).
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
Attached 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+
Attached 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-
Attached 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: 8 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?
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.