Closed Bug 1320000 Opened 3 years ago Closed 3 years ago

XBLChildrenElement::ParseAttribute doesn't delegate to its superclass

Categories

(Core :: XBL, defect)

Product:
Core
Component:
XBL
Type:
defect
Priority:
Not set

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(2 files, 2 obsolete files)

Part 1: Force xbl:children elements to be display:none.
1.77 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
Part 2: Make XBLChildrenElement::ParseAttribute delegate to its superclass.
1.05 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review 
XBLChildrenElement::ParseAttribute doesn't call nsXMLElement::ParseAttribute at the end, which means when we set class="" on an <xbl:children> element, we don't end up at Element::ParseAttribute, which means (a) we don't set NODE_MAY_HAVE_CLASS on the element, and (b) we end up storing a single class name as a string in the nsAttrValue, rather than an nsIAtom, in Element::SetAttr.

This invalidates our assertion in ClassOrClassList in ServoBindings.cpp that if the class="" attribute has a string in the nsAttrValue, that it must be pure white space.
Attached patch patch (obsolete) — — Splinter Review
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8813962 - Flags: review?(mrbkap)
Attachment #8813962 - Attachment is patch: true
layout/reftests/dom/xbl-children-2.xhtml now fails, with a single "FAIL" showing, which I assume is the <xbl:children class="forced"> element.  Those elements are normally not displayed due to this rule:

http://searchfox.org/mozilla-central/rev/feef954874af9a18168e61a75629a9406b847c53/toolkit/content/xul.css#53

Since they're just being treated as regular elements here, I guess it's not a problem to allow this.

(Oddly, when I load that test in a regular browser tab, all four FAILs show.  My guess is that's because xul.css is loaded lazily, and we don't trigger loading it based on an XBL-namespaced element.  Nearly all the other rules in that file are for XUL elements.)
Comment on attachment 8813997 [details] [diff] [review]
patch v2

As much as it doesn't make sense to set |display: block| on an <xbl:children> element, I don't think we should allow that to take effect.

Can we make the xbl|children rule be |display: none !important;| instead?
Attachment #8813997 - Flags: review?(mrbkap) → review-
Attachment #8813997 - Attachment is obsolete: true
Attachment #8815545 - Flags: review?(mrbkap)
Attachment #8815545 - Flags: review?(mrbkap) → review+
Comment on attachment 8815546 [details] [diff] [review]
Part 2: Make XBLChildrenElement::ParseAttribute delegate to its superclass.

Review of attachment 8815546 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.
Attachment #8815546 - Flags: review?(mrbkap) → review+
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bee6662585ed
Part 1: Force xbl:children elements to be display:none. r=mrbkap
https://hg.mozilla.org/integration/mozilla-inbound/rev/523009ae06f6
Part 2: Make XBLChildrenElement::ParseAttribute delegate to its superclass. r=mrbkap
https://hg.mozilla.org/mozilla-central/rev/bee6662585ed
https://hg.mozilla.org/mozilla-central/rev/523009ae06f6
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.