Closed Bug 1463600 (css-contain-style) Opened 6 years ago Closed 2 years ago

Implement CSS 'contain: style'

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
103 Branch
Tracking Flags:
Tracking Status
firefox103 --- fixed

People

(Reporter: dholbert, Assigned: mrobinson)

References

(Depends on 2 open bugs, Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete, Whiteboard: [layout:p2], [wptsync upstream])

Attachments

(1 file, 3 obsolete files)

Bug 1463600 - Implement CSS 'contain: style' r=emilio
48 bytes, text/x-phabricator-request
Details | Review
See https://drafts.csswg.org/css-contain/#containment-style This bug is on making our rendering engine react appropriately to "contain: style". (See also bug 1172087 which has a work-in-progress patch stack for an earlier draft of this spec.)
status-firefox62: affected → ---
Priority: -- → P3
After updating the previous implementation to the new system, "contain:style" now seems to achieve its main functionality for both counters and quotes for a test case each. However, it is a work in progress, so I am not expecting a review for the code yet. The reason I posted the unfinished code is for it to be a reference to my question below. There is an interesting behavior when running the test cases. For counter test, if I open the inspector, uncheck, and then check the "contain:style" property, the tab crashes. For quote test, the page crashes if I open the inspector (console, debugger, etc. are fine though, its specifically the inspector). In case its relevant, an interesting observation is that when we enable or disable contain:style property dynamically in inspector window in Google Chrome, it does not take any affect on the page. I filed a bug to Chrome about this last week, haven't received a response yet. I tried few things according to the unaddressed comments in the 2015 version (link in comment 1), and according to the resources I found in general. It seems to me that problem might be related to FindContainStyleRoot function or its usage in the NotifyDestroyingFrame, though I wasn't able to accurately identify the issue. I would really appreciate any suggestions or leads on how to diagnose this problem and which direction I should look. I've checked out some previous bugs (e.g. bug 1447483) to try to understand the changes to style since 2015. And I thought maybe emilio might have an opinion on this issue.
Taking a short step back to look at the overall picture. Below are just some notes on four major issues we have with our current contain:style implementation: --- 1) Issue described in comment 3. 2) Using elements with "display: inline" with counters and quotes crashes the page. 3) We are scoping to the element, and not to the subtree. Spec says we should scope to the subtree[1]. The difference is: "A scoped property has its effects scoped to a particular element or subtree. If scoped to an element, it must act as if the scoping element was the root of the document for the purpose of evaluating the property’s effects: any uses of the property outside the scoping element must have no effect on the uses of the property on or in the scoping element, and vice versa. If scoped to a sub-tree, it’s the same, except the scoping element itself is counted as "outside" the tree, like the rest of the document, and the effects of the property on that element are unaffected by scoping. When considering the effects of the scoped property on elements inside the subtree, the element at the base of the subtree is treated as if it was the root of the document." This seems to be same in the previous versions of the spec as well. And, current web-platform tests are written to scope to the subtree. Yet, both our 2015 implementation, and Chrome's current implementation of contain:style for counters, seemed to be scoping to the element. For now, I am planning to leave this as it is, i.e. scoping to the element. See [2] for the discussion of css working group on this matter. 4) According to the new spec, we should implicitly create a new counter when style contained [3]. --- After fixing 1,2, and 4, we should be in a decent condition, and can focus on the test cases. [1] https://drafts.csswg.org/css-contain/#containment-style [2] https://github.com/w3c/csswg-drafts/issues/2483 [3] https://github.com/w3c/csswg-drafts/issues/1887
Comment on attachment 8987763 [details] Bug 1463600 - Implement CSS 'contain: style'. https://reviewboard.mozilla.org/r/253038/#review260266 I guess the following issues may explain some of the problems you are seeing. Actually it makes me wonder how your impl works with these two issues around. ::: servo/components/style/properties/computed_value_flags.rs:45 (Diff revision 1) > > /// A flag used to mark styles which are a pseudo-element or under one. > const IS_IN_PSEUDO_ELEMENT_SUBTREE = 1 << 4; > > /// A flag used to mark styles which have contain:style or under one. > const HAS_CONTAIN_STYLE = 1 << 5; This flag needs to be added into `ComputedValueFlags::inherited_flags` below, otherwise it wouldn't be propagated into descendants automatically. ::: servo/components/style/style_adjuster.rs:217 (Diff revision 1) > } > > if self.style > .get_box() > .clone_contain() > .contains(SpecifiedValue::PAINT) This apparently is wrong... Shouldn't you be checking `STYLE` here?
(In reply to Xidorn Quan [:xidorn] UTC 10 (less responsive until July 7) from comment #5) > Comment on attachment 8987763 [details] xidorn, thank you very much for looking into this! Any comments, tips, or ideas are always extremely helpful. > I guess the following issues may explain some of the problems you are seeing. Upon fixing these points, issue #1 (comment 3) is now fixed for both quotes and counters! Issue #2 persists. I'm continuing to look into remaining issues. On a side note, I'm trying to digest some amount of code (and history) while narrowing it down with our issues, and consequently, getting carried away to details [that are possibly unrelated to this bug]. So, though the fixes are most likely very simple, they don't always immediately stand out. Hopefully once I got enough familiarity with the cockpit, it'll be fairly quicker for me to realize if there is a coffee cup on the eject button :) Thanks!
I don't think I saw anything else from the glimpse before, so I have no idea what's the reason for you issue 2. For reproducible crashes, it's usually very helpful to just attach a debugger to the process and trigger crash to let the debugger catch it, and with that you can check what state is going wrong in different level in the stack.
Comment on attachment 8987763 [details] Bug 1463600 - Implement CSS 'contain: style'. https://reviewboard.mozilla.org/r/253038/#review260812 I just took a quick look drive-by, hope it helps a bit to figure out what's going on wrong. If the crash is asserting on GetContainStyleRoot, I suspect it may be because of my comment regarding `mContainStyleRoot`... I don't know how it'd be initialized correctly in presence of dynamic changes of any sort. But as Xidorn said without a stack it's hard to figure out. There's also the questionable choice of choosing frames to be the `contain: style` roots (disclaimer, I haven't gone through the whole spec). But frames can be parented in ways that are different to the DOM, and even though counters are handled on frames, their position (which is what's relevant for `contain: style` is computed using the DOM). There's also the question of what happens when you specify `contain: style` on a `display: contents` element. I suspect that's more of a spec issue though. Thanks a lot for working on this! ::: layout/base/nsCSSFrameConstructor.cpp:8285 (Diff revision 1) > Element* root = mDocument->GetRootElement(); > if (!root) { > return false; > } > > AutoRestoreContainStyleRoot savedContainStyleRoot(mContainStyleRoot, aContent->GetPrimaryFrame()); There's no need for this, PostRestyleEvent is asynchronous. ::: layout/base/nsCSSFrameConstructor.cpp:10241 (Diff revision 1) > AutoRestore<uint16_t> savedDepth(mCurrentDepth); > if (mCurrentDepth != UINT16_MAX) { > mCurrentDepth; > } > > AutoRestoreContainStyleRoot savedContainStyleRoot(mContainStyleRoot, aFrame); It's not clear to me that this `mContainStyleRoot` business is sound at all. Basically, it seems to rely on it being correctly set **somewhere**, then that somewhere is yet to be found I think. Initializing it to null is not correct to handle dynamic changes in any way. For example, if I have a `contain: style` ancestor, and change display on a children of it, we'll run this code for the child, but not for the ancestor. Yet it looks like `mContainStyleRoot` still needs to know about the ancestor. Given `mContainStyleRoot` is intended to be an optimization, and that (at least to me) it's unclear how it's supposed to work, it may be easier to get the implementation done without it (that is, always calling `FindContainStyleRoot`, possibly moving it to `nsCounterManager`), and then, once that's working in all the complex cases (as you seem to have noticed, Blink's implementation is pretty broken for those, per https://bugs.chromium.org/p/chromium/issues/detail?id=855798), figure out if there's something slow and why, and then try to improve it. Unles there's a clean model of how the optimization is supposed to work of course. I guess it could work if it was a `Maybe<nsIFrame*>`, and it was initialized the first time `AutoRestorContainStyleRoot` was called, or something... But again making it work on frames its slightly fishy... In any case I think it's still a better idea to worry about it once the rest of the pieces are working. ::: layout/style/ComputedStyle.h:167 (Diff revision 1) > { > return bool(mBits & Bit::HasTextDecorationLines); > } > > // Does this style context or any of its ancestors have 'contain: style' set? > bool HasContainStyle() const Related to the other bit comment, this name (even though the function is well documented) seems to hint that the style has `contain: style`. Maybe it should be removed to `SelfOrAncestorHasContainStyle`? ::: servo/components/style/style_adjuster.rs:221 (Diff revision 1) > .clone_contain() > .contains(SpecifiedValue::PAINT) > { > self.style > .flags > .insert(ComputedValueFlags::HAS_CONTAIN_STYLE); If this flag is propagated down the tree, then it should be called `SELF_OR_ANCESTOR_HAS_CONTAIN_STYLE` or something like that, shouldn't it? Ditto for the computed style bit.
xidorn, emilio, thank you both very much! Here is where we are right now on Issue #2. Upon reexamining the code, it seems like "ancestor" in FindContainStyleRoot isn't guaranteed to be null in some codepaths(https://bugzilla.mozilla.org/show_bug.cgi?id=1172087#c57). I'll post this into the bug in the link as a followup. After adding a check for that, both quotes and counters are now working without errors when there is no contain:style enabled. When contain:style is enabled, both give errors (but new ones, which is good). After attaching a debugger and looking at call stack (as xidorn suggested), the issue for counters was that we were removing an element from a hash table that we are iterating, i.e. trying to remove an element from an "active" hash table. Please see (https://bugzilla.mozilla.org/show_bug.cgi?id=1403377) for details. After fixing that, we are now getting a new error. So, both counters and quotes with contain:style with a display:inline element still doesn't work, but we've made some progress. Hopefully, I'll look into the new errors tomorrow, and post the updated the code here as well. Either this is a combination of smaller errors and we are fixing them one-by-one, or there is a bigger issue, and we are just fixing the symptoms. (In reply to Emilio Cobos Álvarez (:emilio) from comment #8) > Comment on attachment 8987763 [details] > Bug 1463600 - Implement CSS 'contain: style'. > > There's also the questionable choice of choosing frames to be the `contain: > style` roots (disclaimer, I haven't gone through the whole spec). But frames > can be parented in ways that are different to the DOM, and even though > counters are handled on frames, their position (which is what's relevant for > `contain: style` is computed using the DOM). Thanks for mentioning this! I'm not completely sure of the exact problematic scenarios with using frames, but I'll keep this in mind as we progress. > There's also the question of what happens when you specify `contain: style` > on a `display: contents` element. I suspect that's more of a spec issue > though. I didn't see a discussion for this combination, but yes, we should support it according to spec. (Chrome seems to support it too: https://bugs.chromium.org/p/chromium/issues/detail?id=766650) > There's no need for this, PostRestyleEvent is asynchronous. Done. > It's not clear to me that this `mContainStyleRoot` business is sound at all. Basically, it seems to rely on it being correctly > set **somewhere**, then that somewhere is yet to be found I think. . . > In any case I think it's still a better idea to worry about it once the rest > of the pieces are working. I completely agree. It might be a better idea to just get this working for all test cases abiding the spec, and then we can work on the optimization. I'll continue to the current version tomorrow, but also planning to create an alternate version without the optimization. > should be removed to `SelfOrAncestorHasContainStyle`? For both name change suggestions: Will do. I have a few other cosmetic and repetition-avoiding updates as well that I'll do for the next push. Thanks!
Blocks: 1472885
Attachment #8987763 - Attachment is obsolete: true
Attachment #8987763 - Flags: review?(dholbert)
Comment on attachment 8987764 [details] Bug 1463600 - Implement CSS 'contain: style'. https://reviewboard.mozilla.org/r/253032/#review261192 I know you all are busy with lots of stuff, so thank you very much for helping out! :) ::: commit-message-1c62b:1 (Diff revision 2) > Bug 1463600 - Implement CSS 'contain: style'. r=dholbert I folded the commits for counters and quotes into one as they were tightly coupled. ::: layout/base/nsCSSFrameConstructor.h:2149 (Diff revision 2) > friend class nsFrameConstructorState; > > static > nsIFrame* FindContainStyleRoot(nsIFrame* aDescendant) { > nsIFrame* ancestor = aDescendant; > if (!ancestor || !ancestor->Style()->SelfOrAncestorHasContainStyle()) { (In reply to Yusuf Sermet [:yusuf] from comment #9) > Upon reexamining the code, it seems like "ancestor" in FindContainStyleRoot > isn't guaranteed to be null in some codepaths > (https://bugzilla.mozilla.org/show_bug.cgi?id=1172087#c57). After adding a check > for that, both quotes and counters are now working without errors when there > is no contain:style enabled. Adding a check for "ancestor" assures that we are not crashing for any quotes and counters, but the original issue with inlines persists. ::: layout/base/nsCSSFrameConstructor.cpp:1802 (Diff revision 2) > > case eStyleContentType_Counter: > case eStyleContentType_Counters: { > nsStyleContentData::CounterFunction* counters = data.GetCounters(); > nsCounterList* counterList = > - mCounterManager.CounterListFor(counters->mIdent); > mCounterManager.CounterListFor(GetContainStyleRoot(aParentFrame), @emilio, it seems like the main issue with using inlines with contain:style is that aParentFrame is null for inline elements, which is a problem because we are using frames as contain:style roots, as you expressed your concerns about. But it's not clear to me why inline elements don't have a parent frame yet, at least in the context of nsCSSFrameConstructor (https://searchfox.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#7109 AND https://searchfox.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#5919). Is there a way for us to work around this, or is there some other entity that we can use to track down contain:style other than frames? ::: servo/components/style/style_adjuster.rs:223 (Diff revision 2) > // XXX yusuf: Below chunk promotes inline to inline-block, because > // contain:style enabled inline-level elements crashes the page. > // This is just a workaround. Contain:style should be fixed to work > // with inline-level elements. > if display == Display::Inline { > self.style > .mutate_box() > .set_adjusted_display(Display::InlineBlock, false); > } Although the optimization might complicate things, the problem with inlines won't go away even if remove the all mContainStyleRoot mechanism. I added this to promote inlines to inline-blocks as a temporary solution. I think its better to aim to get main functionality working by having a temporary solution to this rather that being stuck in the same part. I filed another bug for this (https://bugzilla.mozilla.org/show_bug.cgi?id=1472885).
Comment on attachment 8987764 [details] Bug 1463600 - Implement CSS 'contain: style'. https://reviewboard.mozilla.org/r/253032/#review261192 > @emilio, it seems like the main issue with using inlines with contain:style is that aParentFrame is null for inline elements, which is a problem because we are using frames as contain:style roots, as you expressed your concerns about. But it's not clear to me why inline elements don't have a parent frame yet, at least in the context of nsCSSFrameConstructor (https://searchfox.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#7109 AND https://searchfox.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#5919). > > Is there a way for us to work around this, or is there some other entity that we can use to track down contain:style other than frames? That happens because at the time we create generated content for inlines we still don't have a frame at all. There's not much you can do about it without rewriting when generated content is generated, which is also pretty hard given generated content can have random display types which can make you create more frame items. I think style containment should work on the DOM (that is, on `aParentContent`, etc). That's the only real way to make `contain: style` work on `display: contents` really, and would solve this issue as well. > Although the optimization might complicate things, the problem with inlines won't go away even if remove the all mContainStyleRoot mechanism. > > I added this to promote inlines to inline-blocks as a temporary solution. I think its better to aim to get main functionality working by having a temporary solution to this rather that being stuck in the same part. I filed another bug for this (https://bugzilla.mozilla.org/show_bug.cgi?id=1472885). Either whether you keep this or not, this shouldn't be in `set_bits`... Also you should probably check the interactions between this and other bits of code that touch the `display` value.
(In reply to Emilio Cobos Álvarez (:emilio) from comment #12) > Comment on attachment 8987764 [details] > Bug 1463600 - Implement CSS 'contain: style'. > > That happens because at the time we create generated content for inlines we > still don't have a frame at all. > > There's not much you can do about it without rewriting when generated > content is generated, which is also pretty hard given generated content can > have random display types which can make you create more frame items. > > I think style containment should work on the DOM (that is, on > `aParentContent`, etc). That's the only real way to make `contain: style` > work on `display: contents` really, and would solve this issue as well. I see, that's really good to know so I won't beat my head against the wall to try to make it work with frames. I'll try updating it to work on DOM tomorrow. > Either whether you keep this or not, this shouldn't be in `set_bits`... Also > you should probably check the interactions between this and other bits of > code that touch the `display` value. You're right, I moved that chunk to a adjust_for_contain function as before. And to check the interactions, I'll add a test like we previously had for inline-block promotion for contain:paint: test_contain_formatting_context.html @ https://bugzilla.mozilla.org/page.cgi?id=splinter.html&ignore=&bug=1170781&attachment=8685093 Thanks a lot!
Comment on attachment 8987764 [details] Bug 1463600 - Implement CSS 'contain: style'. https://reviewboard.mozilla.org/r/253032/#review261452 Canceling review request, since I don't think this is ready for review yet. Ultimately, you should probably have emilio or xidorn review this (I could also review parts, but I'm not qualified to review all of it). But for now (whenever you next push updates here), you probably want to just drop the "r=" syntax from the end of your commit message, or else this will make Bugzilla prompt whoever you've got [me right now] to start reviewing it (unless they clear the review field on MozReview like I'm doing here). And then you can add reviewers manually on MozReview when you think it's ready for review (or just add the r=whoever syntax back to the commit message at that point). ::: commit-message-1c62b:1 (Diff revision 3) > Bug 1463600 - Implement CSS 'contain: style'. r=dholbert > > MozReview-Commit-ID: 8sCjqrKMook > *** > Bug 1463600 - Implement CSS 'contain: style' for quotes. r=dholbert > > MozReview-Commit-ID: HOuw3dgTfmO Nit: you've got a double commit message here [two in one], probably leftover from a histedit "fold" operation, which just concatenates the folded patches' messages so as not to lose information [but which usually isn't what you actually want].
Attachment #8987764 - Flags: review?(dholbert)
(In reply to Daniel Holbert [:dholbert] from comment #15) > Comment on attachment 8987764 [details] > Bug 1463600 - Implement CSS 'contain: style'. > > https://reviewboard.mozilla.org/r/253032/#review261452 > > Canceling review request, since I don't think this is ready for review yet. > Ultimately, you should probably have emilio or xidorn review this (I could > also review parts, but I'm not qualified to review all of it). > > But for now (whenever you next push updates here), you probably want to just > drop the "r=" syntax from the end of your commit message, or else this will > make Bugzilla prompt whoever you've got [me right now] to start reviewing it > (unless they clear the review field on MozReview like I'm doing here). And > then you can add reviewers manually on MozReview when you think it's ready > for review (or just add the r=whoever syntax back to the commit message at > that point). You're right, sorry about that. I was just updating to share the progress and have the code on reviewboard (useful for me too). I'll make sure to drop the "r=" part when I'm committing without needing reviews. > Nit: you've got a double commit message here [two in one], probably leftover > from a histedit "fold" operation, which just concatenates the folded > patches' messages so as not to lose information [but which usually isn't > what you actually want]. Will fix it! Thanks!
Just for future reference, here is a quick recap of the last push (comment 17): Fast forwarding quite a few bugs and running into walls here. We were able to update the tracking of the contain:style elements to use dom elements instead of frames, as emilio suggested. It indeed solved some crashing issues for inline and content elements (when the optimization was disabled, since that's a whole different story), but there were still issues: - Using display:contents with dynamic changes of contain:style was still buggy - Multi-level contain:style's does not work. I.e., descendant of a contain:style'd element also having contain:style, and so on. The 2nd issue seemed like it was a limitation of the approach, rather than a bug. (though I didn't put a lot of time on that as I'll explain below.) Also note that the current approach is not efficiently suitable to work with the new spec, which says that contain:style'd elements should be able to read from the past. Additionally, as emilio also explained (refer to comment 8), AutoRestoreContainStyleRoot "business" is indeed not sound. Thus, I think it's best for us change our perspective a little, so I tried to come with another approach or pivot the current one, which I'll explain in another comment today.
Comment on attachment 8987764 [details] Bug 1463600 - Implement CSS 'contain: style'. https://reviewboard.mozilla.org/r/253032/#review265258 Code analysis found 6 defects in this patch: - 6 defects found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check path/to/file.cpp` (C/C ) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: layout/base/nsQuoteList.h:107 (Diff revision 6) > // Note: It also reduces runtime for resetting logic. > int32_t GetDepthBeforeForStyleLevels(uint32_t aCustomLevel) { > if (aCustomLevel >= mDepthBeforeForStyleLevels.size()) { > return mDepthBeforeForStyleLevels.back(); > } > else { Warning: Different indentation for 'if' and corresponding 'else' [clang-tidy: readability-misleading-indentation] else { ^ ::: layout/base/nsQuoteList.h:107 (Diff revision 6) > // Note: It also reduces runtime for resetting logic. > int32_t GetDepthBeforeForStyleLevels(uint32_t aCustomLevel) { > if (aCustomLevel >= mDepthBeforeForStyleLevels.size()) { > return mDepthBeforeForStyleLevels.back(); > } > else { Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return] else { ^~~~~~ ::: layout/base/nsQuoteList.h:107 (Diff revision 6) > // Note: It also reduces runtime for resetting logic. > int32_t GetDepthBeforeForStyleLevels(uint32_t aCustomLevel) { > if (aCustomLevel >= mDepthBeforeForStyleLevels.size()) { > return mDepthBeforeForStyleLevels.back(); > } > else { Warning: Different indentation for 'if' and corresponding 'else' [clang-tidy: readability-misleading-indentation] else { ^ ::: layout/base/nsQuoteList.h:107 (Diff revision 6) > // Note: It also reduces runtime for resetting logic. > int32_t GetDepthBeforeForStyleLevels(uint32_t aCustomLevel) { > if (aCustomLevel >= mDepthBeforeForStyleLevels.size()) { > return mDepthBeforeForStyleLevels.back(); > } > else { Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return] else { ^~~~~~ ::: layout/base/nsQuoteList.h:107 (Diff revision 6) > // Note: It also reduces runtime for resetting logic. > int32_t GetDepthBeforeForStyleLevels(uint32_t aCustomLevel) { > if (aCustomLevel >= mDepthBeforeForStyleLevels.size()) { > return mDepthBeforeForStyleLevels.back(); > } > else { Warning: Different indentation for 'if' and corresponding 'else' [clang-tidy: readability-misleading-indentation] else { ^ ::: layout/base/nsQuoteList.h:107 (Diff revision 6) > // Note: It also reduces runtime for resetting logic. > int32_t GetDepthBeforeForStyleLevels(uint32_t aCustomLevel) { > if (aCustomLevel >= mDepthBeforeForStyleLevels.size()) { > return mDepthBeforeForStyleLevels.back(); > } > else { Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return] else { ^~~~~~
This comment only focuses on quotes (not counters). Here is the new logic to apply style containment for quotes: Currently we have a quote list for all quotes in a page, in which each quote is represented as a node. Instead of generating new lists and changing our whole handling of the quotes, we can just augment our node class. Below scenario explains how Firefox's current handling of quotes work; open = that DOM element have an open quote as content close = that DOM element have an close quote as content Level| DOM Tree | Depth Before This Node 0 | open | 0 0 | open | 1 1 | open | 2 2 | open | 3 0 | close | 4 0 | close | 3 Basically, once the "depth before" values are calculated for each node, "text" function decides the printed value using whether it's an open or close quote. After reading our code for quote handling, to be able to make style containment work, it seemed like the only thing that we should really care about is to correctly set the quote depth for each node (i.e. an element with a quote content). We can have an additional member variable for each node, that keeps the number of contain:style's its ancestors have, which we will calculate when we initialize the node. So, we turn the "depth before" variable for a node, to a vector that keeps quote depths for each contain:style level. If a node is in level 2, all children nodes' quote depths will take that node into account. However, none of the ancestor (or anything outside of its scope) nodes' quote depths will be aware that the contain:styled descendants even existed. Consider above example, and assume the level means the # contain:styled ancestors. New structure will look like this; Level| DOM Tree | Depth Before This Node 0 | open | 0 0 | open | 1 1 | open | 2 | 0 2 | open | 3 | 1 | 0 0 | close | 2 0 | close | 1 When "text" function is deciding which quote to print, it reads the "depth before" value from its own "contain style level", meaning if it has 2 ancestors with contain:style, it'll read the 3rd (index 2) element in our new "depth before" vector. Now, this is all good for most basic scenarios. However, to demonstrate the major issue with this; 0 | open | 0 2 | open | 1 | 0 | 0 2 | open | 1 | 0 | *1* 0 | open | 1 2 | open | 2 | 0 | *0* As you can see above, the below starred value will inherit from the above starred value, because they are in the same style level, despite their ancestors being different (and hence they shouldn't be aware of each other's existent according to the spec). So, we need to track the contain:style roots for each node, so that we can decide if the previous node was in our scope. If not, we should treat those style levels as if they don't exist. However, it's not memory efficient to keep DOM Element pointers for contain:style roots for each quote node. So, we only have one dynamic "contain style root vector" in nsCSSFrameConstructor that we use to keep track of things. The maximum number of elements it can get at a given t is equal to the maximum number of nesting contain:style's. In summary, the push in comment 20 implements this whole approach. Currently, all the tests I ran locally work including: - All display types including contents and inline - All dynamic changes - Nested contain:style's - Reading previous values - Not affecting the quotes outside of the scope - Regular quotes without any contain:style - And most importantly (and difficultly), multiple nested contain:style's with completely or partially different scopes, with or without quote nodes in each level. I think this approach itself can be considered as somewhat an optimization. There are also a bunch of implementation choices, fast paths, and optimizations that I didn't get into detail here. But there is of course quite a big room for further optimizations, selection of data structures, cleaning etc. But I think it's in a decent enough condition to get a review before going further. One more advantage of this method is that we are not adding any kind of overhead to the pages that doesn't have quotes. Please feel free to point out any points or cases I may have missed in this logic, or any suggestions for optimizations. :emilio, I would really appreciate your take on this. One thing to note, current patch also includes the old changes for counters. There is no clean cut between them, but I think looking at the diff between revisions 4-6 might be the most cleaner to focus on the relevant changes.
I haven't looked in detail yet, but I think the approach of using a style bit for this is not great, because it's going to break with Shadow DOM, since the inheritance chain is not the same to the GetParentElement() chain. So you need to either use the flattened tree (GetFlattenedTreeParentElement() instead of GetParentElement()), or stop using a style context bit. In particular, the following test-case should catch your '// weird, we were expecting to find an element with contain:style' path: <!doctype html> <style> #light::before { content: open-quote } </style> <div id="host"> <div id="light"></div> </div> <script> host.attachShadow({ mode: 'open' }).innerHTML = `<slot id="slot" style="contain: style"></slot>`; </script> Here, the GetParentElement() chain of #light would be: #host -> body -> html. However, the style inheritance chain (GetFlattenedTreeParent()) is: #light -> #slot -> #host -> body -> html. Which means that #light would end up with a SelfOrAncestorHasContainStyle() returning true, but none of the GetParentElement()'s would be contain: style.
Comment on attachment 8987764 [details] Bug 1463600 - Implement CSS 'contain: style'. https://reviewboard.mozilla.org/r/253032/#review265346 I haven't reviewed all the style level logic, but could you summarize why does contain: style require knowledge in the counters code of all the ancestor contain style roots? It's unclear to me why that'd be needed... You should be able to just ignore a content node if the contain: style root is different. Why are the nodes linked together? In any case, I think Xidorn would be a better person to give feedback on those changes. ::: layout/base/nsCSSFrameConstructor.h:2170 (Diff revision 7) > mozilla::ArenaAllocator<4096, 8> mFCItemPool; > struct FreeFCItemLink { FreeFCItemLink* mNext; }; > FreeFCItemLink* mFirstFreeFCItem; > size_t mFCItemsInUse; > > std::vector<Element*> mContainStyleRoots; This isn't cleared after we do frame construction, which looks fishy at best. What guarantees that the elements here are valid next time we get there? ::: layout/base/nsCSSFrameConstructor.cpp:1668 (Diff revision 7) > QuotesDirty(); > - } > } > } > > // Below is for counters > Element* findStyleRootFor = nullptr; nit: This could be findStyleRootFor = Element::FromNodeOrNull(aFrame->GetContent()). But is there any reason why all the contain style root stuff can't move to the quotes code? Looks like there's nothing particularly specific to the frame constructor here. ::: layout/base/nsCSSFrameConstructor.cpp:1798 (Diff revision 7) > } > > if (resetFrom != 0 && resetFrom-1 < mContainStyleRoots.size()) { > mContainStyleRoots.erase(mContainStyleRoots.begin() resetFrom - 1, mContainStyleRoots.end()); > } > mContainStyleRoots.insert(mContainStyleRoots.end(), newContainStyleRoots.begin(), newContainStyleRoots.end()); This could get some comments. ::: layout/base/nsCSSFrameConstructor.cpp:12026 (Diff revision 7) > if (!ancestor) { > return nullptr; > } > > RefPtr<ComputedStyle> computedStyle = > nsComputedDOMStyle::GetComputedStyleNoFlush(ancestor, nullptr); ResolveStyleFor instead. ::: layout/base/nsCSSFrameConstructor.cpp:12057 (Diff revision 7) > if (!ancestor) { > return; > } > > RefPtr<ComputedStyle> computedStyle = > nsComputedDOMStyle::GetComputedStyleNoFlush(ancestor, nullptr); ResolveStyleFor instead of this. Also the caller should have the first style already. ::: layout/base/nsCSSFrameConstructor.cpp:12067 (Diff revision 7) > } > > while (computedStyle && > !computedStyle->StyleDisplay()->IsContainStyle()) { > ancestor = ancestor->GetParentElement(); > if (!ancestor) { This should probably be a MOZ_ASSERT. ::: layout/base/nsCounterManager.h:268 (Diff revision 7) > int32_t aIndex, > const nsStyleCounterData& aCounterData, > nsCounterNode::Type aType); > > - nsClassHashtable<nsStringHashKey, nsCounterList> mNames; > NameTable* GetNames(Element* aRoot, bool aShouldCreate = true) { > NameTable* names = mContainStyleRoots.Get(aRoot); nit: two space indent. Also, this should probably use LookupForAdd, to remove one hashtable lookup in the aShouldCreate case. ::: layout/base/nsCounterManager.cpp:217 (Diff revision 7) > } > return dirty; > } > > bool > -nsCounterManager::AddResetOrIncrement(nsIFrame* aFrame, int32_t aIndex, > nsCounterManager::AddResetOrIncrement(Element *aRoot, nsIFrame* aFrame, nit: Gecko style is always use the `*` and `&` near the type. ::: layout/base/nsCounterManager.cpp:277 (Diff revision 7) > } > } > } > > bool > -nsCounterManager::DestroyNodesFor(nsIFrame* aFrame) > nsCounterManager::DestroyNodesFor(Element *aRoot, nsIFrame* aFrame) This function is still assuming that you have a frame here, which means that there's no way this works for display: contents right? If there's only a contain: style display: contents node, and no frames below it, would we leave a stale pointer to it in `mContainStyleRoots`? ::: layout/base/nsCounterManager.cpp:289 (Diff revision 7) > // This is unexpected behaviour, but can happen. When happens, > // continue execution instead of crashing. > // If happens, it means we messed up tracking contain style root > // Diagnose AutoRestoreContainStyleRoot, FindContainStyleRoot, etc. > if (MOZ_UNLIKELY(!rootNames)) { > NS_WARNING("Trying to access uninit'ed counter lists."); This should really be a `MOZ_ASSERT`. How can this happen? ::: layout/base/nsCounterManager.cpp:297 (Diff revision 7) > for (auto iter = rootNames->Iter(); !iter.Done(); iter.Next()) { > nsCounterList* list = iter.UserData(); > if (list->DestroyNodesFor(aFrame)) { > destroyedAny = true; > list->SetDirty(); > rootElementsToBeRemoved.push_back(aRoot); This would only end up pushing `aRoot` (potentially multiple times) at most, right? I don't know how this is supposed to work.
(In reply to Emilio Cobos Álvarez (:emilio) from comment #24) > In particular, the following test-case should catch your '// weird, we were > expecting to find an element with contain:style' path: > . > . > Which means that #light would end up with a SelfOrAncestorHasContainStyle() > returning true, but none of the GetParentElement()'s would be contain: style. Thanks a lot for all your comments and pointing out this case! I saved that test-case as a reftest. After updating to use GetFlattenedTreeParentElement(), it's now working for shadow DOM as well. Do you think we should add an assertion to that "//weird,..." path now? > So you need to either use the flattened tree > (GetFlattenedTreeParentElement() instead of GetParentElement()), or stop > using a style context bit. I think using the style context bit really increases our performance so that we don't have to always check all ancestors for contain:style (all credit to dbaron). Of course, if that bit will have another use, we can definitely come up with other approaches to reduce the runtime for contain:style search.
(In reply to Yusuf Sermet [:yusuf] from comment #26) > (In reply to Emilio Cobos Álvarez (:emilio) from comment #24) > > In particular, the following test-case should catch your '// weird, we were > > expecting to find an element with contain:style' path: > > . > > . > > Which means that #light would end up with a SelfOrAncestorHasContainStyle() > > returning true, but none of the GetParentElement()'s would be contain: style. > > Thanks a lot for all your comments and pointing out this case! I saved that > test-case as a reftest. > > After updating to use GetFlattenedTreeParentElement(), it's now working for > shadow DOM as well. Do you think we should add an assertion to that > "//weird,..." path now? The tricky part of this is: Is that really what the spec says? (I'm not sure, it's definitely not what Blink implements). Also tricky is ensuring that changes to the flattened tree are reflected properly (i.e., moving an element from a <slot> to another one changing the `slot` attribute). But we need to update the frame tree already, so I think that should just work. Note that this is related to https://github.com/w3c/csswg-drafts/issues/2679. I think the general consensus is that the flattened tree should be used, but that's not what we implement right now (nsLayoutUtils::CompareTreePosition uses the normal tree position, GetParentNode()). If we decide to use the flattened tree, which is fine probably, we should probably update that to use the flattened tree as a prerequisite. Probably worth filing a dependent bug? > > So you need to either use the flattened tree > > (GetFlattenedTreeParentElement() instead of GetParentElement()), or stop > > using a style context bit. > > I think using the style context bit really increases our performance so that > we don't have to always check all ancestors for contain:style (all credit to > dbaron). Of course, if that bit will have another use, we can definitely > come up with other approaches to reduce the runtime for contain:style search. Sure, I agree the style context bit is worth it in theory, we just can't do it (in a sound way) if we don't use the flattened tree.
In any case, MOZ_ASSERTs are good :)
(In reply to Emilio Cobos Álvarez (:emilio) from comment #25) > Comment on attachment 8987764 [details] > Bug 1463600 - Implement CSS 'contain: style'. > > https://reviewboard.mozilla.org/r/253032/#review265346 > > I haven't reviewed all the style level logic, but could you summarize why > does contain: style require knowledge in the counters code of all the > ancestor contain style roots? It's unclear to me why that'd be needed... You > should be able to just ignore a content node if the contain: style root is > different. Why are the nodes linked together? Sorry for not making this clearer, but as of comment 18, I decided to handle quotes and counters separately. And comment 22 and code updates since was only about quotes. Counters are still problematic, which hopefully I'll look into next. Thanks a lot for all your points for counters, which I'll benefit greatly. > ::: layout/base/nsCSSFrameConstructor.h:2170 > (Diff revision 7) > > mozilla::ArenaAllocator<4096, 8> mFCItemPool; > > struct FreeFCItemLink { FreeFCItemLink* mNext; }; > > FreeFCItemLink* mFirstFreeFCItem; > > size_t mFCItemsInUse; > > > > std::vector<Element*> mContainStyleRoots; > > This isn't cleared after we do frame construction, which looks fishy at > best. What guarantees that the elements here are valid next time we get > there? You're right, I think we can clear that at the same time we clear the mQuoteList (https://searchfox.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#8260). > > ::: layout/base/nsCSSFrameConstructor.cpp:1668 > (Diff revision 7) > > QuotesDirty(); > > - } > > } > > } > > > > // Below is for counters > > Element* findStyleRootFor = nullptr; > > nit: This could be findStyleRootFor = > Element::FromNodeOrNull(aFrame->GetContent()). > > But is there any reason why all the contain style root stuff can't move to > the quotes code? Looks like there's nothing particularly specific to the > frame constructor here. I'll use your suggested function. Just a note, this above bit is for counters (which is unstable as I explained above). I think the main reason to have the contain:style root things in frameconstructor is to not repeat any code in counters and quotes and do as much of the shared work once. So yes, although we can definitely move all the stuff for quotes to the quotes code, I think we can leave it here until we got the counters done. And then maybe we can move some code to nsGenConList as well, which both quotes and counters inherits from. > ::: layout/base/nsCSSFrameConstructor.cpp:1798 > (Diff revision 7) > > } > > > > if (resetFrom != 0 && resetFrom-1 < mContainStyleRoots.size()) { > > mContainStyleRoots.erase(mContainStyleRoots.begin() resetFrom - 1, mContainStyleRoots.end()); > > } > > mContainStyleRoots.insert(mContainStyleRoots.end(), newContainStyleRoots.begin(), newContainStyleRoots.end()); > > This could get some comments. Here is an example; 0 | open | 0 2 | open | 1 | 0 | 0 4 | open | 1 | 0 | 1 | 0 | 0 After processing the second quote, we have 2 contain:styled ancestors in our mContainStyleRoots. Now, when we process the third quote, we see that the last ancestor in mContainStyleRoots is actually our ancestor as well, but we have 2 more levels (nested contain:styled elements) on top of that, i.e. we went deeper in the nesting. So, instead of continuing to travel all ancestors, we break there since we know what we will find after that. So, that last line just merges the new ancestors with the old ones. This was an obvious optimization that felt like we should implement. The if statement above that does resetting, if necessary. An example scenario for that: <div class="container"> // Level 1 <div class="container"> // Level 1.1 <div class="container qclose"></div> // Level 1.1.1 </div> <div class="container qclose"></div> // Level 1.2 </div> When we are processing the quote at 1.2, we have the ancestors for 1, 1.1, and 1.1.1 in mContainStyleRoots. For this one, we should only have 1 and 1.2. So, we remove 1.1 and 1.1.1 (if statement does this), and add 1.2 (the line below the if). I'll add comments to the code. > > while (computedStyle && > > !computedStyle->StyleDisplay()->IsContainStyle()) { > > ancestor = ancestor->GetParentElement(); > > if (!ancestor) { > > This should probably be a MOZ_ASSERT. Sorry I missed this before writing comment 26. Got it, MOZ_ASSERT is good :) > > -nsCounterManager::AddResetOrIncrement(nsIFrame* aFrame, int32_t aIndex, > > nsCounterManager::AddResetOrIncrement(Element *aRoot, nsIFrame* aFrame, > > nit: Gecko style is always use the `*` and `&` near the type. Got it.
(In reply to Emilio Cobos Álvarez (:emilio) from comment #27) > (In reply to Yusuf Sermet [:yusuf] from comment #26) > > The tricky part of this is: Is that really what the spec says? (I'm not > sure, it's definitely not what Blink implements). For what its worth, Chrome's implementation of contain:style is not complete on many levels according to spec. > > Also tricky is ensuring that changes to the flattened tree are reflected > properly (i.e., moving an element from a <slot> to another one changing the > `slot` attribute). But we need to update the frame tree already, so I think > that should just work. > > Note that this is related to > https://github.com/w3c/csswg-drafts/issues/2679. I think the general > consensus is that the flattened tree should be used, but that's not what we > implement right now (nsLayoutUtils::CompareTreePosition uses the normal tree > position, GetParentNode()). > > If we decide to use the flattened tree, which is fine probably, we should > probably update that to use the flattened tree as a prerequisite. Probably > worth filing a dependent bug? I was checking out that functions its' usages. And it seems like you already updated that to use the flattened tree here (https://searchfox.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#1632) as part Bug 1414303. Can you please detail on this more if I'm missing something? > Sure, I agree the style context bit is worth it in theory, we just can't do > it (in a sound way) if we don't use the flattened tree. True, and thanks a lot for catching that.
(In reply to Yusuf Sermet [:yusuf] from comment #30) > I was checking out that functions its' usages. And it seems like you already > updated that to use the flattened tree here > (https://searchfox.org/mozilla-central/source/layout/base/nsLayoutUtils. > cpp#1632) as part Bug 1414303. Can you please detail on this more if I'm > missing something? Yes, GetParentOrHostNode() != GetFlattenedTreeParent(). GetParentOrHostNode uses the normal document order. So, in the example in comment 24, for #light, GetParentOrHostNode() is the same as GetParentElement(). For stuff in a Shadow Root, like the <slot>, GetParentOrHostNode() would be: #shadow-root -> #host -> ... While GetFlattenedTreeParent() would be: #host -> ... So, GetParentOrHostNode is effectively GetParentNode, plus a jump from the shadow root to the shadow host, which is _not_ GetFlattenedTreeParent(), which is the tree layout operates in. I suspect switching those functions to GetFlattenedTreeParent() is easy. Should be a matter of just s/GetParentOrHostNode/GetFlattenedTreeParent, and (the a bit more tricky part): Changing the ComputeIndexOf calls to use FlattenedTreeIterator: https://searchfox.org/mozilla-central/rev/8384a6519437f5eefbe522196f9ddf5c8b1d3fb4/layout/base/nsLayoutUtils.cpp#1691
Depends on: 1477524
(In reply to Emilio Cobos Álvarez (:emilio) from comment #31) That makes sense, thanks lot for clarifying that for me. I'll continue to work on current bug to hopefully get it to a decent shape, but I added a dependent bug for comment 31 to look into the ComputeIndexOf parts.
A quick overview of last week (and also some notes for anyone who'll work on this in the future): I'd like to think there are 3 major (and tightly-coupled) components for this bug; a mechanism for tracking style containment, its application to quotes, and to counters. Last week, the mechanism and its application to quotes was passing our test cases. After applying it to counters (fast-forwarding details), it didn't work. Naturally, I assumed the problem is with counters and focused on that, which wasn't a success. Then, Drude [1] came to rescue, and made me realize that I did two logical mistakes in the mechanism and its implm. to quotes, that cancel each other out, and causing it to actually give correct results for quotes. After fixing those (which was a joy), quotes are continuing to work decently, and counters are working as expected [2]. Along with that, we did some cleaning and relocation for some code to reduce repetition and inefficiency (e.g. moving stuff from nsQuoteNode and nsCounterNode to nsGenConNode, etc.). This can be seen between versions 8-9 (diff/8-9). [1] A fun trivia; Paul Drude (known for Drude Theory) calculated what is now known as Lorentz number by making two wrong assumptions that cancel each other out, and result in a somewhat correct approximation. Despite the errors, it was a breakthrough and used for at least a quarter of a century. https://books.google.com/books?id=lSNoAgAAQBAJ&lpg=PP1&pg=PA23#v=onepage&q&f=false [2] There are known issues with counters that I'll mention in the next comments.
Remaining Issues: 1) Counters are problematic for consecutive contain:style'd elements with the same number of contain:style'd ancestors. Consider below example: > body { > counter-reset: chapter; > } > h1::before { > content: counter(chapter) ". "; > counter-increment: chapter; > } > div { > contain:style; > } > <h1></h1> > <div> > <h1></h1> <!-- Has 1 contain:style ancestor --> > </div> > <!-- No other counter nodes between them --> > <div> > <h1></h1> <!-- Also has 1 contain:style ancestor, but different from the one above --> > </div> > <h1></h1> What it's printing | What it should've print 1. | 1. 2. | 2. 3. | 2. 2. | 2. I worked on this this weekend, and was able to identify the problem. Due to time limitations, I haven't fixed this. So, here is a summary on why this happens and how to fix it. What is the problem: There are two types of counter nodes (USE, CHANGE). Both counter nodes can be kept in the same counter list. During Firefox's current execution, if an element has both, USE nodes are created before CHANGE nodes. However, CHANGE nodes are added to the counter list *before* the USE nodes. This seems a little counter-intuitive, but it seems like it is necessary. This didn't cause any issue before, because order of these nodes didn't matter. However, with style containment, it is vital to add the counter nodes to the counter list in the same order as their creation. How to fix it: The only thing that matters is to set "mResetFrom" variable in the correct order. Other variables do not care about the order. So, the steps are: 1. Save the "mResetFrom" value of the first-ever created counter node of an element 2. Assign that "mResetFrom" value to the first counter node (of the same element) that has been added to the counter list (fyi: list insertion for UseNode happens in nsCounterManager::InitTextFrame) 3. Assign "0" to "mResetFrom" for all the other counter nodes in that element.
Remaining Issues: 2) The WG says [1] ::before child should be able to read outside its scope (fyi: this is what we're doing for quotes as well), but ::after child should not read from outside (i.e. it should create a new counter even if there is a counter with the same name outside of its scope). My understanding was that style-contained elements will read from outside, but will not change it. Thus, this implementation assumes that. (Hence, not passing Gerard's tests in comment 35). It's not clear to me why ::after child should not read outside its scope. Like ::before, it's also a child of the style-contained element, and consequently, part of its subtree. Note: Just for clarification, I don't think Chrome implements this as well. It seems like they are just scoping whatever inside the contain:style'd element to its own subtree, and do not let it read from outside (even if it's ::before). So, it seems like either their take on this is different, or they are in the process of implementation as well. [1] https://github.com/w3c/csswg-drafts/issues/2483
Comment on attachment 8987764 [details] Bug 1463600 - Implement CSS 'contain: style'. https://reviewboard.mozilla.org/r/253032/#review268698 ::: layout/base/nsCSSFrameConstructor.h:2175 (Diff revision 9) > std::vector<Element*> mContainStyleRootsForQuotes; > std::vector<Element*> mContainStyleRootsForCounters; We have 2 variables to keep track of the contain:style roots for quotes and counters. In CSSFrameConstructor, we are calculating these in 3 different locations (Quotes, CounterUse, CounterChange). For clarity and effiency, I'd like to suggest to create a new class (e.g. StyleContainmentManager) that will manage all the related code inside, and keep track of things efficiently. This way, we'll be able to move some code (e.g. functions SetContainStyleParameters, FindContainStyleRootsAndLevel, etc.) to this class from the FrameConstructor. It would look more concise as well. Instead of this 2 vectors, we'll have two StyleContainmentManager pointers; one for quotes, one for counters. emilio, does this seem like a reasonable action? If so, do you think creating a brand new class is OK?
Flags: needinfo?(emilio)
emilio, as MozReview is at its final days, I am in the process of installing Phabricator, and hopefully will ask for your review from there so that it won't be inconvenient for you to continue the review process. Meantime, I wanted to update the latest code here along with some notes (comments 38-41) so that you'll be able to take a look if you'd like. So feel free to share any suggestions or to point out anything that comes to your attention. I'd greatly appreciate that! I just updated my repo, and merged (installed bunch of updates...). I'm hoping to initiate the Phabricator review request tomorrow. Thanks a lot!
Comment on attachment 8987764 [details] Bug 1463600 - Implement CSS 'contain: style'. https://reviewboard.mozilla.org/r/253032/#review268698 > We have 2 variables to keep track of the contain:style roots for quotes and counters. In CSSFrameConstructor, we are calculating these in 3 different locations (Quotes, CounterUse, CounterChange). For clarity and effiency, I'd like to suggest to create a new class (e.g. StyleContainmentManager) that will manage all the related code inside, and keep track of things efficiently. This way, we'll be able to move some code (e.g. functions SetContainStyleParameters, FindContainStyleRootsAndLevel, etc.) to this class from the FrameConstructor. > > It would look more concise as well. > > Instead of this 2 vectors, we'll have two StyleContainmentManager pointers; one for quotes, one for counters. > > emilio, does this seem like a reasonable action? If so, do you think creating a brand new class is OK? I'm still unsure about why these two need to live in nsCSSFrameConstructor, shouldn't it leave in nsFrameConstructorState? These looks really temporary, and depending on which element is being constructed or not. Keeping all this raw pointers to Element nodes across random script running looks a bit of a footgun. For example, what happens with something like: ``` <style> .gencon::before { content: counter(foo); } /* Doesn't really matter, just something that depends on the contain style roots */ </style> <div style="contain: style" id="a"> <div style="contain: style" id="b"> <div style="contain: style" id="c"> <div class="gencon"><div> </div> </div> </div> ``` Let's say we load that page, and now I do something like moving DOM elements around so `a` is wrapped in a new contain style root. Wouldn't we, when we reconstruct frames for `gencon` again, fidnd that `c` is already in `mContainStyleRootsForCounters` and try to reuse the old thing? Also, elements can move away from the DOM and get GCd and what not, and looks like we don't use them, but it looks a bit fishy.
(In reply to Yusuf Sermet [:yusuf] from comment #40) > Remaining Issues: > > 2) The WG says [1] ::before child should be able to read outside its scope > (fyi: this is what we're doing for quotes as well), but ::after child should > not read from outside (i.e. it should create a new counter even if there is > a counter with the same name outside of its scope). My understanding was > that style-contained elements will read from outside, but will not change > it. Thus, this implementation assumes that. (Hence, not passing Gerard's > tests in comment 35). I agree that ::before should be no different to ::after, fwiw. (In reply to Yusuf Sermet [:yusuf] from comment #41) > Comment on attachment 8987764 [details] > Bug 1463600 - Implement CSS 'contain: style'. > > https://reviewboard.mozilla.org/r/253032/#review268698 > > ::: layout/base/nsCSSFrameConstructor.h:2175 > (Diff revision 9) > > std::vector<Element*> mContainStyleRootsForQuotes; > > std::vector<Element*> mContainStyleRootsForCounters; > > We have 2 variables to keep track of the contain:style roots for quotes and > counters. In CSSFrameConstructor, we are calculating these in 3 different > locations (Quotes, CounterUse, CounterChange). For clarity and effiency, I'd > like to suggest to create a new class (e.g. StyleContainmentManager) that > will manage all the related code inside, and keep track of things > efficiently. This way, we'll be able to move some code (e.g. functions > SetContainStyleParameters, FindContainStyleRootsAndLevel, etc.) to this > class from the FrameConstructor. Why do we need to track two different set of roots instead of just one? That should probably be described in a comment somewhere. Looks like, given a particular element, contain: style roots should be the same, regardless of whether you use counters or quotes. It is to handle resets or such? > It would look more concise as well. > > Instead of this 2 vectors, we'll have two StyleContainmentManager pointers; > one for quotes, one for counters. I assume they don't really need to be pointers, and they can just be members, like mCounterManager is, right? Maybe getting generated content split in three is not great, and we could maybe do something like adding a "generated content manager" or something that itself contains the counter manager? Anyway, not sure how the setup you're describing would look just yet, but anything that makes the code more straight-forward and easy to understand / change is great IMO. I wouldn't be fearful of introducing new abstractions if they make the code nicer :)
Flags: needinfo?(emilio)
Comment on attachment 8987764 [details] Bug 1463600 - Implement CSS 'contain: style'. https://reviewboard.mozilla.org/r/253032/#review268896 Code analysis found 1 defect in this patch: - 1 defect found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check path/to/file.cpp` (C/C ) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: layout/base/nsCounterManager.h:234 (Diff revision 10) > * counter to keep track of all scopes with that name. > */ > class nsCounterManager { > public: > // Returns true if frame has counter-reset or counter-increment > bool HasCounterResetsAndIncrements(nsIFrame* aFrame, const nsStyleContent* aStyleContent = NULL); Warning: Use nullptr [clang-tidy: modernize-use-nullptr] bool HasCounterResetsAndIncrements(nsIFrame* aFrame, const nsStyleContent* aStyleContent = NULL); ^~~~~ nullptr
MozReview-Commit-ID: 5w6WvUSf1oO
(In reply to Emilio Cobos Álvarez (:emilio) from comment #43) > I'm still unsure about why these two need to live in nsCSSFrameConstructor, > shouldn't it leave in nsFrameConstructorState? You're right, having that two in the nsFrameConstructorState makes more sense. I'll update it accordingly. > Let's say we load that page, and now I do something like moving DOM elements > around so `a` is wrapped in a new contain style root. Wouldn't we, when we > reconstruct frames for `gencon` again, fidnd that `c` is already in > `mContainStyleRootsForCounters` and try to reuse the old thing? From what I understand, this shouldn't be a problem. Because we are not actually saving the style roots for each element, which would be too costly in terms of memory. For your example, we won't do anything for those containing divs, but only gencon will trigger our mechanism for finding contain:style roots. Thus, it'll find any dynamically added parents as well, if I'm understanding this correctly. Just to demonstrate how we are using `mContainStyleRootsForCounters`: ``` <style> .gencon::before { content: counter(foo); } </style> <div style="contain: style" id="a"> <div style="contain: style" id="b"> <div style="contain: style" id="c"> <div class="gencon"><div> <div class="gencon"><div> </div> </div> </div> ``` First gencon is calculated normally, and all of the style roots are now saved in `mContainStyleRootsForCounters`. Now, the next counter node (in this case, the 2nd gencon), will start searching for style roots, and will find that the innermost style root (id: c) is also in the `mContainStyleRootsForCounters` from the previous counter node. So, it'll stop searching more and will use the roots in `mContainStyleRootsForCounters`. So, every counter node can only see the style roots for only its previous node. The only way this can cause a problem is if our DOM tree changes right between creating two consecutive counter nodes. Do you think this can cause any complications?
(In reply to Emilio Cobos Álvarez (:emilio) from comment #44) > Why do we need to track two different set of roots instead of just one? That > should probably be described in a comment somewhere. > > Looks like, given a particular element, contain: style roots should be the > same, regardless of whether you use counters or quotes. It is to handle > resets or such? We wouldn't need to have two different set of roots if we were saving the roots for each element. But as I wrote above, that would've been costly (to have the list of style root pointers for each element that has quotes or counters). To avoid that, we are calculating two numbers (resetFrom, containStyleLevel) on the fly, that will be enough to calculate that quote or counter node's value. This way, we are only adding an overhead of two unsigned integers to each gencon node. To do this edge-computing-kind-of-approach, we need two different (and very small) sets, one for quotes and one for counters. If we were to use one, quotes could affect the counters, and vice versa, and would result in unexpected behavior. > I assume they don't really need to be pointers, and they can just be > members, like mCounterManager is, right? Maybe getting generated content > split in three is not great, and we could maybe do something like adding a > "generated content manager" or something that itself contains the counter > manager? Anyway, not sure how the setup you're describing would look just > yet, but > anything that makes the code more straight-forward and easy to understand / > change is great IMO. I wouldn't be fearful of introducing new abstractions > if they make the code nicer :) Right, they can definitely be members. There is certainly a potential for further restructuring and maybe even few optimizations for handling generated content. Unfortunately, it seems like time will be a limitation for me to work on that a bit more. I think my plan for the remaining few days is, hopefully, to complete the functionality of counters. That way, we'd at least have a fully working copy, and can always work on making it better.
(In reply to Yusuf Sermet [:yusuf] from comment #47) > (In reply to Emilio Cobos Álvarez (:emilio) from comment #43) > > > I'm still unsure about why these two need to live in nsCSSFrameConstructor, > > shouldn't it leave in nsFrameConstructorState? > > You're right, having that two in the nsFrameConstructorState makes more > sense. I'll update it accordingly. > > > Let's say we load that page, and now I do something like moving DOM elements > > around so `a` is wrapped in a new contain style root. Wouldn't we, when we > > reconstruct frames for `gencon` again, fidnd that `c` is already in > > `mContainStyleRootsForCounters` and try to reuse the old thing? > > From what I understand, this shouldn't be a problem. Because we are not > actually saving the style roots for each element, which would be too costly > in terms of memory. For your example, we won't do anything for those > containing divs, but only gencon will trigger our mechanism for finding > contain:style roots. Thus, it'll find any dynamically added parents as well, > if I'm understanding this correctly. > > Just to demonstrate how we are using `mContainStyleRootsForCounters`: > > ``` > <style> > .gencon::before { content: counter(foo); } > </style> > <div style="contain: style" id="a"> > <div style="contain: style" id="b"> > <div style="contain: style" id="c"> > <div class="gencon"><div> > <div class="gencon"><div> > </div> > </div> > </div> > ``` > > First gencon is calculated normally, and all of the style roots are now > saved in `mContainStyleRootsForCounters`. Now, the next counter node (in > this case, the 2nd gencon), will start searching for style roots, and will > find that the innermost style root (id: c) is also in the > `mContainStyleRootsForCounters` from the previous counter node. So, it'll > stop searching more and will use the roots in > `mContainStyleRootsForCounters`. Sure, I get that in the case this is recreating the whole subtree is fine. My concern is that you can: * Update the layout of the page, then... * Shuffle nodes around (or remove them, or what not), then... * Update only a given subtree. In that case, the persisted state doesn't look trivially correct to me.
Attachment #8987764 - Attachment is obsolete: true
(In reply to Emilio Cobos Álvarez (:emilio) from comment #49) > * Update the layout of the page, then... > * Shuffle nodes around (or remove them, or what not), then... > * Update only a given subtree. > > In that case, the persisted state doesn't look trivially correct to me. It'll be best to try to identify the exact scenario, so that we can have a test case as well. I think one scenario you have in mind is: After the page loads, a dynamic change is made, and caused only a specific subtree to get updated. In this case, the first node inside this tree will find `mContainStyleRootsForCounters` either as empty or as the roots of the last ever node in the page [1]. So, there is no problem for nodes inside this tree. But, the next node after this tree (outside of it), will not be updated accordingly. I think this scenario won't happen ever, because if the value of the node changes, I think Firefox would calculate other nodes as well. Our contain implementation doesn't fundamentally change how Firefox handles the nodes in general. [1] Yes, this might be a problem. We need to make sure that these 2 sets are empty when starting to update a subtree.
(In reply to Emilio Cobos Álvarez (:emilio) from comment #44) > Anyway, not sure how the setup you're describing would look just yet, but > anything that makes the code more straight-forward and easy to understand / > change is great IMO. I wouldn't be fearful of introducing new abstractions > if they make the code nicer :) I just added a new class ContainStyleManager (thanks dholbert for the guideline on adding a new class) that will keep track of the style containment for quotes and counters. This way, we were able to remove almost all of the code from CSSFrameConstructor, it looks cleaner, and it's way more easy to understand and update. Btw, I'm officially using Phabricator from now on, just an fyi.
I'm happy to say we finally have a fully-working copy of style containment. I'm sure there'll be plenty of points&edge cases that'll need fixing&clarifying upon review, but hopefully its fundamental is in alright condition, so it'll be relatively easier to 'add new floors to the building'. For records, (skipping details) the problem in comment 39 is resolved mainly by giving a ContainStyleManager to each GenConList since there are multiple lists with different scopes for counters. For future, I think the only big thing left is the issue in comment 40. If it's decided to separate the ::before and ::after in terms of their effect to contain:style, then that'll need to be another bug following up this one, I think. emilio, I'm sorry about the last minute work :) I know I wasn't able to leave a good amount of time for the review process. But I'll be active on Bugzilla following up the subject, if there is anything I can do or clarify. Tomorrow, I'll file another bug for the tests cases.
Depends on: 1482482
Blocks: 1487493
Whiteboard: [layout:p2]

FF69 flags contain:style as an invalid value.

In FF68 it was ok.

In our particular case we were using "contain: paint style layout" and because it was marked invalid it fell back to another value that then caused the UI to render incorrectly.

(In reply to robertedgar from comment #53)

FF69 flags contain:style as an invalid value. In FF68 it was ok.

Firefox's CSS parser has rejected contain:style (and values including it) ever since Firefox 67, actually, via bug 1530896.

The change in Firefox 69 is that CSS Containment became enabled by default (bug 1487493). For now, you probably don't to use contain:style in your markup -- it really doesn't do much, if you're not using automatic CSS-based counter-numbering or quote-matching. See https://github.com/w3c/csswg-drafts/issues/3280 for more (note that contain:style was marked as "at-risk" in the spec as part of that github issue.)

You probably want just contain: paint layout or contain:strict (if you also want size-containment).

(In reply to robertedgar from comment #53)

FF69 flags contain:style as an invalid value.

In FF68 it was ok.

In our particular case we were using "contain: paint style layout" and because it was marked invalid it fell back to another value that then caused the UI to render incorrectly.

Hmm, but Firefox 68 didn't parse contain at all, right? Bug 1487493 landed in 70 and was uplifted to 69. As far as I know, to use it in FF68 you need to toggle the layout.css.contain.enabled pref yourself. Is that what you were doing?

See https://github.com/w3c/csswg-drafts/issues/3280 for why we removed support for contain: style (but afaict we never shipped it).

In any case seems like you want contain: content, right?

Sorry, the comment above mid-aired :)

Actually on reflection I think it was FF69 implementing size that might be the problem.

The element has initially has a class 'A' with contain:strict ( size layout paint), this is then overridden by another class "B" with contain:paint style layout.

In class A the size is independent of the children, but in class B it is dependent on the children (amongst other changes).

In FF68 everything displays correctly, in FF69 it fails as the height is 0.

So I assume FF69 implemented size whereas FF68 didnt.
This meant for us in FF68 contain in both classes didnt work whereas in FF69 one works the other doesn't.

FWIW in Chrome etc. there was no issue.

Sorry for misinformation before, took me a while to reflect on this and realise it was size being implemented that was the source of our problem

That makes more sense. Could you please file a new bug with a link to the affected page[1], and CC me on it? I'd very much like to take a look.

https://bugzilla.mozilla.org/enter_bug.cgi?product=Core&component=Layout

[1] (or a testcase, or some other way I can reproduce the issue & behavior-difference with e.g. Chrome)

(In reply to Daniel Holbert [:dholbert] from comment #58)

That makes more sense. Could you please file a new bug with a link to the affected page[1], and CC me on it? I'd very much like to take a look.

https://bugzilla.mozilla.org/enter_bug.cgi?product=Core&component=Layout

[1] (or a testcase, or some other way I can reproduce the issue & behavior-difference with e.g. Chrome)

Bug 1579336 - Page Layout issue in FF69 caused by implementing CSS 'contain: size' but not 'contain: style'

(In reply to robertedgar from comment #59)

(In reply to Daniel Holbert [:dholbert] from comment #58)

That makes more sense. Could you please file a new bug
Bug 1579336 - Page Layout issue in FF69 caused by implementing CSS 'contain: size' but not 'contain: style'

(Sorry for taking a while to take a look at that -- I ended up marking that bug as a duplicate of this one, with some notes/reasoning in bug 1579336 comment 6.)

The bug assignee didn't login in Bugzilla in the last 7 months.
:emilio, could you have a look please?
For more information, please visit auto_nag documentation.

Assignee: yusufsermet → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(emilio)
Flags: needinfo?(emilio)

I'm going to plan on picking this back up in the near future. --> self-assigning.

Assignee: nobody → dholbert
Severity: normal → S3
Blocks: container-queries

Add an implementation of CSS contain: style. This introduces two new
data structures, the ContainStyleScope and ContainStyleScopeManager.

ContainStyleScope manages one contain: style "world" which has its own
counter and quote lists. The contents of these lists depend on their
parent scopes, but are not affected by their children.
ContainStyleScopeManager manages a tree of scopes starting at a root
scope which is outside of any contain: style element.

Scopes are stored in a hash table that is keyed off of the nsIContent
which establishes the contain: style scope. When modifying quote or
content lists, the ContainStyleScopeManager is responsible for finding
the appropriate contain: style scope to modify.

Perhaps the most complex part of this is that counters and quotes have
read access to the state of counters and quotes that are in ancestor
contain: style scopes. In the case of counters, USE nodes that are at
the beginning of counter lists might have a counter scope that starts in
an ancestor contain: style scope. When nsCounterNode::SetScope() is
called, the code may look upward in the contain: style scope tree to
find the start of the counter scope. In the case of quotes, the first
node in the quote list must look for the state of quotes in ancestor
contain: style scopes.

Pushed by mrobinson@igalia.com: https://hg.mozilla.org/integration/autoland/rev/86e9449aba35 Implement CSS 'contain: style' r=emilio
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/34524 for changes under testing/web-platform/tests
Whiteboard: [layout:p2] → [layout:p2], [wptsync upstream]

Backed out for causing xpcshell failures on test_css-properties-db.js

Failure line: TEST-UNEXPECTED-FAIL | devtools/shared/tests/xpcshell/test_css-properties-db.js | xpcshell return code: 0

Push with failures

Failure log

Backout link

[task 2022-06-22T12:29:48.728Z] 12:29:48     INFO -  TEST-PASS | devtools/shared/tests/xpcshell/test_require.js | took 1849ms
[task 2022-06-22T12:29:48.828Z] 12:29:48     INFO -  TEST-PASS | devtools/shared/tests/xpcshell/test_console_filtering.js | took 3405ms
[task 2022-06-22T12:29:48.991Z] 12:29:48     INFO -  TEST-PASS | devtools/shared/tests/xpcshell/test_sprintfjs.js | took 1231ms
[task 2022-06-22T12:29:49.077Z] 12:29:49     INFO -  TEST-PASS | devtools/shared/tests/xpcshell/test_stack.js | took 948ms
[task 2022-06-22T12:29:49.089Z] 12:29:49     INFO -  TEST-PASS | devtools/shared/tests/xpcshell/test_executeSoon.js | took 762ms
[task 2022-06-22T12:29:49.162Z] 12:29:49     INFO -  TEST-PASS | devtools/shared/tests/xpcshell/test_defer.js | took 897ms
[task 2022-06-22T12:29:49.164Z] 12:29:49     INFO -  Retrying tests that failed when run in parallel.
[task 2022-06-22T12:29:49.168Z] 12:29:49     INFO -  TEST-START | devtools/shared/tests/xpcshell/test_css-properties-db.js
[task 2022-06-22T12:29:49.625Z] 12:29:49  WARNING -  TEST-UNEXPECTED-FAIL | devtools/shared/tests/xpcshell/test_css-properties-db.js | xpcshell return code: 0
[task 2022-06-22T12:29:49.626Z] 12:29:49     INFO -  TEST-INFO took 448ms
[task 2022-06-22T12:29:49.626Z] 12:29:49     INFO -  >>>>>>>
[task 2022-06-22T12:29:49.626Z] 12:29:49     INFO -  PID 4046 | [Parent 4046, Main Thread] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file /builds/worker/checkouts/gecko/toolkit/crashreporter/nsExceptionHandler.cpp:2972
[task 2022-06-22T12:29:49.626Z] 12:29:49     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2022-06-22T12:29:49.626Z] 12:29:49     INFO -  TEST-PASS | devtools/shared/tests/xpcshell/test_css-properties-db.js | run_test - [run_test : 42] The pseudo elements match on the client and platform. If this assertion fails, then the client side CSS properties list in devtools is out of sync with the CSS properties on the platform. To fix this assertion run `mach devtools-css-db` to re-generate the client side properties. - ["::after","::before","::marker","::backdrop","::cue","::first-letter","::first-line","::selection","::-moz-focus-inner","::-moz-progress-bar","::-moz-range-track","::-moz-range-progress","::-moz-range-thumb","::-moz-meter-bar","::placeholder","::-moz-color-swatch","::file-selector-button"] deepEqual ["::after","::before","::marker","::backdrop","::cue","::first-letter","::first-line","::selection","::-moz-focus-inner","::-moz-progress-bar","::-moz-range-track","::-moz-range-progress","::-moz-range-thumb","::-moz-meter-bar","::placeholder","::-moz-color-swatch","::file-selector-button"]
[task 2022-06-22T12:29:49.628Z] 12:29:49     INFO -  TEST-PASS | devtools/shared/tests/xpcshell/test_css-properties-db.js | run_test - [run_test : 50] The preferences match on the client and platform. If this assertion fails, then the client side CSS properties list in devtools is out of sync with the CSS properties on the platform. To fix this assertion run `mach devtools-css-db` to re-generate the client side properties. - [["aspect-ratio","layout.css.aspect-ratio.enabled"],["container-type","layout.css.container-queries.enabled"],["content-visibility","layout.css.content-visibility.enabled"],["font-optical-sizing","layout.css.font-variations.enabled"],["initial-letter","layout.css.initial-letter.enabled"],["masonry-auto-flow","layout.css.grid-template-masonry-value.enabled"],["math-depth","layout.css.math-depth.enabled"],["math-style","layout.css.math-style.enabled"],["-moz-control-character-visibility","layout.css.moz-control-character-visibility.enabled"],["-moz-osx-font-smoothing","layout.css.osx-font-smoothing.enabled"],["offset-rotate","layout.css.motion-path.enabled"],["overflow-anchor","layout.css.scroll-anchoring.enabled"],["scrollbar-gutter","layout.css.scrollbar-gutter.enabled"],["-webkit-line-clamp","layout.css.webkit-line-clamp.enabled"],["overflow-clip-box-block","layout.css.overflow-clip-box.enabled"],["overflow-clip-box-inline","layout.css.overflow-clip-box.enabled"],["overflow-block","layout.css.overflow-logical.enabled"],["overflow-inline","layout.css.overflow-logical.enabled"],["overscroll-behavior-block","layout.css.overscroll-behavior.enabled"],["overscroll-behavior-inline","layout.css.overscroll-behavior.enabled"],["overscroll-behavior-x","layout.css.overscroll-behavior.enabled"],["overscroll-behavior-y","layout.css.overscroll-behavior.enabled"],["accent-color","layout.css.accent-color.enabled"],["align-tracks","layout.css.grid-template-masonry-value.enabled"],["animation-timeline","layout.css.scroll-linked-animations.enabled"],["backdrop-filter","layout.css.backdrop-filter.enabled"],["color-scheme","layout.css.color-scheme.enabled"],["container-name","layout.css.container-queries.enabled"],["d","layout.css.d-property.enabled"],["font-variation-settings","layout.css.font-variations.enabled"],["hyphenate-character","layout.css.hyphenate-character.enabled"],["justify-tracks","layout.css.grid-template-masonry-value.enabled"],["-moz-context-properties","svg.context-properties.content.enabled"],["offset-anchor","layout.css.motion-path.enabled"],["offset-path","layout.css.motion-path.enabled"],["page","layout.css.named-pages.enabled"],["rotate","layout.css.individual-transform.enabled"],["scale","layout.css.individual-transform.enabled"],["scroll-timeline-axis","layout.css.scroll-linked-animations.enabled"],["scroll-timeline-name","layout.css.scroll-linked-animations.enabled"],["size","layout.css.page-size.enabled"],["translate","layout.css.individual-transform.enabled"],["offset-distance","layout.css.motion-path.enabled"],["overflow-clip-box","layout.css.overflow-clip-box.enabled"],["overscroll-behavior","layout.css.overscroll-behavior.enabled"],["container","layout.css.container-queries.enabled"],["offset","layout.css.motion-path.enabled"],["zoom","layout.css.zoom-transform-hack.enabled"],["scroll-timeline","layout.css.scroll-linked-animations.enabled"],["-moz-transform","layout.css.prefixes.transforms"],["-moz-perspective","layout.css.prefixes.transforms"],["-moz-perspective-origin","layout.css.prefixes.transforms"],["-moz-backface-visibility","layout.css.prefixes.transforms"],["-moz-transform-style","layout.css.prefixes.transforms"],["-moz-transform-origin","layout.css.prefixes.transforms"],["-moz-font-feature-settings","layout.css.prefixes.font-features"],["-moz-font-language-override","layout.css.prefixes.font-features"],["-moz-box-sizing","layout.css.prefixes.box-sizing"],["-moz-transition-duration","layout.css.prefixes.transitions"],["-moz-transition-timing-function","layout.css.prefixes.transitions"],["-moz-transition-property","layout.css.prefixes.transitions"],["-moz-transition-delay","layout.css.prefixes.transitions"],["-moz-animation-name","layout.css.prefixes.animations"],["-moz-animation-duration","layout.css.prefixes.animations"],["-moz-animation-timing-function","layout.css.prefixes.animations"],["-moz-animation-iteration-count","layout.css.prefixes.animations"],["-moz-animation-direction","layout.css.prefixes.animations"],["-moz-animation-play-state","layout.css.prefixes.animations"],["-moz-animation-fill-mode","layout.css.prefixes.animations"],["-moz-animation-delay","layout.css.prefixes.animations"],["-moz-border-image","layout.css.prefixes.border-image"],["-moz-transition","layout.css.prefixes.transitions"],["-moz-animation","layout.css.prefixes.animations"]] deepEqual [["aspect-ratio","layout.css.aspect-ratio.enabled"],["container-type","layout.css.container-queries.enabled"],["content-visibility","layout.css.content-visibility.enabled"],["font-optical-sizing","layout.css.font-variations.enabled"],["initial-letter","layout.css.initial-letter.enabled"],["masonry-auto-flow","layout.css.grid-template-masonry-value.enabled"],["math-depth","layout.css.math-depth.enabled"],["math-style","layout.css.math-style.enabled"],["-moz-control-character-visibility","layout.css.moz-control-character-visibility.enabled"],["-moz-osx-font-smoothing","layout.css.osx-font-smoothing.enabled"],["offset-rotate","layout.css.motion-path.enabled"],["overflow-anchor","layout.css.scroll-anchoring.enabled"],["scrollbar-gutter","layout.css.scrollbar-gutter.enabled"],["-webkit-line-clamp","layout.css.webkit-line-clamp.enabled"],["overflow-clip-box-block","layout.css.overflow-clip-box.enabled"],["overflow-clip-box-inline","layout.css.overflow-clip-box.enabled"],["overflow-block","layout.css.overflow-logical.enabled"],["overflow-inline","layout.css.overflow-logical.enabled"],["overscroll-behavior-block","layout.css.overscroll-behavior.enabled"],["overscroll-behavior-inline","layout.css.overscroll-behavior.enabled"],["overscroll-behavior-x","layout.css.overscroll-behavior.enabled"],["overscroll-behavior-y","layout.css.overscroll-behavior.enabled"],["accent-color","layout.css.accent-color.enabled"],["align-tracks","layout.css.grid-template-masonry-value.enabled"],["animation-timeline","layout.css.scroll-linked-animations.enabled"],["backdrop-filter","layout.css.backdrop-filter.enabled"],["color-scheme","layout.css.color-scheme.enabled"],["container-name","layout.css.container-queries.enabled"],["d","layout.css.d-property.enabled"],["font-variation-settings","layout.css.font-variations.enabled"],["hyphenate-character","layout.css.hyphenate-character.enabled"],["justify-tracks","layout.css.grid-template-masonry-value.enabled"],["-moz-context-properties","svg.context-properties.content.enabled"],["offset-anchor","layout.css.motion-path.enabled"],["offset-path","layout.css.motion-path.enabled"],["page","layout.css.named-pages.enabled"],["rotate","layout.css.individual-transform.enabled"],["scale","layout.css.individual-transform.enabled"],["scroll-timeline-axis","layout.css.scroll-linked-animations.enabled"],["scroll-timeline-name","layout.css.scroll-linked-animations.enabled"],["size","layout.css.page-size.enabled"],["translate","layout.css.individual-transform.enabled"],["offset-distance","layout.css.motion-path.enabled"],["overflow-clip-box","layout.css.overflow-clip-box.enabled"],["overscroll-behavior","layout.css.overscroll-behavior.enabled"],["container","layout.css.container-queries.enabled"],["offset","layout.css.motion-path.enabled"],["zoom","layout.css.zoom-transform-hack.enabled"],["scroll-timeline","layout.css.scroll-linked-animations.enabled"],["-moz-transform","layout.css.prefixes.transforms"],["-moz-perspective","layout.css.prefixes.transforms"],["-moz-perspective-origin","layout.css.prefixes.transforms"],["-moz-backface-visibility","layout.css.prefixes.transforms"],["-moz-transform-style","layout.css.prefixes.transforms"],["-moz-transform-origin","layout.css.prefixes.transforms"],["-moz-font-feature-settings","layout.css.prefixes.font-features"],["-moz-font-language-override","layout.css.prefixes.font-features"],["-moz-box-sizing","layout.css.prefixes.box-sizing"],["-moz-transition-duration","layout.css.prefixes.transitions"],["-moz-transition-timing-function","layout.css.prefixes.transitions"],["-moz-transition-property","layout.css.prefixes.transitions"],["-moz-transition-delay","layout.css.prefixes.transitions"],["-moz-animation-name","layout.css.prefixes.animations"],["-moz-animation-duration","layout.css.prefixes.animations"],["-moz-animation-timing-function","layout.css.prefixes.animations"],["-moz-animation-iteration-count","layout.css.prefixes.animations"],["-moz-animation-direction","layout.css.prefixes.animations"],["-moz-animation-play-state","layout.css.prefixes.animations"],["-moz-animation-fill-mode","layout.css.prefixes.animations"],["-moz-animation-delay","layout.css.prefixes.animations"],["-moz-border-image","layout.css.prefixes.border-image"],["-moz-transition","layout.css.prefixes.transitions"],["-moz-animation","layout.css.prefixes.animations"]]
[task 2022-06-22T12:29:49.629Z] 12:29:49     INFO -  TEST-PASS | devtools/shared/tests/xpcshell/test_css-properties-db.js | run_test - [run_test : 71] The static database and platform match for "-moz-animation". - true == true
[task 2022-06-22T12:29:49.630Z] 12:29:49     INFO -  TEST-PASS | devtools/shared/tests/xpcshell/test_css-properties-db.js | run_test - [run_test : 71] The static database and platform match for "-moz-animation-delay". - true == true
[task 2022-06-22T12:29:49.631Z] 12:29:49     INFO -  TEST-PASS | devtools/shared/tests/xpcshell/test_css-properties-db.js | run_test - [run_test : 71] The static database and platform match for "-moz-animation-direction". - true == true
[task 2022-06-22T12:29:49.631Z] 12:29:49     INFO -  TEST-PASS | devtools/shared/tests/xpcshell/test_css-properties-db.js | run_test - [run_test : 71] The static database and platform match for "-moz-animation-duration". - true == true
[task 2022-06-22T12:29:49.632Z] 12:29:49     INFO -  TEST-PASS | devtools/shared/tests/xpcshell/test_css-properties-db.js | run_test - [run_test : 71] The static database and platform match for "-moz-animation-fill-mode". - true == true
[task 2022-06-22T12:29:49.632Z] 12:29:49     INFO -  TEST-PASS | devtools/shared/tests/xpcshell/test_css-properties-db.js | run_test - [run_test : 71] The static database and platform match for "-moz-animation-iteration-count". - true == true
Flags: needinfo?(dholbert)

Martin, I think you just need to run ./mach devtools-css-db, add the changes and re-land (bug 1320607 tracks getting rid of this, bites us way too often :/).

Assignee: dholbert → mrobinson
Flags: needinfo?(dholbert) → needinfo?(mrobinson)
Upstream PR was closed without merging
Pushed by mrobinson@igalia.com: https://hg.mozilla.org/integration/autoland/rev/e25543fae7ee Implement CSS 'contain: style' r=emilio

(In reply to Emilio Cobos Álvarez (:emilio) from comment #69)

Martin, I think you just need to run ./mach devtools-css-db, add the changes and re-land (bug 1320607 tracks getting rid of this, bites us way too often :/).

Oof. Thanks. I've run this and landed the change again.

Flags: needinfo?(mrobinson)

https://hg.mozilla.org/mozilla-central/rev/e25543fae7ee

Status: NEW → RESOLVED
Closed: 2 years ago
status-firefox103: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 103 Branch
Upstream PR merged by moz-wptsync-bot
Regressions: 1776211
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 1809505
Blocks: 1744309
No longer duplicate of this bug: 1744309
Attachment #8998442 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: