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)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla58
People
(Reporter: emilio, Assigned: emilio)
References
Details
Attachments
(4 files, 2 obsolete files)
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.
Assignee | ||
Comment 2•7 years ago
|
||
Actually that's not the reason for the failure, but I still think we should remove that hack anyway.
Comment 3•7 years ago
|
||
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)
Assignee | ||
Comment 4•7 years ago
|
||
(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).
Comment 5•7 years ago
|
||
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...
Comment 6•7 years ago
|
||
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.
Updated•7 years ago
|
Priority: -- → P2
Updated•7 years ago
|
Priority: P2 → --
Updated•7 years ago
|
Priority: -- → P4
Updated•7 years ago
|
Blocks: stylo-chrome
Comment 7•7 years ago
|
||
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
Assignee | ||
Comment 8•7 years ago
|
||
I'd really prefer if we didn't implement this... It just feels wrong, and can be fixed changing the stylesheets instead.
Comment 9•7 years ago
|
||
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.
Comment 10•7 years ago
|
||
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®exp=true&path= [3] http://searchfox.org/mozilla-central/search?q=.button-box+%3E+.button-text&case=false®exp=true&path=
Assignee | ||
Comment 11•7 years ago
|
||
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. :/
Comment 12•7 years ago
|
||
Try result which does what bz recommands in comment 3. https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9f405c9c8c02473904177321ece5afc882b2b88
Assignee | ||
Comment 13•7 years ago
|
||
(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...
Comment 14•7 years ago
|
||
Ok. Given that this likely only affects chrome and is non-trivial to solve, we should probably punt on this for 57.
Comment 15•7 years ago
|
||
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
Updated•7 years ago
|
Keywords: leave-open
Comment 16•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fc7607d01542
Updated•7 years ago
|
status-firefox57:
--- → wontfix
status-firefox58:
--- → fix-optional
Comment 17•7 years ago
|
||
(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?
Assignee | ||
Comment 18•7 years ago
|
||
(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...
Comment 19•7 years ago
|
||
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...
Assignee | ||
Comment 20•7 years ago
|
||
(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)
Assignee | ||
Comment 21•7 years ago
|
||
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)
Comment 23•7 years ago
|
||
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.
Assignee | ||
Comment 24•7 years ago
|
||
(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 | ||
Updated•7 years ago
|
Assignee: nobody → emilio
Comment 25•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(emilio)
Comment 31•7 years ago
|
||
mozreview-review |
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+
Updated•7 years ago
|
Attachment #8924088 -
Flags: review?(francois)
Comment 32•7 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 38•7 years ago
|
||
mozreview-review-reply |
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 `>`.
Updated•7 years ago
|
Attachment #8924088 -
Flags: review?(francois)
Comment 39•7 years ago
|
||
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 40•7 years ago
|
||
mozreview-review |
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 41•7 years ago
|
||
mozreview-review |
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-
Comment 42•7 years ago
|
||
(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.
Comment 43•7 years ago
|
||
(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.
Assignee | ||
Comment 44•7 years ago
|
||
(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.
Assignee | ||
Comment 45•7 years ago
|
||
(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.
Assignee | ||
Comment 46•7 years ago
|
||
mozreview-review-reply |
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
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8924085 -
Attachment is obsolete: true
Attachment #8924085 -
Flags: review?(jhofmann)
Attachment #8924085 -
Flags: review?(dao+bmo)
Comment 51•7 years ago
|
||
(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)
Assignee | ||
Comment 52•7 years ago
|
||
That makes sense to me, I can try, I guess... Though it's even more risky than this I would say?
Flags: needinfo?(emilio)
Assignee | ||
Comment 53•7 years ago
|
||
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.
Assignee | ||
Comment 54•7 years ago
|
||
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)
Comment 55•7 years ago
|
||
(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.
Assignee | ||
Comment 56•7 years ago
|
||
(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...
Comment 57•7 years ago
|
||
(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
Comment 58•7 years ago
|
||
datareview+ based on the explanation in comment 32.
Flags: needinfo?(francois)
Updated•7 years ago
|
Attachment #8924088 -
Flags: review?(francois) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8924087 -
Attachment is obsolete: true
Attachment #8924087 -
Flags: review?(jhofmann)
Attachment #8924087 -
Flags: review?(dao+bmo)
Comment 61•7 years ago
|
||
> 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)
Assignee | ||
Comment 62•7 years ago
|
||
(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 63•7 years ago
|
||
mozreview-review |
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+
Comment 64•7 years ago
|
||
(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 65•7 years ago
|
||
mozreview-review |
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 66•7 years ago
|
||
mozreview-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+
Comment 67•7 years ago
|
||
> 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...
Assignee | ||
Comment 68•7 years ago
|
||
(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.
Assignee | ||
Comment 69•7 years ago
|
||
(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.
Comment 70•7 years ago
|
||
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.
Comment 71•7 years ago
|
||
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.
Updated•7 years ago
|
Flags: needinfo?(emilio)
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 72•7 years ago
|
||
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)
Comment 73•7 years ago
|
||
Yeah, ok. If we plan to flip that switch soon, we should land this ASAP.
Flags: needinfo?(bzbarsky)
Comment 74•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8924778 -
Flags: review?(xidorn+moz) → review+
Comment 75•7 years ago
|
||
mozreview-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+
Comment 76•7 years ago
|
||
[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.
tracking-firefox58:
--- → ?
Comment 77•7 years ago
|
||
Since bug 1412560 is P1, this certainly should be as well.
Priority: P4 → P1
Comment 78•7 years ago
|
||
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
Comment 79•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0064de758725 https://hg.mozilla.org/mozilla-central/rev/fe14831a814b https://hg.mozilla.org/mozilla-central/rev/5c4cb77d75e5 https://hg.mozilla.org/mozilla-central/rev/9edca0017ec4
Comment 80•7 years ago
|
||
Any reason we need to leave it open? Can we just mark it fixed?
Flags: needinfo?(emilio)
Assignee | ||
Comment 81•7 years ago
|
||
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
Updated•7 years ago
|
Updated•7 years ago
|
Target Milestone: --- → mozilla58
You need to log in
before you can comment on or make changes to this bug.
Description
•