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

Status: VERIFIED FIXED

Description

[Mauricio Collares [:collares]]

Attachment: 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).

Comment 1

[Mauricio Collares [:collares]]

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.

Comment 2

[Boris Zbarsky [:bzbarsky, bz on IRC]]

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?

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Looks like WebKit just excludes scripts. Do you think it would make sense to copy that?

Comment 4

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Given what I remember of the per-spec content model for <option>, yes.

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

Ignoring both svg:script and html:script to match WebKit/Opera.

Comment 6

[Boris Zbarsky [:bzbarsky, bz on IRC]]

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.

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v2

Comment 8

[Boris Zbarsky [:bzbarsky, bz on IRC]]

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.

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v3

Like this, then

Comment 10

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Comment on attachment 672518 [details] [diff] [review]
Patch v3

Yes, just like that.

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/16bf5abecb5b

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

Comment 13

[Mauricio Collares [:collares]]

This is indeed fixed on today's nightly. Thanks a lot!

Comment 14

[bhavana bajaj [:bajaj]]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c3d50ce5cec8