Closed Bug 1170781 (css-contain-paint) Opened 9 years ago Closed 9 years ago

Implement CSS 'contain: paint'

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla45
Tracking Flags:
Tracking Status
firefox41 --- affected
firefox45 --- fixed

People

(Reporter: zentner.kyle, Assigned: zentner.kyle)

References

(
URL
)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 19 obsolete files)

ContainPaint [r=dholbert]
8.09 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
ContainPaintTest [r=dholbert]
24.55 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
See http://dev.w3.org/csswg/css-containment/#containment-paint for details about this portion of the css 'contain' property.
(Dropping dev-doc-needed -- bug 1150081 is the main bug for this feature, and it already has this keyword. This bug here exists just to have a dedicated place to implement/discuss this particular part of the feature.)
Keywords: dev-doc-needed
Attached patch ContainPaint (obsolete) — — Splinter Review
This patch adds the behavior for 'contain: paint'.
Attachment #8627902 - Flags: review?(dholbert)
Attached patch ContainPaintTest (obsolete) — — Splinter Review
This patch adds tests for the common cases of 'contain: paint'.
Attachment #8627904 - Flags: review?(dholbert)
Comment on attachment 8627902 [details] [diff] [review] ContainPaint Initial review notes: >diff --git a/layout/generic/nsBlockFrame.cpp b/layout/generic/nsBlockFrame.cpp >--- a/layout/generic/nsBlockFrame.cpp >+++ b/layout/generic/nsBlockFrame.cpp >+ // If the box has contain: paint (or contain: strict), then it should also >+ // establish a fromatting context. Typo: s/fromatting/formatting/ >+++ b/layout/style/nsRuleNode.cpp >@@ -5707,17 +5707,24 @@ nsRuleNode::ComputeDisplayData(void* aSt >+ if (display->IsPaintContaining()) { >+ // This is supposed to cause contain: paint and contain: layout elements >+ // to become formatting contexts. However, exactly what formatting >+ // context means has not been specificed yet. >+ // Most likely, the intended behavior is similar to EnsureBlockDisplay. >+ // XXX This will convert 'inline' to 'block' (instead of 'inline-block'). Is that right? >+ EnsureBlockDisplay(display->mDisplay); Typo: s/specificed/specified/ Also: please also file a bug in Core|CSS Parsing & Computation on finalizing this behavior, and include the bug number in this XXX comment. That way, when we go to address that followup later on, it'll be easier to find the code that needs updating by just searching for its bug number. (I suspect there may end up being more than one piece of code that have this maybe-EnsureBlockDisplay() pattern.) >@@ -2244,16 +2244,20 @@ struct nsStyleDisplay { >+ bool IsPaintContaining() const { >+ return mContain & NS_STYLE_CONTAIN_PAINT; >+ } >+ This method-name is a little vague/abstract. I'd prefer we just name these accessors "IsContainPaint", "IsContainStyle", and "IsContainLayout" (or maybe "HasContain$FOO"), to make it clearer that this is just a convenience accessor for "contain" property values, and not something more complex.
(In reply to Daniel Holbert [:dholbert] from comment #6) > >+ // This is supposed to cause contain: paint and contain: layout elements > >+ // to become formatting contexts. However, exactly what formatting > >+ // context means has not been specificed yet. > >+ // Most likely, the intended behavior is similar to EnsureBlockDisplay. > >+ // XXX This will convert 'inline' to 'block' (instead of 'inline-block'). Is that right? > >+ EnsureBlockDisplay(display->mDisplay); [...] > Also: please also file a bug in Core|CSS Parsing & Computation on finalizing > this behavior, and include the bug number in this XXX comment. Moreover, I'd just collapse the final two comment-lines together as: // Most likely, the intended behavior is similar to EnsureBlockDisplay, // except with 'inline' converted to 'inline-block' instead of 'block'. // So EnsureBlockDisplay isn't quite right here; see bug NNN. or something like that.
Comment on attachment 8627902 [details] [diff] [review] ContainPaint >+++ b/layout/generic/nsFrame.cpp >@@ -2241,16 +2241,26 @@ nsIFrame::BuildDisplayListForChild(nsDis > // If painting is restricted to just the background of the top level frame, > // then we have nothing to do here. > if (aBuilder->IsBackgroundOnly()) > return; > > nsIFrame* child = aChild; > if (child->GetStateBits() & NS_FRAME_TOO_DEEP_IN_FRAME_TREE) > return; >+ const nsStyleDisplay* ourDisp = StyleDisplay(); >+ Maybe<DisplayListClipState::AutoSaveRestore> clip; You'd want to add a newline between the early-return and your new code here -- but, I think this whole code might want to move further down, becuase a page or so down, I just noticed we have this thing: DisplayListClipState::AutoClipMultiple clipState(aBuilder); ...which we use to do various types of clipping. I think you want to use that, instead of adding a new DisplayListClipState::AutoSaveRestore here. That class is documented here: http://mxr.mozilla.org/mozilla-central/source/layout/base/DisplayListClipState.h#223 and basically it seems like it's a way to avoid multiple AutoSaveRestore instances in this particular method. (This is essentially r=me with the above addressed, but I should probably review the final state of this chunk after we're using the AutoClipMultiple, so just setting feedback+ for now.)
Attachment #8627902 - Flags: review?(dholbert) → feedback+
Comment on attachment 8627904 [details] [diff] [review] ContainPaintTest Ideally, all reftests for this feature should all live in our submitted-to-w3c reftest subdirectory, so that the spec authors & other implementors can benefit from (and sanity-check/agree on) these tests. This makes it more likely that different implementations will be interoperable. So, they should live in a directory called "contain", here: layout/reftests/w3c-css/submitted/ They'll also want a bit of extra metadata headers -- see: http://testthewebforward.org/docs/reftests.html You can look at my "object-fit" reftests in http://mxr.mozilla.org/mozilla-central/source/layout/reftests/w3c-css/submitted/images3/ for reference. Notes: (1) the w3c reftest harness uses <link rel="match"> to connect testcases to their reference cases. So, each testcase will need one of those tags. (on top of reftest.list, which is what we use locally) (2) W3C-submitted reftests should be named -001, -002, etc., per the w3c's test guidelines. (They can have an optional single letter after that, if you find yourself wanting to make "a" and "b" variants of a particular test.) (3) W3C-submitted reftests include a header tag linking to the section of the spec that they're testing. This should optimistically link to the WD (working draft) of the spec, not the ED (editor's draft). In this case, you probably want to use http://www.w3.org/TR/css-containment-1/#containment-paint (Yes, that's a broken link, but it's likely where the spec will end up; and the test-submission script that we run periodically explicitly requires that we're linking to an official spec URL and not an editor's draft.) (4) You should include a hint in English somewhere in the test (in the <title>, and perhaps in comments if there's anything non-obvious beyond that) about what you're testing in a particular test (and how it differs from the other tests).
Two more nits on the tests: - paint-clip.html has "padding-width: 0" in both the testcase & reference case, which I think is invalid CSS & can be dropped. - It looks like some of your testcases/references (e.g. "paint-clip.html" & its reference case) use absolute positioning. It's probably best to avoid using absolute positioning (except when you're explicitly trying to test interactions with abspos stuff) because it adds some complexity. If you're just trying position something further from the upper-left corner of the page, it's best to just give the element a nonzero margin.
Please ignore the previous comment, I figured out how to get the existing clipState to do what I want.
Yay! When you repost, could you tag mattwoodrow to review the nsFrame::BuildDisplayList bit? (He reviewed the introduction of AutoClipMultiple, so he's more likely than I am to know if you're using it correctly.)
Blocks: 1179349
Attached patch ContainPaint (obsolete) — — Splinter Review
I think I addressed all of the concerns for the first patch.
Attachment #8627902 - Attachment is obsolete: true
Attachment #8628428 - Flags: review?(matt.woodrow)
Attachment #8628428 - Flags: review?(dholbert)
Attached patch ContainPaintTest (obsolete) — — Splinter Review
Attachment #8627904 - Attachment is obsolete: true
Attachment #8627904 - Flags: review?(dholbert)
Attachment #8628430 - Flags: review?(dholbert)
Comment on attachment 8628428 [details] [diff] [review] ContainPaint Just one nit: >+++ b/layout/style/nsStyleStructInlines.h > bool > nsStyleDisplay::IsFixedPosContainingBlock(const nsIFrame* aContextFrame) const > { >- return (HasTransform(aContextFrame) || HasPerspectiveStyle() || >+ return ((HasTransform(aContextFrame) || HasPerspectiveStyle() || > !aContextFrame->StyleSVGReset()->mFilters.IsEmpty()) && >- !aContextFrame->IsSVGText(); >+ !aContextFrame->IsSVGText()) || IsContainPaint(); > } This would be more readable if we moved the IsContainPaint() check to the beginning. (That change might impact perf if IsContainPaint were expensive, but luckily it's not.) Right now, it kinda blends in with the existing complex multi-part condition -- I'd prefer: return IsContainPaint() || ((HasTransform(aContextFrame) || HasPerspectiveStyle() || !aContextFrame->StyleSVGReset()->mFilters.IsEmpty()) && !aContextFrame->IsSVGText()); (Note that I've also adjusted the indentation on the last & 2nd-to-last lines to match the parenthesis-grouping-level -- that indentation was off a bit on those lines in your patch. Please make sure code is indented to be aligned with other stuff in the same grouping-level.) r=me with that, & I'm deferring to mattwoodrow to review the nsFrame::BuildDisplayList code. Thanks!
Attachment #8628428 - Flags: review?(dholbert) → review+
Comment on attachment 8628430 [details] [diff] [review] ContainPaintTest Review of attachment 8628430 [details] [diff] [review]: ----------------------------------------------------------------- You should include a test (maybe paint-clip-002.html) which has content -- e.g. text or an image -- which gets clipped. Note that you can't make strong assumptions about exact fonts/sizes, so I'd suggest just having a longish line of text & comparing "contain:paint" vs. "overflow:hidden" in the reference case. Something like the following: (with proper <style>-stylesheet instead of inline styles): <div style="contain:paint; width: 50px; border: 1px solid black"> <div style="width: 300px; margin-left: -50px;"> This is a long string of text that should be clipped at the black border. </div> </div> And then the reference case would be identical except with "overflow:hidden". (The margin-left and the wide 'width' on the inner div should let us test the text overflowing on both the left and the right.) ::: layout/reftests/w3c-css/submitted/contain/paint-clip-001-ref.html @@ +1,5 @@ > +<!DOCTYPE HTML> > +<html> > +<head> > + <meta charset="utf-8"> > + <title>CSS Reftest Reference: 'contain: paint' with various overflowing block descendants.</title> I'm pretty sure w3c reference titles should just be "CSS Reftest Reference". (The rest of the title here, "'contain: paint' with various overflowing block descendants", doesn't really apply to anything in this file.) @@ +3,5 @@ > +<head> > + <meta charset="utf-8"> > + <title>CSS Reftest Reference: 'contain: paint' with various overflowing block descendants.</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-paint"> Also no need for a <link rel="help" tag in the reference case, since that spec link doesn't describe anything that's being exercised in this file. This comment & the one above about <title> apply to all of the reference cases here, I think. (If you like, you can edit the patch file directly in emacs to tweak/remove these lines en masse, and it should update the patch metadata [e.g. line-count] appropriately -- at least, that's worked for me in the past. YMMV. :)) ::: layout/reftests/w3c-css/submitted/contain/paint-clip-001.html @@ +33,5 @@ > + height: 100px; > + background: green; > + } > + .large-red-square { > + } The .large-red-square CSS class here is empty & also unused, so probably wants to be removed. @@ +47,5 @@ > + .foreground { > + position: absolute; > + top: 0; > + left: 0; > + background: rgba(0, 0, 0, 0); This background is transparent, I think? (which is the default behavior for background) So I don't think this line adds anything -- let's get rid of it. (This element's point is to render its border, I think?) @@ +59,5 @@ > +<body> > + <div class="root"> > + <div class="b"> > + <div class="c"> </div> > + <div class="a"> </div> nit: ordering the "b/c/a" in the DOM here is a bit counterintuitive. Consider shuffling the names. ::: layout/reftests/w3c-css/submitted/contain/paint-formatting-context-float-001.html @@ +16,5 @@ > + width: 10px; > + background: blue; > + } > + #a { > + contain: paint; Nit: the indentation is inconsistent here. (The #a/#b CSS rules are indented more than body/#left right now) This is true in all of the test files, I think (except paint-clip-001.html). Please make that consistent. (Search-and-replace for " " in the patch file itself may be your friend here.)
Attached patch ContainPaint (obsolete) — — Splinter Review
IsFixedPosContainingBlock is now much more readable.
Attachment #8628428 - Attachment is obsolete: true
Attachment #8628428 - Flags: review?(matt.woodrow)
Attachment #8628850 - Flags: review?(matt.woodrow)
Attached patch ContainPaintTest (obsolete) — — Splinter Review
I addressed the concerns you mentioned above. I also updated the reftest.list to actually point to the new test names (whoops).
Attachment #8628430 - Attachment is obsolete: true
Attachment #8628430 - Flags: review?(dholbert)
Attachment #8628851 - Flags: review?(dholbert)
Comment on attachment 8628851 [details] [diff] [review] ContainPaintTest Looks good, thanks! r=me
Attachment #8628851 - Flags: review?(dholbert) → review+
Comment on attachment 8628850 [details] [diff] [review] ContainPaint Review of attachment 8628850 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsFrame.cpp @@ +2393,5 @@ > + nsRect contentRect = GetContentRectRelativeToSelf() + > + aBuilder->ToReferenceFrame(this); > + nscoord radii[8]; > + GetContentBoxBorderRadii(radii); > + clipState.ClipContentDescendants(contentRect, radii); I believe we need to use a separate AutoSaveRestore object for this clip, because ClipContentDescendants will set mClipUsed to true, and then the ApplyClipPropClipping path below will assert that this isn't set (if taken). We need a new stack object (with contained DisplayItemClip) for each clip we want to apply. AutoClipMultiple provides two stack DisplayItemClip objects for convenience/performance, but these are both used (potentially) with the existing clip calls. If it's not possible to hit the ApplyClipPropClipping path when IsContainPaint is true then I think we'd be ok, but we'll want to add assertions/comments to make this very clear, since it's non-obvious currently.
Ah, sorry for leading you astray w/ AutoClipMultiple; thanks for the info, matt! So, you probably do want something closer to the code I was commenting on in comment 8. Though, you probably should insert the new Maybe<AutoSaveRestore> closer to where we have the AutoClipMultiple, instead of at the very top of the function -- that way: (1) we'll be logically grouping clipping operations (which is nice for code-understandability). (2) we'll be doing this new work after a bunch of "we're not going to actually paint" early-return cases, instead of having to do it before those early-returns
Attached patch ContainPaint (obsolete) — — Splinter Review
I had an older version of the patch in version control, so I just pulled out and retested this version.
Attachment #8628850 - Attachment is obsolete: true
Attachment #8628850 - Flags: review?(matt.woodrow)
Attachment #8628987 - Flags: review?(matt.woodrow)
Attachment #8628987 - Flags: review?(matt.woodrow) → review+
Two things: (1) The Try run ( https://treeherder.mozilla.org/#/jobs?repo=try&revision=78e0ffb152ab ) turned up an interesting issue -- it looks like tall content inside of "contain:paint" can trigger scrollbars right now. (i.e. it affects scrollable overflow, even though it's not painted) That is likely undesirable. But, don't worry about that too much, because: (2) The Editor's Draft was changed just yesterday, to indicate that "contain:paint" is now supposed to take effect by influencing "overflow", and making it take on a new value "clip": https://hg.csswg.org/drafts/rev/b1853180ac26 Apparently the CSSWG resolved this back in April: https://lists.w3.org/Archives/Public/www-style/2015Apr/0405.html ...though it didn't make it into the spec draft until now. :-/ So, per the new spec text, we really need to implement "overflow: clip", and then "contain: paint" just operates by forcing "overflow" to be at least "clip", basically. It's possible our "overflow: clip" implementation could look very similar to what you already have for "contain:paint", though...
Comment on attachment 8628987 [details] [diff] [review] ContainPaint [tagging current patch as "feedback-" to make it clearer that we're going to have to change strategy here to match the new spec text (making "contain:paint" influence "overflow"), per second half of comment 24.]
Attachment #8628987 - Flags: feedback-
Blocks: 1184248
Attached patch ContainPaint (obsolete) — — Splinter Review
This change (with the tests from the other patch), has been tested at https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4e180449844. Note that I've filed the followup Bug 1184248, to deal with the use of 'overflow: -moz-hidden-unscrollable' instead of 'overflow: clip'.
Attachment #8628987 - Attachment is obsolete: true
Attachment #8634254 - Flags: review?(dholbert)
Attached patch ContainPaintTest (obsolete) — — Splinter Review
These tests had to change slightly, since the spec behavior changed slightly (clip to padding-box instead of content-box).
Attachment #8628851 - Attachment is obsolete: true
Attachment #8634256 - Flags: review?(dholbert)
Comment on attachment 8634254 [details] [diff] [review] ContainPaint Review of attachment 8634254 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/nsRuleNode.cpp @@ +5550,5 @@ > + if (display->mOverflowX == NS_STYLE_OVERFLOW_VISIBLE || > + display->mOverflowY == NS_STYLE_OVERFLOW_VISIBLE) { > + // XXX This actually sets overflow to -moz-hidden-unscrollable. > + display->mOverflowX = NS_STYLE_OVERFLOW_CLIP; > + display->mOverflowY = NS_STYLE_OVERFLOW_CLIP; I don't think this is quite right. The spec says: "If the computed value of overflow-x or overflow-y would otherwise have been visible, it must instead compute to clip." ...but this code sets *both* properties to 'clip'. For example -- it looks like this would do the wrong thing on style like: "contain:paint; overflow-x: visible; overflow-y: scroll" (Looks like it'd nerf the "scroll" value to be "clip" here.) @@ +5732,5 @@ > + // supposed to become a formatting context has not been specified. Most > + // likely, the intended behavior is similar to EnsureBlockDisplay, > + // except with 'inline' converts to 'inline block' instead of 'block'. > + // XXX This behavior is probably wrong in some cases; see bug 1179349. > + EnsureBlockDisplay(display->mDisplay); Per discussion on bug 1179349, I think EnsureBlockDisplay is probably too heavy-handed here, and the "right thing" is actually pretty simple, so we should just do it. I think we *only* need to promote "inline" to "inline-block" here, and we can leave every other display value alone. (Though you should also put an XXX comment here mentioning that 'ruby' values may need some conversions as well, once the spec has sorted them out.) ::: layout/style/nsStyleStructInlines.h @@ +140,5 @@ > { > + return IsContainPaint() || > + ((HasTransform(aContextFrame) || HasPerspectiveStyle() || > + !aContextFrame->StyleSVGReset()->mFilters.IsEmpty()) && > + !aContextFrame->IsSVGText()); I think we need to put IsContainPaint() inside of the mega-condition here, actually -- just add it to the list of existing "||" conditions. (The !aContextFrame->IsSVGText() check needs to "win" and prevent us from being a fixed-pos containing block, or else we can end up with SVG <text> elements having abspos children, which they are not set up to handle. That's why we have that IsSVGText exception here in the first place. [Sorry for not noticing that sooner; glad I caught it before this landed. :)])
Comment on attachment 8634256 [details] [diff] [review] ContainPaintTest Review of attachment 8634256 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the tests with the following addressed: ::: layout/reftests/w3c-css/submitted/contain/contain-paint-clip-001.html @@ +62,5 @@ > + <div class="b"> </div> > + <!--These two absolutely positioned elements are checking that all sides are--> > + <!--clipped. They also test that clipping is done correctly on absolutely--> > + <!--positioned elements.--> > + <div class="background"> </div> For empty elements like this one, probably best to drop the single-space character inside the element. It doesn't have any effect on layout, but it creates an extra text node in the DOM, and makes the testcase marginally more complex as a result. So, this should just be: <div class="background"></div> (You should be able to fix this across the board with search-and-replace for "> </" in your patch file.) ::: layout/reftests/w3c-css/submitted/contain/contain-paint-clip-002-ref.html @@ +13,5 @@ > + height: 100px; > + background: green; > + margin: 25px; > + padding: 25px; > + overflow: hidden; Move "overflow: hidden" to the beginning of this CSS rule -- instead of at the end -- so that it's in the same spot where the testcase has "contain: paint" (which is what this "overflow" decl is mimicking). This makes it easier to compare the testcase & the reference case in a merge tool or with "diff" & understand the differences between them. ::: layout/reftests/w3c-css/submitted/contain/contain-paint-clip-002.html @@ +1,5 @@ > +<!DOCTYPE HTML> > +<html> > +<head> > + <meta charset="utf-8"> > + <title>CSS Test: 'contain: paint' with various overflowing block descendants.</title> Title isn't correct on this test -- this is copypasted from the "001" version, it looks like. (This test here is checking overflowing text content, not "various overflowing block descendants".) @@ +21,5 @@ > + </style> > +</head> > +<body> > + <div class="root"> > + This text should be clipped to the box. Lorem ipsum dolor sit amet, consectetur adipiscing elit. Integer nec odio. Praesent libero. Sed cursus ante dapibus diam. Sed nisi. Nulla quis sem at nibh elementum imperdiet. Duis sagittis ipsum. Praesent mauris. Fusce nec tellus sed augue semper porta. Mauris massa. Vestibulum lacinia arcu eget nulla. Class aptent taciti sociosqu ad litora torquent per conubia nostra, per inceptos himenaeos. Curabitur sodales ligula in libero. Three things: (1) Wrap this line to 80 characters (Alt-q in emacs) so that it's readible in editors/viewers which truncate long lines. (2) There's a trailing space character at the end -- remove that. (3) Please add a long word at the beginning, which will overflow off the right edge of the containment elem, & will let us check that we're clipping in *both* dimensions. (instead of just clipping vertically) e.g. Use a long string of 'a' characters, or the alphabet repeated twice (abcdef...zabcdef...z, 52 characters). (These changes all apply to this paragraph in the reference case, as well.) ::: layout/reftests/w3c-css/submitted/contain/contain-paint-containing-block-absolute-001.html @@ +22,5 @@ > + width: 100px; > + height: 100px; > + background: green; > + top: 0; > + left: 0; Nit: you have this "top"/"left" style at the bottom of the CSS rule here, vs. in the fixedpos version of this test, they're at the top of the rule. Please make that consistent (changing one test or the other), so that the only difference between these two testcases is the *relevant* difference. (their "position" value)
Attachment #8634256 - Flags: review?(dholbert) → review+
Please add a testcase that has "contain:paint; overflow-y: scroll" (the example at the beginning of comment 28), to test the bug that I brought up there & be sure we don't accidentally nerf that overflow value. (If you like, this test could be "contain-paint-clip-003.html", which would be identical to your contain-paint-clip-002.html except that we add "overflow-y: scroll" to the containment element in the testcase & reference case.)
Comment on attachment 8634256 [details] [diff] [review] ContainPaintTest We also probably need a mochitest here to verify we're doing "display" fixup correctly. It should iterate through all of the valid "display" values (from property_database.js), and give some dummy-element style="contain:paint; display: [$displayVal]", and check getComputedStyle(...).display to be sure we didn't modify the display, except for the special-cases, which you can define in a map in the JS file. Right now I think that map of special-cases would just be: { "inline": "inline-block" } ...per comment 28; and we can extend the map with expected conversions for "ruby" display values when we learn what's supposed to happen there. [relaxing r+ on tests patch to feedback+, since I should take another look once you've added this mochitest & the reftest mentioned in previous comment.]
Attachment #8634256 - Flags: review+ → feedback+
Attached patch ContainPaint (obsolete) — — Splinter Review
I've addressed the feedback. I also made list-item change to block in the formatting contextify-logic, since Tab was clear that that was the correct behavior. This patch (and the updated test patch) are on try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0f8f283e872.
Attachment #8634254 - Attachment is obsolete: true
Attachment #8634254 - Flags: review?(dholbert)
Attachment #8635597 - Flags: review?(dholbert)
Attached patch ContainPaintTest (obsolete) — — Splinter Review
I've addressed the feedback and added the two new reftests. I also added a mochitest similar to what you described. Please let me know if it should be different / check fewer extra conditions.
Attachment #8634256 - Attachment is obsolete: true
Attachment #8635598 - Flags: review?(dholbert)
(In reply to Kyle Zentner from comment #32) > I also made list-item change to block in the formatting contextify-logic, > since Tab was clear that that was the correct behavior. I'm not 100% sure that that's what Tab meant. He said "list-item will just become a BFC" (bug 1179349 comment 2), but I don't think that meant "change its display to block". (Note that "display:list-item" does generate a block for its children [along with the list marker]. I think Tab was just saying that its generated block must be a BFC. Worth clarifying, though.)
It's conceivable that someone might want to have a list-item whose children are paint-contained and/or layout-contained. Though I wonder what paint containment would mean for the list symbol... (the bullet, number, etc.) For now, it looks like <li style="overflow: -moz-hidden-unscrollable"> clips the bullet -- but there still is a bullet, as can be seen in the frame tree.)
Whoops. I guess I should have been reading more carefully. I don't think the spec is ambiguous here, since the bullet disappears if either overflow value is not set to visible. I'll remove this part of the patch, and add a test confirming the current behavior.
Thanks. (So that "switch" statement should just become an "if" check then.) (More review feedback coming shortly, if you want to hold off on uploading an updated patch until that's in.)
Attached patch ContainPaint (obsolete) — — Splinter Review
Under test at https://treeherder.mozilla.org/#/jobs?repo=try&revision=537ca9f74fc1
Attachment #8635597 - Attachment is obsolete: true
Attachment #8635597 - Flags: review?(dholbert)
Attachment #8636240 - Flags: review?(dholbert)
Attached patch ContainPaintTest (obsolete) — — Splinter Review
Attachment #8635598 - Attachment is obsolete: true
Attachment #8635598 - Flags: review?(dholbert)
Attachment #8636241 - Flags: review?(dholbert)
Comment on attachment 8636240 [details] [diff] [review] ContainPaint Review of attachment 8636240 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the following addressed: ::: layout/generic/nsBlockFrame.cpp @@ +6649,5 @@ > // (http://dev.w3.org/csswg/css-writing-modes/#block-flow) > + // If the box has contain: paint (or contain: strict), then it should also > + // establish a formatting context. > + if ((GetParent() && StyleVisibility()->mWritingMode != > + GetParent()->StyleVisibility()->mWritingMode) || Indent "GetParent" by 1 more space here, to keep it aligned with the thing above it (which you've pushed rightward by 1 space). ::: layout/style/nsRuleNode.cpp @@ +5544,5 @@ > SETDSC_ENUMERATED | SETDSC_UNSET_INITIAL, > parentDisplay->mOverflowY, > NS_STYLE_OVERFLOW_VISIBLE, 0, 0, 0, 0); > > + // When 'contain: paint', update overflow from 'visible' to 'clip' s/update overflow/update overflow-[x|y]/ Also: add a period at the end of the sentence here. @@ +5733,5 @@ > + // to become formatting contexts. However, exactly how the element is > + // supposed to become a formatting context has not been specified. > + // This behavior should be mostly correct, but ruby and table related > + // elements may need to be changed. > + // XXX This behavior is probably wrong in some cases; see bug 1179349. This large comment feels a bit too abstract. (Made sense before, sine it was previously about behavior that was abstracted away behind EnsureBlockDisplay. But now it's just describing a single minor display-fixup.) Also, I think it's wrong about table elements now - those don't need adjusting here, per bug 1179349 comment 2. Simplify this to say something like: // An element with contain:paint or contain:layout needs to "be a // formatting context". For the purposes of the "display" property, that // just means we need to promote "display:inline" to "inline-block". // XXX We may also need to promote ruby display vals; see bug 1179349. @@ +5737,5 @@ > + // XXX This behavior is probably wrong in some cases; see bug 1179349. > + // It's okay to cache this change in the rule tree for the same > + // reasons as floats in the previous condition. > + switch (display->mDisplay) { > + case NS_STYLE_DISPLAY_INLINE: Per comment 37, let's just make this an "if" check. Switch feels like overkill here. (though if we end up fixing up another pile of display values down the line, we can upgrade this to use switch.) ::: layout/style/nsStyleStructInlines.h @@ +139,5 @@ > nsStyleDisplay::IsFixedPosContainingBlock(const nsIFrame* aContextFrame) const > { > + return (IsContainPaint() || HasTransform(aContextFrame) || > + HasPerspectiveStyle() || > + !aContextFrame->StyleSVGReset()->mFilters.IsEmpty()) && Indentation is off on the 2nd and 3rd lines here. They're inside of a parenthesized expression, so they should be indented to be inside that expression, to make the nesting clear. So: please indent HasPerspectiveStyle and !aContextFrame to line up directly below IsContainPaint.
Attachment #8636240 - Flags: review?(dholbert) → review+
Attached patch ContainPaint (obsolete) — — Splinter Review
I think I've addressed all the feedback. This patch is under test at https://treeherder.mozilla.org/#/jobs?repo=try&revision=c531b19ac842
Attachment #8636240 - Attachment is obsolete: true
Attachment #8636264 - Flags: review?(dholbert)
Comment on attachment 8636264 [details] [diff] [review] ContainPaint >+++ b/layout/style/nsRuleNode.cpp >+ if (display->IsContainPaint()) { >+ // This is supposed to cause contain: paint and contain: layout elements >+ // to become formatting contexts. >+ // XXX This behavior is probably wrong in some cases; see bug 1179349. [...] >+ if (display->mDisplay == NS_STYLE_DISPLAY_INLINE) { >+ display->mDisplay = NS_STYLE_DISPLAY_INLINE_BLOCK; >+ } The "This is supposed to cause[...]" formulation here is a bit odd. It implies that this code here is all that's required to make us a formatting context. (But it's not, as you know; there are also further requirements unrelated to "display" which are handled elsewhere.) Please use something a bit more specific, closer to my suggested text in comment 40. (Feel free to copypaste if you like.) r=me with that. No need to re-request review; just reupload with "r+" already set, and add "[r=dholbert]" in the attachment name on bugzilla if you like, to be extra explicit for sheriffs.
Attachment #8636264 - Flags: review?(dholbert) → review+
Attached patch ContainPaint [r=dholbert] (obsolete) — — Splinter Review
Oh, I missed that you had suggested text for the comment. This patch uses the text from comment 40.
Attachment #8636264 - Attachment is obsolete: true
Attachment #8636268 - Flags: review+
Comment on attachment 8636241 [details] [diff] [review] ContainPaintTest Review of attachment 8636241 [details] [diff] [review]: ----------------------------------------------------------------- Review notes on mochitest: ::: layout/style/test/mochitest.ini @@ +278,5 @@ > [test_setPropertyWithNull.html] > [test_attribute_selector_eof_behavior.html] > [test_css_loader_crossorigin_data_url.html] > [test_box_size_keywords.html] > +[test_contain_formatting_context.html] The top section of this file is alphabetically sorted, and we should stick with that. (I'm not sure why the bottom section here is the wild west, sorting-wise.) So -- please insert this new test up top, in the right alphabetical spot (after test_condition_text_assignment.html). ::: layout/style/test/test_contain_formatting_context.html @@ +17,5 @@ > + // Atkins Jr. in bug 1179349. Ultimately, it should be based on a > + // specification. > + var expectedChanges = { > + 'inline' : 'inline-block', > + 'list-item' : 'block', (This 'list-item' entry needs to be removed here, of course.) @@ +26,5 @@ > + 'ruby-text', > + 'ruby-base-container', > + 'ruby-text-container', > + 'run-in', > + 'contents' "contents" is now known to be no-change, I think, right? (per Tab's response to your post on the list) So I don't think that entry should be in the unknownChange list. @@ +51,5 @@ > + ]; > + var displayInfo = gCSSProperties["display"]; > + var test = document.getElementById('test'); > + var displayVals = displayInfo.initial_values.concat(displayInfo.other_values); > + var deDent = ((s) => s.replace(/ +/g, ' ')); What's this deDent thing? Doesn't seem to be used here, so I think it can be removed? @@ +63,5 @@ > + is(getComputedStyle(test).display, dispVal, > + `'contain: paint' should not change 'display: ${dispVal}'`); > + } else if (~unknownChange.indexOf(dispVal)) { > + todo(false, > + `Should 'display: ${dispVal}' change with 'contain: paint'?`); So, we're not testing our behavior at all for the display values in this "unknownChange" category. So, we could change their behavior without this test noticing. That's somewhat bad. Please *do* test the behavior for these ones instead of using this "todo", and perhaps just add an XXX comment mentioning something like "Note: display:ruby[-*] & display:run-in may end up needing to be added to the expectedChange array, as the CSS Containment spec matures." @@ +65,5 @@ > + } else if (~unknownChange.indexOf(dispVal)) { > + todo(false, > + `Should 'display: ${dispVal}' change with 'contain: paint'?`); > + } else { > + todo(false, `'display: ${dispVal}' is not known to this test.`); Let's drop this final "not known to this test" check and the "noChange" array, and just assume all display values are noChange unless otherwise specified here. (Right now, this would require any future 'display' value to be explicitly added to this test. That's overhead that's not really necessary; any new display value will almost certainly be its own formatting context, and hence be in category "noChange". So it's extremely unlikely that they'll be an "interesting" case that'll be worth considering in this test. So, no point in forcing the developer adding the new display value to have to grok and edit this test just to add their new display value -- it should be enough for them to just update property_database.js, and then this test will automagically pick up the new value & make a reasonable assumption about it.)
Reftest changes look good, so I think tests patch will be r+ once the mochitest is updated.
Attached patch ContainPaintTest (obsolete) — — Splinter Review
I'd forgotten about the mochitest and hadn't cleaned it up. I think this addresses everything?
Attachment #8636241 - Attachment is obsolete: true
Attachment #8636241 - Flags: review?(dholbert)
Attachment #8636307 - Flags: review?(dholbert)
Comment on attachment 8636307 [details] [diff] [review] ContainPaintTest >+++ b/layout/style/test/test_contain_formatting_context.html >@@ -0,0 +1,40 @@ >+<!DOCTYPE HTML> >+<html> >+<!-- >+https://bugzilla.mozilla.org/show_bug.cgi?id=1179349 >+--> >+<head> Probably best to link to use this bug's bug number here. ^^ (so 1170781, not 1179349) (This mochitest is related to both bugs, but someone looking for where the test was added would be confused if they visited bug 1179349 & saw no commit that added this test.) r=me with that changed. Thanks!
Attachment #8636307 - Flags: review?(dholbert) → review+
Attached patch ContainPaintTest [r=dholbert] (obsolete) — — Splinter Review
That makes sense. Done.
Attachment #8636307 - Attachment is obsolete: true
Attachment #8636324 - Flags: review+
Attached patch ContainPaint [r=dholbert] (obsolete) — — Splinter Review
The CSSWG clarified on Wednesday that the computed value rules between 'overflow-x' and 'overflow-y' apply before 'contain: paint'. This version of the patch is updated to adhere to this clarification. I'm assuming it's fine to leave this r+.
Attachment #8636268 - Attachment is obsolete: true
Attachment #8644972 - Flags: review+
Attached patch ContainPaintTest [r=dholbert] (obsolete) — — Splinter Review
This updates changes the tests to test the 'contain: paint' behavior in the way clarified by the CSSWG. I'm assuming it's fine to leave this r+.
Attachment #8636324 - Attachment is obsolete: true
Attachment #8644973 - Flags: review+
The changes to this patch are basically just due to bitrot. dholbert, do you want to review them again? Otherwise I'll mark them checkin-needed.
Attachment #8644972 - Attachment is obsolete: true
Flags: needinfo?(dholbert)
Attachment #8685092 - Flags: review+
This patch is also an update due to bitrot. This and the above have been successfully tested at https://treeherder.mozilla.org/#/jobs?repo=try&revision=136c39e6ff5d.
Attachment #8644973 - Attachment is obsolete: true
Attachment #8685093 - Flags: review+
(In reply to Kyle Zentner from comment #51) > dholbert, do you want to review them again? Otherwise I'll mark them > checkin-needed. As a qucik sanity-check, I compared the patches in "meld" against earlier versions of the patches here. Changes look good, and Try run looks good (there's orange, but it all looks intermittent and/or unrelated to this bug.) I'll go ahead & mark this as "checkin-needed" (given that you're ready to do so), to remove one bugmail/bugzilla-roundtrip here. For future reference, you should add "r=dholbert" in the commit message (in the patch file itself) when posting final patch versions -- or even early patch versions, depending on your level of optimism :). Sheriffs can add it for you when triaging checkin-needed bugs [as they'll hopefully do here], but it's nice to give them one fewer thing that they need to worry about.
Flags: needinfo?(dholbert)
Keywords: checkin-needed
Comment on attachment 8685092 [details] [diff] [review] ContainPaint [r=dholbert] [explicitly setting "r+" to make it easier for sheriffs to see that these patches are definitely good to go]
https://hg.mozilla.org/mozilla-central/rev/746b38b755df https://hg.mozilla.org/mozilla-central/rev/4fed58b6e141
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Do we have bugs tracking the graphics implications of this (or are there none)? I'm thinking specifically of position:fixed working with async scrolling correctly in a 'contain: paint'-backed layer (but perhaps there are others I've not thought of).
We don't have any bugs tracking the graphics implications yet. There might be some implications. I'm just not familiar enough with all the details to know. However, since the current specification calls for most of the implementation being equivalent to setting 'overflow: clip', I suspect there are none. However, until 1184248 is fixed, we actually implement it with 'overflow: -moz-hidden-unscrollable', which might have various issues. I believe the specific issue you raised shouldn't be a problem because the spec says: > Giving an element paint containment has the following effects: > ... > 2. The element must act as a containing block for absolutely positioned and fixed positioned descendants.
Depends on: 1280183
Alias: css-contain-paint
Blocks: 1465250
Kyle, I glanced at your tests and reftests in attachment 8644973 [details] [diff] [review] . 1- Does the rule body { margin: 0; } serve a purpose in the tests? I think such rule is not being part of any test, is not necessary and can safely be removed everywhere. Also, I think body {margin: 0;} should or would have an unwanted side effect in layout/reftests/w3c-css/submitted/contain/contain-paint-clip-005.html . If the resulting layout is supposed to *_not display_* a bullet, then I believe the test *_must not_* use body {margin: 0;} 2- Maybe 1 test could use better descriptive id attributes or classes (instead of "a", "b" and "c"); the tests do not have any text assert and do not use a pass-fail-conditions sentence but overall they are basic ones. 3- <title>CSS Test: ... can be improved with <title>CSS Containment Test: ... 4- There is no <div class="c"> in /layout/reftests/w3c-css/submitted/contain/contain-paint-clip-001.html so ".c {..." rule can be safely removed.
Hi Gérard! Thanks for the feedback, though I'm not sure Kyle is actively watching bugzilla (nor has he probably thought about this for years :)), so I'll reply on his behalf. Yusuf (CC'able as ":yusuf") is working on getting our contain:paint impl fixed up & shippable, so he's probably the best person to direct questions to at this point about "contain:paint" in Firefox. (Yusuf, I'm tagging you to follow up on 2 clearly-worth-fixing things here; see my #followup tags below, please file a followup to fix those.) (In reply to Gérard Talbot from comment #60) > 1- Does the rule > body { > margin: 0; > } > serve a purpose in the tests? I think such rule is not being part of any > test, is not necessary and can safely be removed everywhere. I think you're right here in general (I think it doesn't serve a purpose), though I'm not sure it matters much in general. I've seen some mozilla developers use "body{margin:0}" as boilerplate in tests to make tests marginally (pun!) more predictable/debuggable -- e.g. so that elements at the top-left of the HTML really end up at 0,0. Anyway, to the extent that it breaks a test somehow, I agree we should remove it [see below], but I'm not really moved to care about it beyond that. (But feel free to file bugs or submit patches for instances of this if you disagree.) > Also, I think body {margin: 0;} should or would have an unwanted side effect > in > layout/reftests/w3c-css/submitted/contain/contain-paint-clip-005.html . If > the resulting layout is supposed to *_not display_* a bullet, then I believe > the test *_must not_* use body {margin: 0;} That seems like a good observation. Yusuf, would you mind investigating and fixing up this test? (probably in a helper bug blocking this one) (#followup) > 2- Maybe 1 test could use better descriptive id attributes or classes > (instead of "a", "b" and "c"); the tests do not have any text assert and do > not use a pass-fail-conditions sentence but overall they are basic ones. Agreed in spirit; patches accepted? :) (If this is just feedback/advice: I quite agree. If you're suggesting that tests actively need fixing, please file a bug w/ links to the particular tests in question, and perhaps :yusuf or I can take a look.) > 3- <title>CSS Test: ... > can be improved with > <title>CSS Containment Test: ... I think this "CSS Test:" title was part of some suggested test boilerplate on testthewebforward.org or a similar reftest guide, back when these tests were written (in 2015). I'm mixed on whether I agree with your suggestion, because "Containment" is a long word, so adding "Containment" at the beginning of the <title> makes it more likely that more-important details later on in the title will be clipped/ellipsized in the browser titlebar. As the title mentions contain/containment somewhere, I don't think it's particularly useful to have "Containment" duplicated at the start (particularly given that it's also mentioned in the test name & directory, presumably.) > 4- There is no <div class="c"> in > /layout/reftests/w3c-css/submitted/contain/contain-paint-clip-001.html > so ".c {..." rule can be safely removed. :yusuf, would you mind fixing this as well? (sharing the same helper-bug as the other issue if you like) (#followup)
Flags: needinfo?(ysermet)
Thank you all for your comments and suggestions! I'll make sure to address these as soon as possible.
(In reply to Daniel Holbert [:dholbert] from comment #61) > Hi Gérard! Thanks for the feedback, I have been asked to review (and improve if needed) contain tests in http://test.csswg.org/suites/css-contain-1_dev/nightly-unstable/ anyway. So, I was bound (destined) to review Mozilla's own css contain tests layout/reftests/w3c-css/submitted/contain/ eventually. > though I'm not sure Kyle is actively > watching bugzilla Kyle's email address was in the sending list when I hit the Save Changes button. > (nor has he probably thought about this for years :)), so > I'll reply on his behalf. Yusuf (CC'able as ":yusuf") is working on getting > our contain:paint impl fixed up & shippable, so he's probably the best > person to direct questions to at this point about "contain:paint" in Firefox. > > (Yusuf, I'm tagging you to follow up on 2 clearly-worth-fixing things here; > see my #followup tags below, please file a followup to fix those.) > > (In reply to Gérard Talbot from comment #60) > > 1- Does the rule > > body { > > margin: 0; > > } > > serve a purpose in the tests? I think such rule is not being part of any > > test, is not necessary and can safely be removed everywhere. > > I think you're right here in general (I think it doesn't serve a purpose), > though I'm not sure it matters much in general. > It does not matter much in general. But there is a widely acknowledged requirement regarding tests submitted to web-platform-tests.org : to avoid extraneous elements, to have code focus on the target of tests. " For all tests extraneous elements on the page should be avoided so it is clear what is part of the test " https://web-platform-tests.org/writing-tests/general-guidelines.html#be-short " The test contains no extraneous content. " https://wiki.csswg.org/test/css2.1/review-checklist#test-design > I've seen some mozilla developers use "body{margin:0}" as boilerplate in > tests to make tests marginally (pun!) more predictable/debuggable -- e.g. so > that elements at the top-left of the HTML really end up at 0,0. Anyway, to > the extent that it breaks a test somehow, I agree we should remove it [see > below], but I'm not really moved to care about it beyond that. (But feel > free to file bugs or submit patches for instances of this if you disagree.) > > > Also, I think body {margin: 0;} should or would have an unwanted side effect > > in > > layout/reftests/w3c-css/submitted/contain/contain-paint-clip-005.html . If > > the resulting layout is supposed to *_not display_* a bullet, then I believe > > the test *_must not_* use body {margin: 0;} I am now not entirely sure of what I wrote above; I would need time to investigate/scrutinize to be sure, including with latest MS-Edge. But I know in some cases [1], the list marker requires an horizontal area to be displayed. Margin applying to list (<ul> and <li>) varies among mainstream browsers [3]. [1] http://test.csswg.org/suites/css2.1/20110323/html4/after-content-display-003.htm http://test.csswg.org/suites/css2.1/20110323/html4/universal-selector-005.htm [2] item 3 in "28 proposals to improve testcase writing guidelines" http://lists.w3.org/Archives/Public/public-css-testsuite/2011Dec/0004.html [3] Consistent list indentation (among browsers) https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Lists_and_Counters/Consistent_list_indentation#Finding_Consistency > That seems like a good observation. Yusuf, would you mind investigating and > fixing up this test? (probably in a helper bug blocking this one) (#followup) > > > 2- Maybe 1 test could use better descriptive id attributes or classes > > (instead of "a", "b" and "c"); the tests do not have any text assert and do > > not use a pass-fail-conditions sentence but overall they are basic ones. > > Agreed in spirit; patches accepted? :) > > (If this is just feedback/advice: I quite agree. If you're suggesting that > tests actively need fixing, please file a bug w/ links to the particular > tests in question, and perhaps :yusuf or I can take a look.) Only 1 test could use more meaningful id attributes or classes. Kyle's tests are overall basic ones. item 18 in http://lists.w3.org/Archives/Public/public-css-testsuite/2011Dec/0004.html > > 3- <title>CSS Test: ... > > can be improved with > > <title>CSS Containment Test: ... > > I think this "CSS Test:" title was part of some suggested test boilerplate > on testthewebforward.org or a similar reftest guide, back when these tests > were written (in 2015). I'm mixed on whether I agree with your suggestion, > because "Containment" is a long word, so adding "Containment" at the > beginning of the <title> makes it more likely that more-important details > later on in the title will be clipped/ellipsized in the browser titlebar. more-important details about a test should preferably go into a test assertion text [4] or into <!-- comments --> inside a test. [4] https://web-platform-tests.org/writing-tests/css-metadata.html#test-assertions [5] "The title is descriptive but not too wordy." https://web-platform-tests.org/reviewing-tests/checklist.html#all-tests "Title is unique and descriptive but not wordy. (It should make sense in the test suite's table of contents.)" https://wiki.csswg.org/test/css2.1/review-checklist#metadata > As the title mentions contain/containment somewhere, I don't think it's > particularly useful to have "Containment" duplicated at the start > (particularly given that it's also mentioned in the test name & directory, > presumably.) I am no longer sure if the module name is particularly useful in the <title> text; there has been a lot of changes since 2010. I have always included the module name in the <title>. Right now, all contain tests in http://test.csswg.org/suites/css-contain-1_dev/nightly-unstable/html/chapter-3.htm use <title>CSS-contain test: ... or <title>CSS Containment Test: ...
(In reply to Gérard Talbot from comment #63) > > > Also, I think body {margin: 0;} should or would have an unwanted side effect > > > in > > > layout/reftests/w3c-css/submitted/contain/contain-paint-clip-005.html . If > > > the resulting layout is supposed to *_not display_* a bullet, then I believe > > > the test *_must not_* use body {margin: 0;} > > I am now not entirely sure of what I wrote above; I would need time to > investigate/scrutinize to be sure, including with latest MS-Edge. But I know > in some cases [1], the list marker requires an horizontal area to be > displayed. Margin applying to list (<ul> and <li>) varies among mainstream > browsers [3]. > > [1] > http://test.csswg.org/suites/css2.1/20110323/html4/after-content-display-003. > htm > http://test.csswg.org/suites/css2.1/20110323/html4/universal-selector-005.htm > [2] item 3 in "28 proposals to improve testcase writing guidelines" > http://lists.w3.org/Archives/Public/public-css-testsuite/2011Dec/0004.html > [3] Consistent list indentation (among browsers) > https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Lists_and_Counters/ > Consistent_list_indentation#Finding_Consistency I tried looking into what may cause a problem with the current test, which uses body margin: 0, and checked out the current default css values for html elements in different browsers (edge, ie explorer, chrome, opera). The exact problematic scenario is not clear to me. But, if we want to make sure that we are independent from any possibilities of having different layout-related effects of list elements in different browsers, I can just add margin: 0 to the ul element, along with the padding: 0, as [3] suggests. > > > Also, I think body {margin: 0;} should or would have an unwanted side effect > > > in > > > layout/reftests/w3c-css/submitted/contain/contain-paint-clip-005.html . If > > > the resulting layout is supposed to *_not display_* a bullet, then I believe > > > the test *_must not_* use body {margin: 0;} > > I am now not entirely sure of what I wrote above; Of course, I may be wrong. But my guess is that you were saying if we wanted to effectively test if the bullet (which is outside of the green box) will disappear due to the containment, using body margin: 0 would make this test ineffective, since the bullet will be outside of the window anyway. But, since we are using an additional margin (25px) for the text (li element), we are making sure that the bullet is inside the window, and disappears due to containment. > But there is a widely acknowledged requirement regarding tests submitted to web-platform-tests.org : to avoid extraneous elements, to have code focus on the target of tests. Removed all body margins. >> 4- There is no <div class="c"> in >> /layout/reftests/w3c-css/submitted/contain/contain-paint-clip-001.html >> so ".c {..." rule can be safely removed. >:yusuf, would you mind fixing this as well? (sharing the same helper-bug as the other issue if you like) (#followup) Removed it. >>> 2- Maybe 1 test could use better descriptive id attributes or classes >>> (instead of "a", "b" and "c"); the tests do not have any text assert and do >>> not use a pass-fail-conditions sentence but overall they are basic ones. >> >> Agreed in spirit; patches accepted? :) >> >> (If this is just feedback/advice: I quite agree. If you're suggesting that >> tests actively need fixing, please file a bug w/ links to the particular >> tests in question, and perhaps :yusuf or I can take a look.) > Only 1 test could use more meaningful id attributes or classes. Kyle's tests are overall basic ones. item 18 in > http://lists.w3.org/Archives/Public/public-css-testsuite/2011Dec/0004.html There are a few tests that uses the notation (e.g a,b) discouraged in above link. Do you want me to update those now, or wait for a new bug filed for it? >> As the title mentions contain/containment somewhere, I don't think it's >> particularly useful to have "Containment" duplicated at the start >> (particularly given that it's also mentioned in the test name & directory, >> presumably.) >I am no longer sure if the module name is particularly useful in the <title> text; there has been a lot of changes since 2010. >I have always included the module name in the <title>. Right now, all contain tests in >http://test.csswg.org/suites/css-contain-1_dev/nightly-unstable/html/chapter-3.htm >use ><title>CSS-contain test: ... >or ><title>CSS Containment Test: ... Waiting for a consensus on the title name. IMHO, though I agree we don't need to make the title longer by specifying the containment since it's already mentioned in the directory and inside the test, it might be better to be consistent with other tests. We could update those as well, of course. In the mean time, I'll post a new bug for the fixes mentioned here as dholbert advised.
Flags: needinfo?(ysermet)
Depends on: 1468268
(In reply to Yusuf Sermet [:yusuf] from comment #64) [snipped] Sorry for the delay. I wish I could have replied sooner but could not (..skipping a story here..). > my guess is that you were saying if we wanted > to effectively test if the bullet (which is outside of the green box) will > disappear due to the containment, using body margin: 0 would make this test > ineffective, since the bullet will be outside of the window anyway. Yes. That is exactly what I am afraid of. The test reviewer has to check that the bullet, in normal conditions, would be inside viewport in all mainstream browsers if the rule body {margin: 0;} remains in layout/reftests/w3c-css/submitted/contain/contain-paint-clip-005.html . > > But there is a widely acknowledged requirement regarding tests submitted to web-platform-tests.org : to avoid extraneous elements, to have code focus on the target of tests. > > Removed all body margins. Good! That is best IMO. > >>> 2- Maybe 1 test could use better descriptive id attributes or classes > >>> (instead of "a", "b" and "c"); the tests do not have any text assert and do > >>> not use a pass-fail-conditions sentence but overall they are basic ones. > >> > >> Agreed in spirit; patches accepted? :) > >> > >> (If this is just feedback/advice: I quite agree. If you're suggesting that > >> tests actively need fixing, please file a bug w/ links to the particular > >> tests in question, and perhaps :yusuf or I can take a look.) > > > Only 1 test could use more meaningful id attributes or classes. Kyle's tests are overall basic ones. item 18 in > > http://lists.w3.org/Archives/Public/public-css-testsuite/2011Dec/0004.html > > There are a few tests that uses the notation (e.g a,b) discouraged in above > link. Do you want me to update those now, or wait for a new bug filed for it? You can leave the "a", "b" id attributes (or classes) in Kyle's tests since Kyle's tests are rather basic. I am mentionning though that too many unsemantic, undescriptive id attributes or classes can make tests difficult to understand. " It's helpful to people trying to understand the test if you use meaningful class and ID names " https://wiki.csswg.org/test/format#body-content > Waiting for a consensus on the title name. There is no consensus or strong unambiguous directive. > IMHO, though I agree we don't > need to make the title longer by specifying the containment since it's > already mentioned in the directory and inside the test, it might be better > to be consistent with other tests. We could update those as well, of course. I am for consistency with other tests. But it's not a mistake if you leave the current <title> text as they are in Kyle's tests.
(In reply to Gérard Talbot from comment #65) > Yes. That is exactly what I am afraid of It should all be good right now. For everything else, sounds great, thanks!
Depends on: 1553548
Depends on: 1563059
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: