Closed
Bug 801486
Opened 12 years ago
Closed 12 years ago
Deep tree traversal in HTMLOptionElement.prototype.text ends up displaying <script> tag contents
Categories
(Core :: DOM: Core & HTML, defect)
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)
86 bytes,
text/html
|
Details | |
3.81 KB,
patch
|
bzbarsky
:
review+
bajaj
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•12 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•12 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.
tracking-firefox18:
--- → ?
tracking-firefox19:
--- → ?
![]() |
||
Comment 2•12 years ago
|
||
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•12 years ago
|
||
Looks like WebKit just excludes scripts. Do you think it would make sense to copy that?
Assignee: nobody → Ms2ger
![]() |
||
Comment 4•12 years ago
|
||
Given what I remember of the per-spec content model for <option>, yes.
Assignee | ||
Updated•12 years ago
|
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
Assignee | ||
Comment 5•12 years ago
|
||
Ignoring both svg:script and html:script to match WebKit/Opera.
Attachment #671808 -
Flags: review?(bzbarsky)
![]() |
||
Comment 6•12 years ago
|
||
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•12 years ago
|
||
Attachment #671808 -
Attachment is obsolete: true
Attachment #672488 -
Flags: review?(bzbarsky)
![]() |
||
Comment 8•12 years ago
|
||
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-
Updated•12 years ago
|
Assignee | ||
Comment 9•12 years ago
|
||
Like this, then
Attachment #672488 -
Attachment is obsolete: true
Attachment #672518 -
Flags: review?(bzbarsky)
![]() |
||
Comment 10•12 years ago
|
||
Comment on attachment 672518 [details] [diff] [review] Patch v3 Yes, just like that.
Attachment #672518 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 11•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/16bf5abecb5b
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
status-firefox19:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Assignee | ||
Updated•12 years ago
|
status-firefox17:
--- → unaffected
status-firefox18:
--- → affected
Assignee | ||
Comment 12•12 years ago
|
||
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•12 years ago
|
||
This is indeed fixed on today's nightly. Thanks a lot!
Status: RESOLVED → VERIFIED
Comment 14•12 years ago
|
||
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.
Description
•