Closed Bug 1374247 Opened 7 years ago Closed 7 years ago

stylo: We don't implement the hack to allow child combinators to match across XBL <children> elements (could we remove it?).

Categories

(Core :: CSS Parsing and Computation, enhancement, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(4 files, 2 obsolete files)

Bug 1374247: Don't match in the add-ons detail page against .box-inherit, but against the scrollbox inner box.
59 bytes, text/x-review-board-request
johannh
: review+
dao
: review+
Details
Bug 1374247: Avoid using <children> in XUL buttons.
59 bytes, text/x-review-board-request
dao
: review+
Details
Bug 1374247: Remove the XBL children matching hack, and assert against it.
59 bytes, text/x-review-board-request
francois
: review+
Details
Keep the hack working on late beta and release, so we can land this a little earlier.
2.36 KB, patch
xidorn
: review+
bzbarsky
: review+
Details | Diff | Splinter Review 
http://searchfox.org/mozilla-central/rev/7cc377ce3f0a569c5c9b362d589705cd4fecc0ac/layout/style/nsCSSRuleProcessor.cpp#2450

We don't have anything like that right now, which is making my patch at bug 1371130 fail layout/reftests/bugs/232990-1b.xhtml.

Before that patch we were failing to restyle one of the two elements, which happily enough for that test case made it look correct.

I wonder if we can just kill that hack. It'd complicate multiple aspects of the code.
Boris, wdyt, could we remove that hack?
Flags: needinfo?(bzbarsky)
Actually that's not the reason for the failure, but I still think we should remove that hack anyway.
The real questions are:

1)  Do we have styles in the Firefox UI that depend on this hack?
2)  Do we have styles in extensions that depend on this hack?

At least when the hack was introduced, the answer to #1 was a resounding "yes".  If I had to guess in the absence of data, I would guess it's still "yes".  But we could try to answer this by the simple expedient of doing a MOZ_CRASH any time the hack causes a match when the non-hacked version would not match and seeing whether try passes (or indeed whether the browser starts up).
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #3)
> The real questions are:
> 
> 1)  Do we have styles in the Firefox UI that depend on this hack?
> 2)  Do we have styles in extensions that depend on this hack?
> 
> At least when the hack was introduced, the answer to #1 was a resounding
> "yes".  If I had to guess in the absence of data, I would guess it's still
> "yes".  But we could try to answer this by the simple expedient of doing a
> MOZ_CRASH any time the hack causes a match when the non-hacked version would
> not match and seeing whether try passes (or indeed whether the browser
> starts up).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcd7b85c003f018f0510b8afa20e5d9bf184b7dc

Seems it should really be fine, the only non-intermittent test that fails is the one added in that same patch that tests it (layout/reftests/dom/xbl-children-1.xhtml).
Unfortunately, I would expect the failures to be "This part of the browser UI looks wrong", not something we test in CI.  :(  So a clean try run is very much not sufficient to have confidence here...
That is, a clean try run with the hack just disabled.  A clean try run with the hack MOZ_CRASHing when it would trigger (as comment 3 suggests) would be much more confidence-inducing.
Priority: -- → P2
Priority: P2 → --
Priority: -- → P4
Lacking of this hack is part of the reason why xbl-children-1.xhtml [1] fails.

Another reason is that the reftest has a selector `.a > xbl|children > .b { text-decoration: underline; }`. When matching <div class="b">, this selector will be rejected by bloom filter because we do not seem to add <children> to the filter in [2].

[1] http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/layout/reftests/dom/reftest.list#52
[2] http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/servo/components/style/traversal.rs#657
I'd really prefer if we didn't implement this... It just feels wrong, and can be fixed changing the stylesheets instead.
How easy would it be to audit m-c for selectors susceptible to this problem? Now that legacy addons are gone, the only XBL consumers are in-tree. But we'll need to support all the frontend code, which is a lot more XBL than we have in content.
xul:button [1] has tree structure like:

<xul:hbox class="box-inherit button-box">
  <children>
     <xul:image class="button-icon">
     <xul:label class="button-text">

And we have many selectors like `.button-box > .button-icon` [2] or `.button-box > .button-text` [3], which needs this hack to skip <children> in the tree structure in order to match.

I don't know how easy to audit everything systematically like this for now.

[1] http://searchfox.org/mozilla-central/rev/5696c3e525fc8222674eed6a562f5fcbe804c4c7/toolkit/content/widgets/button.xml#226,229
[2] http://searchfox.org/mozilla-central/search?q=.button-box+%3E+.button-icon&case=false&regexp=true&path=
[3] http://searchfox.org/mozilla-central/search?q=.button-box+%3E+.button-text&case=false&regexp=true&path=
Yeah, I'll disable that hack and see what's broken, that's probably easier.

We also need to insert the XBL children element on the bloom filter though, which is pretty annoying, because those are really supposed to match.

We can move the bloom filter to work on the DOM tree I guess, but that means a bit more painful rebuilds and such, which is annoying. :/
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #12)
> Try result which does what bz recommands in comment 3.
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=d9f405c9c8c02473904177321ece5afc882b2b88

Ugh, that's very orange.

So, we need to do something here, but it's no fun. So there are three independent problems, afaict:

 (1) <xbl:children> match on the DOM, but there's a hack to skip it. This is what this bug is about. But on top of that:
 (2) <xbl:children> aren't part of the flattened tree. This means they don't end up in the bloom filter, which is pretty wrong.

Our whole bloom filter stuff is based on the flattened tree depth, so it's not easy to work around it.

This is for sure the same problem blink has with <slot> and Shadow DOM. They shipped an implementation of <slot> as a non-participant on the flattened tree, and that's pretty broken. That's being fixed though.

So basically, I think for (2) it'd be nice if <xbl:children> was part of the flattened tree, and had display: contents by default. But I suspect that's going to be non-trivial. I don't see a clearer path for it though.

As for (1), I'm not fond at all of this hack. We can definitely add it to selectors, but it's just nasty, and I'd prefer for it to not exist. I think it wouldn't be hard to debug which selector is matching using the patch in the comment above, and fixing them one by one...
Ok. Given that this likely only affects chrome and is non-trivial to solve, we should probably punt on this for 57.
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc7607d01542
Annotate xbl-children-1.xhtml with the bug number. r=me
(In reply to Emilio Cobos Álvarez [:emilio] from comment #13)
> So, we need to do something here, but it's no fun. So there are three
> independent problems, afaict:
> 
>  (1) <xbl:children> match on the DOM, but there's a hack to skip it. This is
> what this bug is about. But on top of that:
>  (2) <xbl:children> aren't part of the flattened tree. This means they don't
> end up in the bloom filter, which is pretty wrong.
> 
> Our whole bloom filter stuff is based on the flattened tree depth, so it's
> not easy to work around it.

I'm not sure I understand the problem here.

For (2), bloom filter doesn't include bits from <xbl:children>, I suppose that means any selector intends to match against <xbl:children> would always be rejected by bloom filter, is that correct?

And for (1), there is a hack to skip matching <xbl:children> on DOM...

Can we, instead of removing this hack, force this hack to apply? That says, can we consistently skip <xbl:children> in all cases?

A quick search indicates that there is only one rule explicitly wants to match <xbl:children>, which is for setting its display to none in xul.css. I don't see any other CSS file includes children element, and I don't see any <xbl:children> element in any .xml file which has any id or class.

Given the second point, IIUC, consistently skipping <xbl:children> would be easier?
(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive Nov 5 ~ Dec 16) from comment #17)
> 
> For (2), bloom filter doesn't include bits from <xbl:children>, I suppose
> that means any selector intends to match against <xbl:children> would always
> be rejected by bloom filter, is that correct?

We'll reject only <children> when there are to the left of a descendant combinator.

> Given the second point, IIUC, consistently skipping <xbl:children> would be
> easier?

Kind of... If I understand correctly, the only time we should be selector-matching an <xbl:children> descendant is when the fallback distribution is showing, that is, no node is actually assigned to that insertion point.

I think in this case <xbl:children> should appear on the flat tree instead (this is the equivalent problem to bug 1410170).

In any case, skipping them in TNode::parent_element is a bad idea (because that is supposed to be the light tree, and other callers would be wrong in this case). Instead, we should add a different method I guess... But that's not great, and does feel like a hack.

I can try to make <xbl:children> with fallback content appear on the flat tree I think. It should be doable...
FWIW, this is the try push similar to comment 12 (because I overlooked that comment): https://treeherder.mozilla.org/#/jobs?repo=try&revision=c07aa4547ac76fd0a8ca0ea9d9d3fb919ae4a17a This may also assert for more cases because I replaced all "return false" in that function.

This is a try push for what I was thinking about in comment 17 that we apply this hack unconditionally, and assert when the hack doesn't apply but normal path applies (that is, we matches the <xbl:children> element itself somehow): https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f660c510674bed8160ff6c1f37b20d805a4c7c9 This one only asserts in the xbl-children-1.xhtml test, but doesn't yield anything elsewhere.

So if making the hack unconditionally applied (that we always ignore <xbl:children> after ">") is easier, I guess we can go that way.

I still suspect that we never really need to match any xbl:children...
(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive Nov 5 ~ Dec 16) from comment #19)
> FWIW, this is the try push similar to comment 12 (because I overlooked that
> comment):
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=c07aa4547ac76fd0a8ca0ea9d9d3fb919ae4a17a This may
> also assert for more cases because I replaced all "return false" in that
> function.
> 
> This is a try push for what I was thinking about in comment 17 that we apply
> this hack unconditionally, and assert when the hack doesn't apply but normal
> path applies (that is, we matches the <xbl:children> element itself
> somehow):
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=4f660c510674bed8160ff6c1f37b20d805a4c7c9 This one
> only asserts in the xbl-children-1.xhtml test, but doesn't yield anything
> elsewhere.
> 
> So if making the hack unconditionally applied (that we always ignore
> <xbl:children> after ">") is easier, I guess we can go that way.

Yeah, but it makes the code uglier, and makes us add one-off methods to rust-selectors... :(

I can look into the failures, I suspect there aren't that many rules that rely on this hack, but of course a single rule that does and happens to be in a common sheet, and we get such amount of orange...
Flags: needinfo?(emilio)
Depends on: 1412011
For reference, here's a try run with some of the selectors fixed and an assertion tweaked so that it shows the selector with a mismatch:

 * https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a2eba8eee1afc88d07a9060a366edcfdaf4b867&selectedJob=139888579

There are a few, but from the ones I've tried to fix, they're mostly overly-specific selectors that use child combinators presumably for "performance", I guess?

I can try to change that NS_RUNTIMEABORT with NS_ASSERTION to get the complete list of them, I suppose, and re-evaluate.

Implementing the hack may be easier (selector-matching wise), but we need to also tweak invalidation and such, which may not be as easy...

While considering whether to implement it I found bug 1412011, and I fear similar special-cases will have similar obscure bugs...
Flags: needinfo?(emilio)
Didn't intend to drop ni?
Flags: needinfo?(emilio)
If our plan is to remove this hack, it would probably be helpful if we can remove it from the old style system before the soft code freeze for 58... which is supposed to start in one week.
(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive Nov 5 ~ Dec 16) from comment #23)
> If our plan is to remove this hack, it would probably be helpful if we can
> remove it from the old style system before the soft code freeze for 58...
> which is supposed to start in one week.

So, I took my previous patch, rebased it, and spent the whole day trying to look for suspicious selectors and to make them trigger.

At this point, I haven't been able to trigger any of them for a fair amount of time, and I think that's because most of the stylesheets we have are overly generic, but none of our about: pages and such use those <xul> elements (and there are no XUL-based add-ons anymore).

So, at this point, I'm not sure what's the best path forward, I'm tempted to do the following:

 * Land my patch with the stylesheet fixes and disabling the hack.
 * Make the assertion a diagnostic assert, with a message asking to file a bug against this one.
 * Fix those bugs, if any. I can take care of that.

Does that seem reasonable? Is there any precedent of something like that?
Flags: needinfo?(xidorn+moz)
Assignee: nobody → emilio
This sounds like a reasonable path forward. I think we can go with this plan.

It doesn't immediately occur to me any precedent of this kind of things. bz may know more about how we handle something like this in the past.
Flags: needinfo?(xidorn+moz) → needinfo?(bzbarsky)
Flags: needinfo?(emilio)
Comment on attachment 8924088 [details]
Bug 1374247: Remove the XBL children matching hack, and assert against it.

https://reviewboard.mozilla.org/r/195302/#review200396

r=me with the issue below addressed.

Also the new usage of `MOZ_CRASH_UNSAFE_PRINTF` needs review from a data steward in addition.

::: layout/style/nsCSSRuleProcessor.cpp:2402
(Diff revision 1)
> -          if (SelectorMatchesTree(element, selector, aTreeMatchContext,
> -                                  aFlags)) {
> +          xblChildrenMatched =
> +            SelectorMatchesTree(element, selector, aTreeMatchContext, aFlags);

I think it is better to do
```c++
if (SelectorMatchesTree(...)) {
  xblChildrenMatched = true;
}
```
which should be more complete?
Attachment #8924088 - Flags: review?(xidorn+moz) → review+
Attachment #8924088 - Flags: review?(francois)
francois, this MOZ_CRASH_UNSAFE_PRINTF usage includes a violating selector. That selector can only come from XBL stylesheet, since it is something specific to <xbl:children>, so I think it should belongs to Category 1 “Technical data”.
Comment on attachment 8924088 [details]
Bug 1374247: Remove the XBL children matching hack, and assert against it.

https://reviewboard.mozilla.org/r/195302/#review200396

> I think it is better to do
> ```c++
> if (SelectorMatchesTree(...)) {
>   xblChildrenMatched = true;
> }
> ```
> which should be more complete?

I used `xblChildrenMatched |= SelectorMatchesTree(...)`, which is equivalent. I don't think it matters though, because this loop effectively only has a single iteration when the combinator is `>`.
Attachment #8924088 - Flags: review?(francois)
For data steward review of the last patch, in case the review request is cleared by mistake again. See comment 32.
Flags: needinfo?(francois)
Comment on attachment 8924085 [details]
Bug 1374247: Remove useless customizableMode.inc.css rule.

https://reviewboard.mozilla.org/r/195296/#review200442

So

::: browser/themes/shared/customizableui/customizeMode.inc.css
(Diff revision 2)
> -.customizationmode-button > .box-inherit {
> -  border-width: 0;
> -  padding: 3px 0;
> -  padding-inline-start: 0;
> -  padding-inline-end: 0;
> -}

So this doesn't regress bug 1002931?
Comment on attachment 8924086 [details]
Bug 1374247: Avoid using <children> in XUL buttons.

https://reviewboard.mozilla.org/r/195298/#review200446

I'm slightly concerned about the perf impact of this as descendent selectors are expected to be slower than child selectors...

::: browser/base/content/browser.css:670
(Diff revision 2)
>    -moz-binding: url("chrome://browser/content/search/search.xml#browser-search-autocomplete-result-popup");
>  }
>  
>  /* Overlay a badge on top of the icon of additional open search providers
>     in the search panel. */
> -.addengine-item > .button-box > .button-icon,
> +.addengine-item .button-box .button-icon {

Can you get rid of .button-box?

::: browser/themes/linux/controlcenter/panel.css:8
(Diff revision 2)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  %include ../../shared/controlcenter/panel.inc.css
>  
> -.identity-popup-expander > .button-box,
> -.identity-popup-permission-remove-button > .button-box {
> +.identity-popup-expander .button-box,
> +.identity-popup-permission-remove-button .button-box {

Use :-moz-any?

::: browser/themes/linux/searchbar.css:140
(Diff revision 2)
>  
>  .searchbar-engine-one-off-item > .button-box {
>    padding: 0;
>  }
>  
> -.searchbar-engine-one-off-item > .button-box > .button-text {
> +.searchbar-engine-one-off-item .button-box .button-text {

Can you get rid of .button-box?

::: browser/themes/linux/searchbar.css:144
(Diff revision 2)
>  
> -.searchbar-engine-one-off-item > .button-box > .button-text {
> +.searchbar-engine-one-off-item .button-box .button-text {
>    display: none;
>  }
>  
> -.searchbar-engine-one-off-item > .button-box > .button-icon {
> +.searchbar-engine-one-off-item .button-box .button-icon {

ditto

::: browser/themes/linux/searchbar.css:194
(Diff revision 2)
>    height: 16px;
>    margin: -7px -9px 7px 9px;
>    list-style-image: url("chrome://browser/skin/badge-add-engine.png");
>  }
>  
> -.addengine-item > .button-box > .button-text,
> +.addengine-item .button-box .button-text {

ditto

::: browser/themes/linux/searchbar.css:258
(Diff revision 2)
>  }
>  
> -.search-setting-button-compact > .button-box > .button-icon {
> +.search-setting-button-compact .button-icon {
>    list-style-image: url("chrome://browser/skin/settings.svg");
>    -moz-context-properties: fill;
>    fill: currentColor;

Can you just set this stuff on .search-setting-button-compact and get rid of .button-icon?

::: browser/themes/osx/searchbar.css:132
(Diff revision 2)
>    background-color: Highlight;
>    background-image: none;
>    color: HighlightText;
>  }
>  
> -.searchbar-engine-one-off-item > .button-box > .button-text {
> +.searchbar-engine-one-off-item .button-box .button-text {

Can you get rid of .button-box?

::: browser/themes/osx/searchbar.css:136
(Diff revision 2)
>  
> -.searchbar-engine-one-off-item > .button-box > .button-text {
> +.searchbar-engine-one-off-item .button-box .button-text {
>    display: none;
>  }
>  
> -.searchbar-engine-one-off-item > .button-box > .button-icon {
> +.searchbar-engine-one-off-item .button-box .button-icon {

ditto

::: browser/themes/osx/searchbar.css:183
(Diff revision 2)
>    height: 16px;
>    margin: -7px -9px 7px 9px;
>    list-style-image: url("chrome://browser/skin/badge-add-engine.png");
>  }
>  
> -.addengine-item > .button-box > .button-text,
> +.addengine-item .button-box .button-text {

ditto

::: browser/themes/shared/incontentprefs/preferences.inc.css:67
(Diff revision 2)
>    margin-bottom: 0 !important;
>  }
>  
> -menulist > hbox > label,
> -menuitem > label,
> -button > hbox > label {
> +menulist label,
> +menuitem label,
> +button label {

Use :-moz-any?

::: browser/themes/windows/searchbar.css:134
(Diff revision 2)
>    background-color: Highlight;
>    background-image: none;
>    color: HighlightText;
>  }
>  
>  .searchbar-engine-one-off-item > .button-box {

Does this need an update?

::: toolkit/themes/shared/popupnotification.inc.css:111
(Diff revision 2)
>  .popup-notification-dropmarker {
>    flex: none;
>    padding: 0 15px;
>  }
>  
> -.popup-notification-dropmarker > .button-box > hbox {
> +.popup-notification-dropmarker .button-box hbox {

Please use .box-inherit

::: toolkit/themes/shared/popupnotification.inc.css:123
(Diff revision 2)
>    display: -moz-box;
>    padding: 0;
>    margin: 0;
>  }
>  
> -.popup-notification-dropmarker > .button-box > .button-menu-dropmarker > .dropmarker-icon {
> +.popup-notification-dropmarker .button-menu-dropmarker .dropmarker-icon {

Can you get rid of .button-menu-dropmarker?
Attachment #8924086 - Flags: review?(dao+bmo) → review-
(In reply to Dão Gottwald [::dao] from comment #41)
> > -.addengine-item > .button-box > .button-icon,
> > +.addengine-item .button-box .button-icon {
> 
> Can you get rid of .button-box?

Oh, hm, I hadn't seen the following patch yet.
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) (Away Oct. 24 – Nov. 12) from comment #10)
> xul:button [1] has tree structure like:
> 
> <xul:hbox class="box-inherit button-box">
>   <children>
>      <xul:image class="button-icon">
>      <xul:label class="button-text">
> 
> And we have many selectors like `.button-box > .button-icon` [2] or
> `.button-box > .button-text` [3], which needs this hack to skip <children>
> in the tree structure in order to match.

I'm not sure why the binding is structured like this in the first place. Do we even have XUL buttons with non-anonymous children? Perhaps we should get rid of that <children> element or move it to be a sibling of button-icon.
(In reply to Dão Gottwald [::dao] from comment #40)
> Comment on attachment 8924085 [details]
> Bug 1374247: Remove useless customizableMode.inc.css rule.
> 
> https://reviewboard.mozilla.org/r/195296/#review200442
> 
> So
> 
> ::: browser/themes/shared/customizableui/customizeMode.inc.css
> (Diff revision 2)
> > -.customizationmode-button > .box-inherit {
> > -  border-width: 0;
> > -  padding: 3px 0;
> > -  padding-inline-start: 0;
> > -  padding-inline-end: 0;
> > -}
> 
> So this doesn't regress bug 1002931?

Err, indeed it (probably) does. I had not been able to see this rule in the toolbox, but that makes sense because I'm on Linux and there's a giant %ifdef for OSX / Windows only.

I'll remove this and put a version of the rule that works without the hack on the fourth patch.
(In reply to Dão Gottwald [::dao] from comment #41)
> Comment on attachment 8924086 [details]
> Bug 1374247: Simplify some overly-specific selectors that rely on this hack.
> 
> https://reviewboard.mozilla.org/r/195298/#review200446
> 
> I'm slightly concerned about the perf impact of this as descendent selectors
> are expected to be slower than child selectors...

I mentioned this in the commit message. I expect this not to have an impact on performance the way selector matching works.

We keep a bloom filter with all the classes and such in ancestors, so we fast-reject those. Also, we only match rules with a class-name in the rightmost compound selector if an element has that class-name. So in practice we'll only attempt to match them when they are pretty likely to match.
Comment on attachment 8924086 [details]
Bug 1374247: Avoid using <children> in XUL buttons.

https://reviewboard.mozilla.org/r/195298/#review200446

> Can you get rid of .button-box?

Yup, done.

> Use :-moz-any?

I'd prefer not to, because actually we don't optimize those out (we don't insert them in the ancestor filter), and I think it's not a terrible selector otherwise.

I filed bug 1413533 to consider doing it though. If the frontend has a lot of ancestor selectors using `:-moz-any` it could be worth I think.

> Can you get rid of .button-box?

It goes away in the later patch.

> ditto

Ditto :)

> Can you just set this stuff on .search-setting-button-compact and get rid of .button-icon?

They're all inherited, so yeah, I can do that, nice one :)

> Can you get rid of .button-box?

Done

> ditto

Done

> Use :-moz-any?

Same concern as above. Not sure if it actually matters... Can do it if you want.

> Does this need an update?

Goes away later.

> Please use .box-inherit

Done
Attachment #8924085 - Attachment is obsolete: true
Attachment #8924085 - Flags: review?(jhofmann)
Attachment #8924085 - Flags: review?(dao+bmo)
(In reply to Dão Gottwald [::dao] from comment #43)
> (In reply to Ting-Yu Lin [:TYLin] (UTC+8) (Away Oct. 24 – Nov. 12) from
> comment #10)
> > xul:button [1] has tree structure like:
> > 
> > <xul:hbox class="box-inherit button-box">
> >   <children>
> >      <xul:image class="button-icon">
> >      <xul:label class="button-text">
> > 
> > And we have many selectors like `.button-box > .button-icon` [2] or
> > `.button-box > .button-text` [3], which needs this hack to skip <children>
> > in the tree structure in order to match.
> 
> I'm not sure why the binding is structured like this in the first place. Do
> we even have XUL buttons with non-anonymous children? Perhaps we should get
> rid of that <children> element or move it to be a sibling of button-icon.

Any thoughts on this?
Flags: needinfo?(emilio)
That makes sense to me, I can try, I guess... Though it's even more risky than this I would say?
Flags: needinfo?(emilio)
Hm, actually, the non-anonymous children of the button binding is the button text isn't it?

The "button-icon" and such are the fallback content, which I guess is used for when there's no anon children, so it can be customized... So not sure refactoring it to avoid <children> is possible at all.
Comment on attachment 8924088 [details]
Bug 1374247: Remove the XBL children matching hack, and assert against it.

Err, this still needs data-r.
Attachment #8924088 - Flags: review+ → review?(francois)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #53)
> Hm, actually, the non-anonymous children of the button binding is the button
> text isn't it?

I don't think is. We usually use the label attribute, which is covered by <xul:label class="button-text" xbl:inherits="value=label.../>, and then there's also <xul:label class="button-highlightable-text" xbl:inherits="xbl:text=label.../>. The second label in particular being fallback content for when there's no text node child doesn't seem to make sense.
(In reply to Dão Gottwald [::dao] from comment #55)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #53)
> > Hm, actually, the non-anonymous children of the button binding is the button
> > text isn't it?
> 
> I don't think is. We usually use the label attribute, which is covered by
> <xul:label class="button-text" xbl:inherits="value=label.../>, and then
> there's also <xul:label class="button-highlightable-text"
> xbl:inherits="xbl:text=label.../>. The second label in particular being
> fallback content for when there's no text node child doesn't seem to make
> sense.

Hm, you're right, the <button>s I was looking at were HTML buttons, not XUL.

Yeah, that's true, but still we have stuff like:

 * http://searchfox.org/mozilla-central/rev/aa1343961fca52e8c7dab16788530da31aab7c8d/browser/base/content/webrtcIndicator.xul#29

I think that's the only one I could find, actually. I guess I can give this a spin, though not sure if this should block this bug...
(In reply to Emilio Cobos Álvarez [:emilio] from comment #56)
> Yeah, that's true, but still we have stuff like:
> 
>  *
> http://searchfox.org/mozilla-central/rev/
> aa1343961fca52e8c7dab16788530da31aab7c8d/browser/base/content/
> webrtcIndicator.xul#29
> 
> I think that's the only one I could find, actually.

Actually that should end up here: http://searchfox.org/mozilla-central/rev/aa1343961fca52e8c7dab16788530da31aab7c8d/toolkit/content/widgets/button.xml#240
datareview+ based on the explanation in comment 32.
Flags: needinfo?(francois)
Attachment #8924088 - Flags: review?(francois) → review+
Attachment #8924087 - Attachment is obsolete: true
Attachment #8924087 - Flags: review?(jhofmann)
Attachment #8924087 - Flags: review?(dao+bmo)
> Can we, instead of removing this hack, force this hack to apply? 

We can.

The idea at the time the hack was introduced was to try to unify styling for XBL and Shadow DOM as much as possible, and in Shadow DOM there is no such hack.  So we _can_ force the hack to always apply, but it might complicate transition away from XBL toward shadow DOM if we do that.

> Does that seem reasonable? Is there any precedent of something like that?

This seems pretty plausible, yes.  Especially if landed near the beginning of a nightly cycle.

Also it would be really nice if the assert/crash message somehow includes the selector involved or some other information that makes it possible to track things down...

Sending some sort of "Intent" mail to dev-platform would be a good idea too.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #61)
> Also it would be really nice if the assert/crash message somehow includes
> the selector involved or some other information that makes it possible to
> track things down...

Yup, that's basically what the patch does :).

I'll make sure to send an intent to remove to dev-platform so people are aware.
Comment on attachment 8924088 [details]
Bug 1374247: Remove the XBL children matching hack, and assert against it.

https://reviewboard.mozilla.org/r/195302/#review200744
Attachment #8924088 - Flags: review?(francois) → review+
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #61)
> > Does that seem reasonable? Is there any precedent of something like that?
> 
> This seems pretty plausible, yes.  Especially if landed near the beginning
> of a nightly cycle.

So, emilio has a pretty different idea with the patch, that we're going to remove this hack and use a diagnostic assertion to identify and fix any selector relying on the hack. This is what the current patch is doing.

Since this is a potentially more risky way, it would probably be better to land as soon as possible.

> Sending some sort of "Intent" mail to dev-platform would be a good idea too.

I agree that some sort of "intent" email would be great. I forgot about that :/
Comment on attachment 8924086 [details]
Bug 1374247: Avoid using <children> in XUL buttons.

https://reviewboard.mozilla.org/r/195298/#review200940
Attachment #8924086 - Flags: review?(dao+bmo) → review+
Comment on attachment 8924084 [details]
Bug 1374247: Don't match in the add-ons detail page against .box-inherit, but against the scrollbox inner box.

https://reviewboard.mozilla.org/r/195294/#review200944
Attachment #8924084 - Flags: review?(dao+bmo) → review+
> Since this is a potentially more risky way, it would probably be better to land as soon as possible.

The question is this: how sure are we that we will catch all the cases that need to be caught before it makes its way to release?  Especially if we're actually removing the functionality, not just preffing it off...
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #67)
> > Since this is a potentially more risky way, it would probably be better to land as soon as possible.
> 
> The question is this: how sure are we that we will catch all the cases that
> need to be caught before it makes its way to release?  Especially if we're
> actually removing the functionality, not just preffing it off...

I can also just keep the functionality on release, if you think that's better. I have no strong opinion, but I personally think that the chances that literally nobody in our nightly / beta population hit an assertion before the same code causes a bug on release is highly unlikely though.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #68)
> I can also just keep the functionality on release, if you think that's
> better. I have no strong opinion, but I personally think that the chances
> that literally nobody in our nightly / beta population hit an assertion
> before the same code causes a bug on release is highly unlikely though.

Per conversation on IRC with Boris, it's 11 days until 59, which isn't much. So let's wait until then to land this, that way we don't need to rush and uplift patches to beta, hopefully.
The problem of waiting it until 59 is that, I'm going to enable stylo on chrome since early 59, which means the diagnostic assertion would not be effective anymore, and thus we are not able to catch any usage of that.
I think we should land it in 58, so that we have a better confidence that it is not necessary to implement in stylo. Otherwise we really have no chance to check that theory anymore, unless we want to further delay stylo chrome for this single bug.
Flags: needinfo?(emilio)
Flags: needinfo?(bzbarsky)
I'm fine with whatever you think it's more appropriate, honestly. I agree that if the plan is enabling stylo in chrome in early 59 we should probably land it pretty much now...

This patch keeps the assert in nightly and "early" beta, and makes the hack work in "late" beta and release. Since the hack is additive, there's no risk of doing anything while the hack does not apply that happens to break when the hack does apply.

Let me know if you'd be fine with landing this with this extra patch.
Flags: needinfo?(emilio)
Attachment #8924778 - Flags: review?(xidorn+moz)
Attachment #8924778 - Flags: review?(bzbarsky)
Yeah, ok.  If we plan to flip that switch soon, we should land this ASAP.
Flags: needinfo?(bzbarsky)
Comment on attachment 8924778 [details] [diff] [review]
Keep the hack working on late beta and release, so we can land this a little earlier.

r=me
Attachment #8924778 - Flags: review?(bzbarsky) → review+
Attachment #8924778 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8924084 [details]
Bug 1374247: Don't match in the add-ons detail page against .box-inherit, but against the scrollbox inner box.

https://reviewboard.mozilla.org/r/195294/#review201340

r=me too, fwiw :)
Attachment #8924084 - Flags: review?(jhofmann) → review+
[Tracking Requested - why for this release]:
We plan to enable stylo on chrome in Firefox 59, and stylo doesn't implement this hack, so we need to test the removal of the hack in 58 to give us more confidence to ship stylo on chrome.
status-firefox58: fix-optional → affected
tracking-firefox58: --- → ?
Since bug 1412560 is P1, this certainly should be as well.
Priority: P4 → P1
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0064de758725
Don't match in the add-ons detail page against .box-inherit, but against the scrollbox inner box. r=johannh,dao
https://hg.mozilla.org/integration/autoland/rev/fe14831a814b
Avoid using <children> in XUL buttons. r=dao
https://hg.mozilla.org/integration/autoland/rev/5c4cb77d75e5
Remove the XBL children matching hack, and assert against it. r=xidorn,francois
https://hg.mozilla.org/integration/autoland/rev/9edca0017ec4
Keep the hack working on late beta and release, so we can land this a little earlier. r=bz,xidorn
Any reason we need to leave it open? Can we just mark it fixed?
Flags: needinfo?(emilio)
Yup, the leave-open tag was added when landing the test annotation. This should be marked fixed.
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(emilio)
Resolution: --- → FIXED
Depends on: 1414513
Depends on: 1414544
Depends on: 1414553
Depends on: 1414556
Depends on: 1414570
Depends on: 1414786
Depends on: 1415623
Depends on: 1424013
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: