See http://dev.w3.org/csswg/css-containment/#containment-layout for details about this portion of the css 'contain' property.

pretty sure Kyle meant to link to: http://dev.w3.org/csswg/css-containment/#containment-style

This implements 'contain: style' on counters using counter scoping.

This implements 'contain: style' on counters using counter scoping.

This tests 'contain: style' on counters using counter scoping.

This implements 'contain: style' on quotes by adding scoping to them.

This tests the behavior of quotes with 'contain: style'.

Implement page breaking behavior for 'contain: style'. I'm not 100% sure this behavior is correct.

This tests the behavior of the ContainStyleBreak tests.

These patches should be applied in the order uploaded here. The commit messages also record the order. Most likely more tests are needed, so this should be considered a WIP.

Comment on attachment 8627975 [details] [diff] [review] ContainStyleCounters Review of attachment 8627975 [details] [diff] [review]: ----------------------------------------------------------------- Ultimately, these should get review from a style-system peer (dbaron or bz or heycam). I skimmed part 1, though, and here are some drive-by mostly-stylistic nits that I ran across: ::: layout/base/nsCSSFrameConstructor.cpp @@ +10338,5 @@ > > if (!aPossiblyLeafFrame) { > aPossiblyLeafFrame = aFrame; > } > + nsCounterScopePush saveCounters(&mCounterManager, aFrame, (nit: drop trailing whitespace on this line) ::: layout/base/nsCounterManager.cpp @@ +168,5 @@ > "null check nodeContent as well, since if nodeContent " > "is for the root, startContent (which is before it) " > "must be too"); > > + Nit: revert this new blank line, inserted in contextual code. @@ +278,5 @@ > return false; > } > > +struct AddRootArgs { > + nsIFrame *mFrame; Nit (for here and the chunk below this, and probably other places): Gecko coding style is to snuggle pointer stars with the type, not the variable name: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Declarations (since the "*" is part of the type) Old code isn't 100% consistent about this, but new code should match this style, generally. ::: layout/base/nsCounterManager.h @@ +302,5 @@ > +class nsCounterScopePush > +{ > +public: > + explicit nsCounterScopePush(nsCounterManager *aManager, nsIFrame *aFrame, > + bool use) nit: (1) no need for "explicit" on this constructor (2) "bool use" should be indented more, to be inside the open-paren. (3) (and pointers should be type-snuggled here as well) ::: layout/style/nsStyleStruct.h @@ +2249,5 @@ > mOverflowX != NS_STYLE_OVERFLOW_CLIP; > } > > + bool IsStyleContaining() const { > + return NS_STYLE_CONTAIN_STYLE & mContain; Nit: the Paint verison of this function (below this one) has these operands in the opposite order: return mContain & NS_STYLE_CONTAIN_PAINT; Probably best to make the ordering consistent. (And per end of bug 1170781 comment 6, I think we should call these e.g. "IsContainStyle". "Is${FOO}Containing" sounds slightly more abstract/complex - makes me think of e.g. IsFixedPosContainingBlock() -- when really this is just a check for a single property value.)

I forgot to re-add the style-break-001.html test to reftest.list. It's there now.

I've made all the relatively simple changes requested or implied by dholbert's comments. dbaron, if you're not the most appropriate person to be reviewing some of these changes, please let me know or update the review flag. Thanks!

Comment on attachment 8628952 [details] [diff] [review] ContainStyleCounters nsCounterScopePush should use aUse rather than use as the name of the third constructor argument. > : nsCounterNode(// Fake a content index for resets and increments > // that comes before all the real content, with > // the resets first, in order, and then the increments. >- aPropIndex + (aChangeType == RESET >- ? (INT32_MIN) >- : (INT32_MIN / 2)), >+ aPropIndex + >+ (aChangeType == RESET ? (INT32_MIN) : >+ aChangeType == INCREMENT ? (2 * (INT32_MIN / 3)) >+ /* aChangeType == ROOT */ : (INT32_MIN / 3)), A few things here: (1) I was expecting ROOT to come first, then RESET, then INCREMENT, since, if I'm understanding the spec correctly, on an element with contain: style, counter-increment and counter-reset will apply inside the element but not outside. (I'm not convinced this is the right way around, but I don't think the decision matters a whole lot.) (2) Likewise, I'd have expected ROOT to be first in the Type enum, since I expect the type enum to be ordered by how we expect nodes on the same element to sort. I'm still trying to understand how you handle the end of something being a root, though. For example, if you have: <style> body { counter-reset: list } li { display: block; counter-increment: list } li::before { content: counter(list) } </style> I would expect: <li>one</li> <li style="contain: style">zero</li> <li>two</li> and it's not clear to me how you get the "two" item to have the right number. Worse, it's not clear to me how you end up acting as though there's no scope and thus, with the same styles, I would expect: <div style="contain: strict"> <div> <li>one</li> </div> <li>one</li> </div> (i.e., what you'd get without the contain:strict or contain:style if there were no counter-reset on the body). Also, it's not clear to me how you handle (given the above styles): <div style="contain: style; counter-reset: list 3"> <li>four</li> </div> if the ROOT node comes after the INCREMENT and USE nodes. Finally, what happens if you need to create a ROOT for a counter that doesn't exist yet -- for example, because the only current uses and increments are in descendants or later siblings, but for which a reset on a previous sibling is later added? It seems like you end up without the root because the counter wasn't in mNames beforehand. That makes me skeptical of the approach of adding ROOT nodes, actually. (It would also be nice if the mechanism had the performance characteristics you'd expect of a mechanism that was doing isolation.) That said, it's been slightly over a decade since I looked closely at this code (bug 3247), so my memory is a bit rusty, and I admit I didn't try to understand the SetScope changes, which could be hiding a bunch of magic.

Comment on attachment 8628958 [details] [diff] [review] ContainStyleBreak This isn't going to work if we dynamically construct frames for a subtree inside of the content:style element, e.g., in response to a style change or content tree change that's rooted inside the contain:style element. You could solve that by walking up the tree at the callers of the constructor of nsFrameConstructorState (like we do for fixed and abs-pos containing blocks) or inside the constructor, I guess (perhaps passing the parent frame), which might be cleaner. It feels pretty odd to introduce something inefficient to support what's supposed to be a performance mechanism, though.

(In reply to David Baron [:dbaron] ⏰UTC+2 (busy until July 20) from comment #19) > Comment on attachment 8628952 [details] [diff] [review] > ContainStyleCounters > > nsCounterScopePush should use aUse rather than use as the name of > the third constructor argument. > > > > : nsCounterNode(// Fake a content index for resets and increments > > // that comes before all the real content, with > > // the resets first, in order, and then the increments. > >- aPropIndex + (aChangeType == RESET > >- ? (INT32_MIN) > >- : (INT32_MIN / 2)), > >+ aPropIndex + > >+ (aChangeType == RESET ? (INT32_MIN) : > >+ aChangeType == INCREMENT ? (2 * (INT32_MIN / 3)) > >+ /* aChangeType == ROOT */ : (INT32_MIN / 3)), > > A few things here: > > (1) I was expecting ROOT to come first, then RESET, then INCREMENT, > since, if I'm understanding the spec correctly, on an element with > contain: style, counter-increment and counter-reset will apply inside > the element but not outside. (I'm not convinced this is the right way > around, but I don't think the decision matters a whole lot.) > > (2) Likewise, I'd have expected ROOT to be first in the Type enum, > since I expect the type enum to be ordered by how we expect nodes on > the same element to sort. Yeah, those should definitely be reordered. Re-reading the spec, it's clear that uses on the containing element should be themselves contained (i.e. the ROOT comes first). Relevant part of the spec: > "...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." > > > I'm still trying to understand how you handle the end of something > being a root, though. For example, if you have: > > <style> > body { counter-reset: list } > li { display: block; counter-increment: list } > li::before { content: counter(list) } > </style> > > I would expect: > > <li>one</li> > <li style="contain: style">zero</li> > <li>two</li> > > and it's not clear to me how you get the "two" item to have the right > number. > > > Worse, it's not clear to me how you end up acting as though there's > no scope and thus, with the same styles, I would expect: > > <div style="contain: strict"> > <div> > <li>one</li> > </div> > <li>one</li> > </div> > > (i.e., what you'd get without the contain:strict or contain:style > if there were no counter-reset on the body). > > > Also, it's not clear to me how you handle (given the above styles): > > <div style="contain: style; counter-reset: list 3"> > <li>four</li> > </div> > > if the ROOT node comes after the INCREMENT and USE nodes. > Yes, these cases all end up doing approximately what you described here right now (incorrect behavior). They appear to work with the order above changed, but I'm going to do more testing to be sure (and write up these cases into reftests). > > Finally, what happens if you need to create a ROOT for a counter that > doesn't exist yet -- for example, because the only current uses and > increments are in descendants or later siblings, but for which a reset > on a previous sibling is later added? It seems like you end up without > the root because the counter wasn't in mNames beforehand. I completely missed this case. I have a plan to solve it by maintaining a list for a "prototype counter", which will be copied whenever a new counter is first used. > > That makes me skeptical of the approach of adding ROOT nodes, actually. > (It would also be nice if the mechanism had the performance > characteristics you'd expect of a mechanism that was doing isolation.) > I'm also not satisfied that this patch has negative performance when counters and contain are used together. However, in order for style recalculations to be done correctly when content is inserted into containing elements, some record of the ROOT locations needs to be made. The alternative would be scanning the frame tree, which I *think* would have much worse performance consequences. It would be nice to, and should be possible to, move all of the ROOT nodes to a different data structure, but I haven't thought of an efficient way to tie that into the existing scoping algorithm without significantly complicating the memory management. > > > That said, it's been slightly over a decade since I looked closely > at this code (bug 3247), so my memory is a bit rusty, and I admit > I didn't try to understand the SetScope changes, which could be > hiding a bunch of magic. You are correct that SetScope is doing a bit of magic here, in that it's responsible for determining when scopes end. I'll try to make the logic there clearer. Thanks for the feedback!

The main difference for this patch is the order fix, and the SetScope logic has been expanded. Hopefully it is clearer now.

I've added all the cases you mentioned above as reftests. There's also a few cases to check that dynamically inserted counters are scoped correctly.

Aside from a fixing the deletion of scope nodes from the quote list, this patch isn't any different from the previous version.

This patch was only updated so that it would apply cleanly to the previous patches.

This patch completely changes how 'contain: style' prevents break before from working. I think this is what you had in mind? It does work, but it's probably pretty slow (during paginated reflow only). If there's another way you'd like me to try to implement this, please let me know.

This version of the patch adds a test which dynamically inserts 'page-break-before: always' elements.

These patches are being tested at: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a55d63ed0c9a A slightly older version of these patches was tested at: https://treeherder.mozilla.org/#/jobs?repo=try&revision=521f2f237471

The previous version of this patch contained an html file I shouldn't have added.

Comment on attachment 8634352 [details] [diff] [review] ContainStyleCounters I should have decided this earlier, but: I'm simply not comfortable with this approach. It's adding complexity to the code, and it's slowing things down for a feature that's meant to speed things up. I think you should try writing a different way, as follows: (1) store a bit on the style context that says whether the element or one of its ancestors has contain:style. (It might need to go in mNoneBits rather than mDependentBits like the current bits; I think heycam had a patch that started using mNoneBits for storage recently, but I'm not sure if it landed yet.) (2) make all elements inside a contain:style simply use a different counter manager or different quotes list (or a different substructure of the counter manager or quotes manager), *lazily created*, that's associated with the contain:style element. (3) actually make the optimization of making QuotesDirty()/CountersDirty() mark only the relevant counter manager or quotes list as dirty. (The counter manager's current RecalcAll should probably have been called RecalcDirty, since it does track dirty state on counter lists.) And make finding this relatively cheap by at least only walking up the tree when the bit from (1) is set. (4) change the break-* test to just check the bit on the style context rather than walk up the tree

Comment on attachment 8634361 [details] [diff] [review] ContainStyleBreak In addition to (4) above, this isn't the only place we look at mBreakBefore/mBreakAfter. At least nsFlexContainerFrame also does; I didn't look further.

I've begun addressing the feedback above. This patch contains the work common to all of the style containment. The method differs slightly from your suggestion, so I'd appreciate your opinion on it.

I added a commit message.

This patch implements break-* related functionality.

This patch implements counter-* related functionality. Like the previous patch, it passes all the tests I've written here. I'm not convinced the performance will be much better than the previous version.

(In reply to Kyle Zentner from comment #32) > Created attachment 8642735 [details] [diff] [review] > ContainStyleBase > > I've begun addressing the feedback above. This patch contains the work > common to all of the style containment. The method differs slightly from > your suggestion, so I'd appreciate your opinion on it. If you mean that I was confusing the places for bits on the style context with the places for bits on the rule node, that part looks fine.

This version of the ContainStyleCountersV2 patch passes all tests, and I think has a reasonable chance of actually being correct. I'm still not sure if there are major changes I should make though.

This patch implements style containment for quotes in essentially the same way as counters.

This version of the ContainStyleCountersV2 patch removes the mStyleRoots array, and instead uses an AutoRestore like class to set / reset the style root.

This is a simple update to the ContainStyleQuotesV2 patch so that it cleanly applies over the ContainStyleCountersV2 patch.

These patches are being tested at https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc5a320496da along with my layout containment patches.

This version of the patch is under test above. It adds a little minor cleanup, and actually frees nodes correctly when a style root is destroyed.

This version of the contain style quotes patch has similar fixes to the counter containment patch. It is also under test above.

Comment on attachment 8642738 [details] [diff] [review] ContainStyleBreakV2 This looks fine in general, but you need the same thing for all the mBreakAfter checks (which are in the same files, except 2 in nsCSSFrameConstructor instead of one).

Comment on attachment 8644564 [details] [diff] [review] ContainStyleCountersV2 >+class AutoRestoreStyleRoot { Please use MOZ_STACK_CLASS, MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER, MOZ_GUARD_OBJECT_NOTIFIER_PARAM, and MOZ_GUARD_OBJECT_NOTIFIER_INIT. See MXR for other examples, e.g., https://mxr.mozilla.org/mozilla-central/source/xpcom/glue/AutoRestore.h I think mStyleRoot should be called mContainStyleRoot, since that's clearer out-of-context. (Probably the same for AutoRestoreStyleRoot, GetStyleRoot, FindStyleRoot.) I think GetStyleRoot should take the frame you're finding the root for as an argument, and should have an assertion (probably NS_ASSERTION) that FindStyleRoot matches mStyleRoot. Otherwise it's not clear that you're managing the style root correctly. (I was thinking about suggesting that you move the variable from the frame constructor to the frame constructor state, which might help manage it clearly, although it's probably ok as is.) >+ // XXX kzentner: Is the style root always correct here? >+ mCounterManager.RecalcDirty(GetStyleRoot()); This seems like it's probably wrong. I think you want to remove the parameter to RecalcDirty and just have it always take the codepath it takes when you give it a null param (which is currently unused). >+ static >+ nsIFrame* FindStyleRoot(nsIFrame* aDescendant) { >+ nsIFrame* ancestor = aDescendant; >+ while (ancestor && !ancestor->StyleDisplay()->IsContainStyle()) { >+ ancestor = ancestor->GetParent(); >+ } >+ return ancestor; >+ } This should have a quick test for HasContainStyle(), and return the root frame as a fast path if not. nsCounterManager.h: I think the aRoot arguments in nsCounterManager should be aContainStyleRoot; otherwise it doesn't make sense out-of-context. Same for mRoots -> mContainStyleRoots. nsCounterManager.cpp: >- (!startContent || >+ ((!startContent || This extra ( just seems unnecessary. If it weren't, you'd need to fix the indentation, though. >+ // We hit the beginning of the list, but not necessarily the top >+ // of the frame tree. If we're not starting a new scope ourselves, >+ // we should use this as the start of scope. This case should only >+ // be hit from using style containment. >+ (start == First() && (aNode->mType != nsCounterNode::RESET)))) { I don't understand this bit. Could you explain? For RecalcDirty, see above comments on nsCSSFrameConstructor. I think you should also rewrite RecalcDirty with use of ConstIter() rather than using EnumerateRead, which we're trying to remove. See, say, https://hg.mozilla.org/mozilla-central/rev/43ae47ffe011 for examples. (You don't have to do it for the existing enumeration -- but you should for the new one. Probably better to deal with the existing one in a separate patch, which I'd welcome, but which you don't have to write here.) Also, opening { of a function goes on its own line (but you can probably get rid of the new functions). Same ConstIter() for SetAllCounterStylesDirty (at least for the new parts), and for Dump. Otherwise I think this is fine (modulo the one bit I didn't understand above).

Comment on attachment 8644567 [details] [diff] [review] ContainStyleQuotesV2 > nsCSSFrameConstructor::RecalcQuotesAndCounters() > { >+ // XXX kzentner: Is the style root always correct here? >+ nsIFrame* styleRoot = GetStyleRoot(); > if (mQuotesDirty) { > mQuotesDirty = false; >- mQuoteList.RecalcAll(); >+ mQuotesManager.RecalcDirty(styleRoot); > } So here I think you need to add a mechanism that tracks which list is dirty, at the points that set mQuotesDirty to true. Then you should again, not pass the style root to RecalcDirty, but instead recalc the lists that are dirty. It would also be good to add some tests that fail with the current state (e.g., dynamic changes to counters and quotes that are inside of contain:style). (RecalcQuotesAndCounters is called from outside the frame constructor, or from EndUpdate which is in turn called from outside; there probably isn't any other frame constructor stuff on the stack when it's called.) >+static PLDHashOperator >+ClearList(const nsIFrame* aKey, nsQuoteList* aList, void* aClosure) { >+ aList->Clear(); >+ return PL_DHASH_NEXT; >+} >+ >+void >+nsQuotesManager::Clear() { >+ mLists.EnumerateRead(ClearList, nullptr); >+} Same comment about ConstIter(). >+class nsQuotesManager { >+public: >+ >+ bool DestroyNodesFor(nsIFrame* aRoot, nsIFrame* aFrame) { And, again, probably aContainStyleRoot rather than just aRoot. >+ nsQuoteList* GetList(nsIFrame* aRoot, bool aShouldCreate = true) { >+ nsQuoteList* list = mLists.Get(aRoot); >+ if (!list && aShouldCreate) { >+ list = new nsQuoteList(); >+ mLists.Put(aRoot, list); >+ } >+ return list; >+ } use 2-space indent >+ void RecalcDirty(nsIFrame* aRoot) { >+ nsQuoteList* list = GetList(aRoot, false); >+ if (list) { >+ list->RecalcAll(); >+ } >+ } per above, this should enumerate all lists and look at their dirty bits otherwise I think this is good

This version of the contain style quotes patch adds a test which dynamically inserts quote usage inside of a style containing element.

This version of the contain style break patch should cover every use of mBreakBefore and mBreakAfter.

I think that this version of the contain style counters patch addresses all of the feedback above.

I think that this version of the contain style quotes patch addresses all of the feedback above.

Comment on attachment 8645198 [details] [diff] [review] ContainStyleBreakV2 In nsTableFrame, please break the long line after the && and line up correctly. In nsTableRowFrame, please break both long lines after the && and line up correctly. r=dbaron with that

Comment on attachment 8645200 [details] [diff] [review] ContainStyleCountersV2 >+ AutoRestoreContainStyleRoot(nsIFrame*& aLocation, nsIFrame* aPotentialRoot MOZ_GUARD_OBJECT_NOTIFIER_PARAM) Please put MOZ_GUARD_OBJECT_NOTIFIER_PARAM on a second line, lined up with nsIFrame. In FindContainStyleRoot: >+ if (!ancestor || !ancestor->StyleContext()->HasContainStyle()) { >+ // We're either the root, or we're not contain style. >+ return nullptr; >+ } This (to address my comment in comment 50) isn't right in three ways: * first, no need to null-check ancestor; it's the same as aDescendant which should be guaranteed non-null * second, null is not equivalent to being the root, so the comment doesn't make sense (although fixing my previous point should also fix that) (Null means root in SetScope because in SetScope the element pointer is actually the element's parent.) * third, you should return the root frame here, via: 1. making the method non-static 2. mPresContext->PresShell()->GetRootFrame() Missed this from comment 50: >nsCounterManager.h: > >I think the aRoot arguments in nsCounterManager should be >aContainStyleRoot; otherwise it doesn't make sense out-of-context. >Same for mRoots -> mContainStyleRoots. >- startContent))) { >+ startContent) || >+ // Test if we hit the beginning of the list. This implies that >+ // there are no acceptable start of scopes for this counter inside >+ // the current 'contain: style' element (or the whole document, if >+ // there is no such element). >+ // Therefore, we should treat this null start of scope as an >+ // "implicit reset", which will start our scope unless we are the >+ // start of our own scope. >+ // If there is no such 'contain: style' element, and we should be >+ // scoped to the entire document, then the !startContent test >+ // above would have succeeded, so this condition should only >+ // succeed from using style containment. >+ (start == First() && aNode->mType != nsCounterNode::RESET))) { I still don't understand why this change is needed. With the new approach, doesn't contain:style mean that elements with different contain:style roots have different lists? It seems like this will just make things incorrect if you have a contain:style scope that gets its counters started by an implicit reset, and then later has a counter that should not be inside that implicit reset. I think this change can just be dropped. In RecalcDirty and SetAllCounterStylesDirty.: >+ for (auto iter = mContainStyleRoots.ConstIter(); !iter.Done(); iter.Next()) { Please wrap after the final ;. There are a few other 80th column violations in the patch as well. r=dbaron with that fixed (or we can discuss the SetScope issue above if needed)

Oh, and it seems like tests ought to catch these two issues: (In reply to David Baron [:dbaron] (busy Aug. 8-Aug. 30) from comment #57) > * third, you should return the root frame here, via: > 1. making the method non-static > 2. mPresContext->PresShell()->GetRootFrame() > > It seems like this will just make things incorrect if you have a > contain:style scope that gets its counters started by an implicit reset, > and then later has a counter that should not be inside that implicit > reset.

Comment on attachment 8645201 [details] [diff] [review] ContainStyleQuotesV2 In NotifyDestroyingFrame: >- if (mQuoteList.DestroyNodesFor(aFrame)) >+ if (mQuotesManager.DestroyNodesFor(styleRoot, aFrame)) > QuotesDirty(); >+ mQuotesManager.MarkContainStyleRootDirty(styleRoot); > } This needs braces around the 2-line if-body so that both lines are inside the if. > nsGenConInitializer* initializer = >- new nsGenConInitializer(node, &mQuoteList, >+ new nsGenConInitializer(node, >+ mQuotesManager.GetList(GetContainStyleRoot(aParentFrame)), > &nsCSSFrameConstructor::QuotesDirty); >+ mQuotesManager.MarkContainStyleRootDirty(GetContainStyleRoot(aParentFrame)); Please put the result of GetContainStyleRoot in a variable instead of calling it twice. Please make mDirty a member variable of nsQuoteList (just like it is for nsCounterList) instead of creating the RootTableEntry struct. Remember to initialize it (probably to false) in the constructor. Along with that, please remove the MarkContainStyleRootDirty call from nsCSSFrameConstructor::CreateGeneratedContentFrame to a SetDirty call on the list in nsQuoteNode::InitTextFrame. Please at least file a followup bug on moving mDirty into nsGenConList, which will also allow setting it to move into nsGenConList::DestroyNodesFor from its callers (and removing the exposed MarkContainStyleRootDirty method entirely). Even better, that could be a followup patch. r=dbaron with that

Comment on attachment 8634362 [details] [diff] [review] ContainStyleBreakTest Transferring review requests on the tests to dholbert. (Note that I may have suggested some new tests in code review comments above.) Also, dholbert and kzentner should figure out which of them should finish this work up.

I've been planning on closing the loop here, but haven't gotten to it yet (and I'm on vacation for the next couple weeks). But I intend to finish off the remaining bits here and on the other "contain:" bugs once I'm back. (unless Kyle takes care of misc. fixup first, which he should feel free, but not pressured, to do.)

Kyle's back (woohoo!) and I believe he'll be looking into addressing review feedback here. I'm still on the hook to review the attached test patches, though -- I'll do that in the next day or so.

Comment on attachment 8634362 [details] [diff] [review] ContainStyleBreakTest Review of attachment 8634362 [details] [diff] [review]: ----------------------------------------------------------------- This "page-break-*" test patch needs a bit of work, as noted below -- marking r- for the moment. ::: layout/reftests/w3c-css/submitted/contain/contain-style-break-001.html @@ +1,5 @@ > +<!DOCTYPE HTML> > +<html> > +<head> > + <meta charset="utf-8"> > + <title>CSS Test: 'contain: style' causes page-break-* to be ignored on descendants.</title> How does this test verify that page-break-* is ignored? I think this might need to use "reftest-print" if it actually wants to test page-break-* behavior... (And in that case, it can't be a w3c-css-submitted test, since "reftest-print" is a Mozilla-specific thing. So then these tests should live in layout/reftests somewhere.) @@ +12,5 @@ > + } > + .container { > + contain: style; > + width: 20em; > + height: 10em; Since you've got 10 of these elements stacked vertically, each ~14em tall, this test ends up being several thousand pixels tall, which is too tall for a reftest to be. (Anything outside of the viewport won't get snapshotted and hence won't be testable.) So, a few things: - These elements probably need to be smaller and/or fewer in number. - Look at how they get laid out into a short 'reftest-print' page, too (by watching reftest run, and/or inspecting a reftest failure snapshot). IIRC "reftest-print" pages are 3 inches tall, minus some reserved page margins. I think only a few [maybe 3?] of those pages will fit into a desktop reftest window. So, be sure that all the content that you care about fits into those tiny pages (or onto a single tiny page even, if you're trying to test that no page breaks are forced). - Probably best to avoid 'em' units for sizing in reftests, unless you're explicitly trying to test em units (or explicitly controlling what an 'em' is, by setting 'font-size'). It's clearer & more consistent to just use px values, for more fine-grained control. @@ +29,5 @@ > + height: 10em; > + } > + </style> > +</head> > +<body class="reftest-paint"> Drop "reftest-paint" here -- there is no such thing as "reftest-paint". (I think this might have wanted to say "reftest-wait", but it's not needed in this testcase, so you can just get rid of it here.) (And if you did want 'reftest-wait', it would go on <html>, not on <body>.) ::: layout/reftests/w3c-css/submitted/contain/contain-style-break-002.html @@ +1,2 @@ > +<!DOCTYPE HTML> > +<html class="reftest-wait"> As with the previous test file, this should also be "reftest-print". (and needs to have shorter elements so that everything that you care about will actually fit) @@ +18,5 @@ > + background: orange; > + border: 1em solid black; > + margin: 1em; > + } > + .b-after { "b-after" isn't used at all in this testcase. Should it be? @@ +40,5 @@ > + var containers = document.getElementsByClassName('container'); > + var toInsert = document.getElementById('b-before-template').textContent; > + for (var idx in containers) { > + var container = containers[idx]; > + containers.innerHTML = toInsert; s/containers/container/ in this line, I think? (containers is the set; container is the current one) Also, the <script type="template"> / textContent / innerHTML stuff feels hacky. (And, assuming you actually want to test "b-after" here, it's hard to make it support that class as well.) It'd be better to build your DOM programmatically, something like the following [untested] example-code: function buildSubtree(innerClassName) { var outerDiv = document.createElement("div"); var innerDiv = document.createElement("div"); innerDiv.setAttribute("class", innerClassName); outerDiv.appendChild(innerDiv); return outerDiv; } function doTest() { ... // ...do for first half of containers: container.appendChild(buildSubtree("b-before")); // ...do for second half of containers: container.appendChild(buildSubtree("b-after")); } (I'm assuming you want "b-after" on the second half, simply based on your other testcase.) @@ +46,5 @@ > + document.documentElement.classList.remove("reftest-wait"); > + } > + </script> > +</head> > +<body class="reftest-paint" onload="doTest()"> As in the other testcase, drop "reftest-paint" here.) @@ +47,5 @@ > + } > + </script> > +</head> > +<body class="reftest-paint" onload="doTest()"> > + <div class="container"> </div> It's better to just just make your leaf-nodes completely empty, instead of having a blank space inside of them. IIRC, a blank space inside of a div will create a textNode with value " " in the DOM. It doesn't render in the page's layout, but it does show up in the DOM, making certain debugging steps a little more confusing (because you'll see these textNodes as actual children) ::: layout/reftests/w3c-css/submitted/contain/reftest.list @@ +7,5 @@ > == contain-paint-formatting-context-float-001.html contain-paint-formatting-context-float-001-ref.html > == contain-paint-formatting-context-margin-001.html contain-paint-formatting-context-margin-001-ref.html > == contain-style-quote-001.html contain-style-quote-001-ref.html > +== contain-style-break-001.html contain-style-break-001-ref.html > +== contain-style-break-002.html contain-style-break-001-ref.html If these test files are using "contain-style-break-001-ref.html" as their reference, they should be numbered -001a and -001b, not -001 and -002.

(In reply to Daniel Holbert [:dholbert] from comment #63) > How does this test verify that page-break-* is ignored? I think this might > need to use "reftest-print" if it actually wants to test page-break-* > behavior... (And in that case, it can't be a w3c-css-submitted test, since > "reftest-print" is a Mozilla-specific thing. So then these tests should > live in layout/reftests somewhere.) (Tangent: I just discovered that we do actually have a handful of "w3c-css/submitted" testcases that use reftest-print, but I think that's a mistake -- so I still think new reftest-print testcases should go elsewhere, for now. I posted over in bug 685012 comment 49 to ask about the existing ones.)

Comment on attachment 8636765 [details] [diff] [review] ContainStyleCountersTest Review of attachment 8636765 [details] [diff] [review]: ----------------------------------------------------------------- Review notes on contain-layout-empty-* in this patch (I'll review the contain-style-* parts separately): ::: layout/reftests/w3c-css/submitted/contain/contain-layout-empty-001-ref.html @@ +1,1 @@ > +<!DOCTYPE HTML> Is this file actually used? Its corresponding testcase looks nothing like it (and looks like it's really a mochitest). @@ +15,5 @@ > + height: 0; > + } > + </style> > +</head> > +<body class="reftest-paint"> As noted in comment 63 (for another test patch), please drop "reftest-paint" throughout this patch as well. @@ +16,5 @@ > + } > + </style> > +</head> > +<body class="reftest-paint"> > + <div class="fun"> This class="fun" is unused, so let's get rid of it. ::: layout/reftests/w3c-css/submitted/contain/contain-layout-empty-001.html @@ +5,5 @@ > + <title>CSS Test: 'contain: layout' on various elements causes them to act as > + though they are empty</title> > + <link rel="author" title="Kyle Zentner" href="mailto:kzentner@mozilla.com"> > + <link rel="help" href="http://www.w3.org/TR/css-containment-1/#containment-layout"> > + <link rel="match" href="layout-zero-001-ref.html"> The reference file "layout-zero-001-ref.html" here doesn't seem to exist. Also: this testcase is "contain-layout", I think it really belongs on bug 1178895 (or a followup) -- not as part of this bug. @@ +25,5 @@ > + "inline-block", "inline-table", "table", "table-cell", "table-column", > + "table-column-group", "table-footer-group", "table-header-group", > + "table-row", "table-row-group", "flex", "inline-flex", "grid", > + "inline-grid", "ruby", "ruby-base", "ruby-text", "ruby-base-container", > + "ruby-text-container ", "run-in"]; If this testcase really wants to be a mochitest (as I think it wants to be, as noted below), we should build this array of display values dynamically, using gCSSProperties["display"], provided by property_database.js. Otherwise, this list is doomed to end up incomplete (or with stale values). @@ +34,5 @@ > + outerIdx += 1; > + innerIdx = 0; > + } > + if (outerIdx === displays.length) { > + return; I think this wanted to be "return false" (to explicitly break out of the while loop in the caller). Though as noted below for doTest, I think we should restructure the logic anyway, so don't worry about this return statement for the moment. @@ +52,5 @@ > + var realSnapshot = snapshotWindow(window); > + outerElem.classList.remove("container"); > + innerElem.classList.remove("contained"); > + outerElem.classList.add("fake-container"); > + innerElem.classList.add("fake-contained"); Two things: (1) When you say "fake" here, I think you really mean "reference", right? (based on the fact that it's used to generate a reference for assertSnapshots below) Maybe use "ref" or "reference" instead of "fake" in your class/variable-naming, be clearer about its purpose. (2) This chunk of ~27 lines of code is a bit hard to read/understand. Could you add a couple comments here, to label what each section of code is doing at a high level (and maybe a few newlines between conceptual chunks), to make things easier to follow? @@ +60,5 @@ > + outerElem.removeChild(point); > + innerElem.style.position = "absolute"; > + innerElem.style.left = rect.x; > + innerElem.style.top = rect.y; > + holder.appendChild(innerElem); Does innerElem ever get removed from |holder| here? (after we've generated the "fake" snapshot). It doesn't look like it does, which I expect means 'holder' ends up holding a zillion copies of this element. Can you make sure it's removed? (and make the removal clear, e.g. in an explicit "// clean up" section at the end of this function) @@ +62,5 @@ > + innerElem.style.left = rect.x; > + innerElem.style.top = rect.y; > + holder.appendChild(innerElem); > + var fakeSnapshot = snapshotWindow(window); > + assertSnapshots(realSnapshot, fakeSnapshot, true, null, "contain: layout", "position: absolute"); Does 'assertSnapshots' actually work here? I think that's a mochitest-provided function, right? So I think this reftest really want to be a mochitest? (And its reference case might be unused?) @@ +67,5 @@ > + return true; > + } > + function doTest() { > + while (tick()) { } > + } I'd like to restructure the code here. My concerns about the current structure: - "while(tick()) { }" is a bit infinite-loop-looking. tick being complex -- is a bit infinite-loop-looking, and tick()'s structure seems a bit odd so that it can support this way of being called. - tick() seems like it really wants to just be a double-for-loop, but it's flattened out in a way that's harder to read & easy to accidentally mess up. IMO a clearer way of structuring this would be something like: function doTest() { var outerIdx = 0; var innerIdx = 0; for (var outerIdx = 0; outerIdx < displays.length; outerIdx++) { for (var innerIdx = 0; innerIdx < displays.length; innerIdx++) { testDisplayValues(displays[outerIdx], displays[innerIdx]); } } ...and then testDisplayValues would have the bulk of tick()'s logic. @@ +70,5 @@ > + while (tick()) { } > + } > + //function doTest() { > + //displays.forEach(function (outer) { > + //displays.forEach(function (inner) { Please remove all of this commented-out code.

(In reply to Daniel Holbert [:dholbert] from comment #65) > IMO a clearer way of structuring this would be something like: > > function doTest() { > var outerIdx = 0; > var innerIdx = 0; > > for (var outerIdx = 0; outerIdx < displays.length; outerIdx++) { > for (var innerIdx = 0; innerIdx < displays.length; innerIdx++) { > testDisplayValues(displays[outerIdx], displays[innerIdx]); > } [er, sorry for my poor editing; clearly the first two lines (declaring outer/innerIdx as local vars) could be removed from this sample code]

...and one more note on "reftest-print" in ContainStyleBreakTest: As noted in bug 685012 comment 50 / 51, it looks like the w3c-css directory *can* have reftest-print tests after all, but you just need to add an "@page" rule to prompt other browsers to paginate, since they don't understand what "reftest-print" means. So, no need to move your contain-style tests to another directory -- just give them <html class="reftest-print"> along with an @page rule like the one in http://mxr.mozilla.org/mozilla-central/source/layout/reftests/w3c-css/submitted/css21/pagination/moz-css21-block-page-break-inside-avoid-1.html?force=1

Comment on attachment 8636765 [details] [diff] [review] ContainStyleCountersTest Review of attachment 8636765 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/reftests/w3c-css/submitted/contain/contain-style-counter-increment-001-ref.html @@ +13,5 @@ > + } > + .inc:before { > + counter-increment: i; > + content: "Counter = " counter(i); > + height: 2rem; Get rid of this 'height' -- it has no effect, because the element it applies to (a generated content node) has (default) 'display:inline'. This applies to 4 files: contain-style-counter-increment-001.html & its reference (quoted here) contain-style-counter-insert-001.html & its reference ::: layout/reftests/w3c-css/submitted/contain/contain-style-counter-insert-001.html @@ +27,5 @@ > + <script> > + function doTest() { > + document.getElementById('c1').innerHTML = '<div class="inc"> </div>'; > + document.getElementById('c2').innerHTML = '<div class="inc"> </div>' + > + '<div class=style="counter-reset: i 2;"> </div>'; Typo -- drop "class=" from "class=style=" in this line. ::: layout/reftests/w3c-css/submitted/contain/contain-style-counter-scope-007.html @@ +37,5 @@ > + function doTest() { > + document.getElementById('c').innerHTML = > + document.getElementById('c-template').textContent; > + var body = document.getElementsByTagName('body')[0]; > + body.innerHTML = document.getElementById('body-before-template').textContent + body.innerHTML; Unfortunately, I don't think this file actually tests what it wants to test. Its title says that it's testing "dynamically inserted counter-reset scope above container" -- but, since the dynamic modification (quoted here) is done via clobbering body.innerHTML, there's not *actually* any insertion/reparenting. The test effectively renders one document, and then replaces it with all-new HTML which has to be re-parsed and rebuilt into a new DOM from scratch. (At least, an all-new DOM subtree inside of <body>.) If you want to test DOM mutations and reparenting (and I think you do), I'm pretty sure you need to use DOM APIs like someNode.appendChild, not body.innerHTML. ::: layout/reftests/w3c-css/submitted/contain/contain-style-counter-scope-008.html @@ +33,5 @@ > + document.getElementById('c').innerHTML = > + document.getElementById('c-template').textContent; > + var body = document.getElementsByTagName('body')[0]; > + body.innerHTML += > + document.getElementById('body-after-template').textContent; Same here -- I'm pretty sure "body.innerHTML +=" overwrites the current value of body.innerHTML, which means we have to re-parse the HTML, which means this test is really loading one DOM and then a completely-different DOMs, instead of loading one DOM and then a modified version of that same DOM. So, as with the previous test, I think you need to be using DOM APIs like appendChild here, instead of innerHTML. ::: layout/reftests/w3c-css/submitted/contain/contain-style-counter-scope-009.html @@ +35,5 @@ > + document.getElementById('c').innerHTML = > + document.getElementById('c-template').textContent; > + var body = document.getElementsByTagName('body')[0]; > + body.innerHTML += > + document.getElementById('body-after-template').textContent; Same here. ::: layout/reftests/w3c-css/submitted/contain/reftest.list @@ +12,5 @@ > +== contain-style-counter-increment-001.html contain-style-counter-increment-001-ref.html > +== contain-style-counter-insert-001.html contain-style-counter-insert-001-ref.html > +== contain-style-counter-scope-001.html contain-style-counter-scope-001-ref.html > +== contain-style-counter-scope-002.html contain-style-counter-scope-001-ref.html > +== contain-style-counter-scope-003.html contain-style-counter-scope-001-ref.html These should be numbered "001a", "001b", "001c", etc. (not "001", "002", "003"), since they all share 001-ref as their reference case. That way, someone viewing a particular testcase here can quickly predict the name of the reference case, without having to do any research.

Comment on attachment 8645197 [details] [diff] [review] ContainStyleQuotesTest Review of attachment 8645197 [details] [diff] [review]: ----------------------------------------------------------------- This quotes test-patch looks pretty good -- r=me with the following addressed: ::: layout/reftests/w3c-css/submitted/contain/contain-style-quote-001-ref.html @@ +31,5 @@ > +Nec at facilisi ullamcorper vituperatoribus, stet ipsum luptatum sit eu, volumus invenire ea sea. An mentitum suscipit sea, tractatos theophrastus ad cum, pro nonumy soluta vivendum ei. Sonet quando labores has ut, ad justo fabulas senserit has, tale concludaturque sea ne. Has nominavi intellegebat eu, eum ubique recteque deterruisset ut, nullam aliquip vim no. Porro latine disputationi vix ei, usu no nihil integre, no facer forensibus nam. Eu vero affert nec. > + </p> > + > + <div class="set nqclose"> </div> > + <div class="set nqclose"> </div> The "set" class doesn't have any style associated with it. Drop that class here, I think? @@ +36,5 @@ > + <p class="qopen qclose"> > +At cum erat integre, id vis tritani laboramus. Vero partiendo conclusionemque ex est. Inani partiendo splendide sed no. Putant doctus interpretaris vis id, eius habemus eu eam, quot vivendum vim at. Usu dolore nostro accumsan ea. > + </p> > + <div class="set nqopen"> </div> > + <div class="set nqopen"> </div> (Here too) ::: layout/reftests/w3c-css/submitted/contain/contain-style-quote-001.html @@ +8,5 @@ > + <link rel="match" href="style-quote-001-ref.html"> > + <style> > + body { > + margin: 2em; > + } I don't think this increased margin on the body is useful here -- none of the content is in danger of running off of the left or top side. Maybe get rid of this 'body' rule, to simplify these tests? (This applies to all 4 tests included in this patch.) @@ +21,5 @@ > + } > + </style> > +</head> > +<body> > + <p class="qopen"> Remove trailing space here. (This applies to the reference case, too). @@ +22,5 @@ > + </style> > +</head> > +<body> > + <p class="qopen"> > +Lorem ipsum dolor sit amet, cu per agam quaerendum reprehendunt, scripta vivendo perfecto vis eu. Sea fastidii principes an. Sed in duis senserit, et cum impedit voluptaria, in nam sumo affert recteque. Sanctus suavitate voluptatibus te ius. Eos id insolens gloriatur repudiandae, vim in adhuc principes reprimique, vis ad omnium pericula reprimique. Two things: (1) Please reduce the amount of text in this testcase & its reference, so that it can actually fit on-screen on a mobile display. (Right now, if I view the testcase in a ~phone-sized window, most of the testcase is offscreen, including the entire style-containment node that we're trying to test.) (2) Please wrap long lines of text to 80 chars, unless they need to be long for some reason. (though suggestion-(1) may make this a non-issue, if you get rid of most of the text) (These apply to the reference case, too). ::: layout/reftests/w3c-css/submitted/contain/contain-style-quote-002.html @@ +26,5 @@ > + </script> > + <script> > + function doTest() { > + document.getElementById('inner').innerHTML = > + document.getElementById('inner-template').textContent; (Note: *here*, the innerHTML tweak looks fine to me, in contrast to the usages that I griped about in my previous bug-comment. Here, we're adding content *inside of* an existing DOM node and seeing what happens -- this is OK. But in the other tests, the innerHTML modification ends up inadvertantly clobbering & replacing the nodes whose interactions we'd like to test. Hopefully the contrast between those tests & this test makes that clearer.)

As Kyle was inactive for more than a year now, I think it's safe to unassign him. Sebastian

The spec has now changed, and the patches here likely don't apply anymore). In the interests of sanity, I'm going to close this bug as "incomplete" and file a new bug for the updated implementation. (The patches/discussion here will definitely inform the new bug, though!)