Closed Bug 1476495 Opened 6 years ago Closed 6 years ago

"contain: layout" and ink overflow

Categories

(Core :: Layout, defect, P3)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: bugzilla, Assigned: morgan)

References

Details

(Keywords: testcase)

Attachments

(1 file)

Bug 1476495 - Treat overflow in contain:layout elements as ink overflow.
(deleted), text/x-review-board-request
dholbert
: review+
Details
Blocks: css-contain-layout
Keywords: testcase
Priority: -- → P3
Yeah, "contain:layout" doesn't have any rendering effect in Firefox yet (even with the "contain" pref flipped on). Thanks for the bug & the testcases! This bug basically covers bug 1463593 comment 2 part 3 (see discussion of ConsiderChildOverflow there). Tentatively assigning to Morgan to work on next/soon (I think she's in the middle of some contain:size stuff at the moment).
Assignee: nobody → mreschenberg
Also: http://www.gtalbot.org/BrowserBugsSection/CSS3Contain/contain-layout-ink-overflow-017.html http://www.gtalbot.org/BrowserBugsSection/CSS3Contain/contain-layout-ink-overflow-018.html - - - - - I want and am planning to eventually submit those 6 tests and their associated reference files to the W3C CSS3 Containment Test suite. Their filenames should be the same as now.
(In reply to Gérard Talbot from comment #2) > I want and am planning to eventually submit those 6 tests and their > associated reference files to the W3C CSS3 Containment Test suite. Their > filenames should be the same as now. Thanks, that's good to hear! Do you have a rough timeline on when those will be submitted to the W3C testsuite & be in web platform tests? (That'll help us decide whether Morgan should include automated tests [& how comprehensive/how many] when she lands her patch for this bug.)
Flags: needinfo?(bugzilla)
Timeline: it should be soon; most likely before august 10th. As soon as I know how to submit tests with GitHub, I will submit them. It is also my intent to submit all the contain: [layout | paint | size | strict | content] tests that I made and will make.
Flags: needinfo?(bugzilla)
OK, thanks! We'll want to include a few tests ourselves here, then, to provide some test coverage until those ones land and are synchronized around. (I think Morgan's got a fix nearly ready here.)
Attachment #8994313 - Flags: review?(dholbert)
Comment on attachment 8994313 [details] Bug 1476495 - Treat overflow in contain:layout elements as ink overflow. https://reviewboard.mozilla.org/r/258908/#review265836 ::: layout/generic/nsFrame.h:629 (Diff revision 1) > - // contain: paint, which we should interpret as -moz-hidden-unscrollable > - // by default. > - if (aDisp->IsContainPaint()) { > + // contain: paint, contain:layout which we should interpret > + // as -moz-hidden-unscrollable by default. > + if (aDisp->IsContainPaint() || aDisp->IsContainLayout()) { This isn't quite correct -- this will treat overflow as not-painted-at-all (rather than as ink overflow). Simple testcase using -moz-hidden-unscrollable directly -- we'd like contain:layout to paint all of the 'a' characters here, but -moz-hidden-unscrollable does not (and hence isn't a good replacement for contain:layout): data:text/html,<div style="width:50px; border: 1px solid black; overflow: -moz-hidden-unscrollable">aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa ::: layout/reftests/w3c-css/submitted/contain/contain-layout-overflow-001.html:5 (Diff revision 1) > +<!DOCTYPE HTML> > +<html> > +<head> > + <meta charset="utf-8"> > + <title>CSS Test: 'contain: size' on multicol elements should cause them to be sized and baseline-aligned as if they had no contents.</title> title needs an update here. ::: layout/reftests/w3c-css/submitted/contain/contain-layout-overflow-001.html:30 (Diff revision 1) > +<body> > + <!--CSS Test: Elements that overflow and have contain:layout applied should not change the scrollable overflow regions of their parent --> In this testcase, please include something like the following: - an "overflow:auto" div of intermediate width/height (say, 100x100) - ...which has a child div that is tiny (say, 50x50) with a visible border and "contain:layout" - ...which has a child div that is huge (say, 200x200) and a visible background color. That should result in the innermost thing's background being painted all the way up to the border (scrollbars) of the scrollable element (overflowing its own "contain:layout" parent, but in a way that doesn't create scrollable overflow, as can be seen by the fact that no scrollbars end up being drawn) And in the reference case, you could just have the outermost two divs without any need to set "overflow" at all, and you can put the colorful background on the outer div instead. (This sort of testcase would catch the issue I brought up with emulating -moz-hidden-unscrollable.)
Attachment #8994313 - Flags: review?(dholbert) → review-
(In reply to Daniel Holbert [:dholbert] from comment #1) > This bug basically covers bug 1463593 comment 2 part 3 (see discussion of > ConsiderChildOverflow there). Quoting for this bug: ========= This would probably be implemented via adding some subtlety to nsFrame::ConsiderChildOverflow. (Right now this method treats scrollable overflow on the child as scrollable overflow on the parent, and ink-overflow on the child as ink-overflow on the parent. But on a box with layout-containment, we'll want to union *both* categories (from that box's kids) into our ink overflow. ========= (I think this ^^ is what we should probably be doing here, instead of using -moz-hidden-unscrollable.)
Comment on attachment 8994313 [details] Nit: line 7: <link rel="help" href="https://drafts.csswg.org/css-contain/#containment-size"> should be instead <link rel="help" href="https://drafts.csswg.org/css-contain/#containment-layout">
> In this testcase, I think it should be in a distinct, separate, additional test.. > please include something like the following: > - an "overflow:auto" div of intermediate width/height (say, 100x100) > - ...which has a child div that is tiny (say, 50x50) with a visible border > and "contain:layout" > - ...which has a child div that is huge (say, 200x200) and a visible > background color. > > That should result in the innermost thing's background being painted all the > way up to the border (scrollbars) of the scrollable element (overflowing its > own "contain:layout" parent, but in a way that doesn't create scrollable > overflow, as can be seen by the fact that no scrollbars end up being drawn) Daniel, I got curious and wanted to figure out how your description would work (and could not) and so I created the configuration/code scenario you described into: http://www.gtalbot.org/BrowserBugsSection/CSS3Contain/contain-layout-overflow-006-DHolbertModif.html > And in the reference case, you could just have the outermost two divs > without any need to set "overflow" at all, and you can put the colorful > background on the outer div instead. http://www.gtalbot.org/BrowserBugsSection/CSS3Contain/contain-layout-overflow-006-DHolbertModif-ref.html
(In reply to Gérard Talbot from comment #10) > > In this testcase, > > I think it should be in a distinct, separate, additional test.. Either way. I think it's the main thing to test here, so I think it should be in our "main" test. :) If we have additional tests, that's fine. > I created the configuration/code scenario you described Nice! Thanks. Incidentally I just noticed one thing I'd misstated in my description (if you'd like to correct the HTML-comment-copy of my description in the HTML comment within your test): # That should result in the innermost thing's background being # painted all the way up to the border (scrollbars) I shouldn't have mentioned "(scrollbars)" there. The point is that there shouldn't be *any* scrollbars. :)
Comment on attachment 8994313 [details] Bug 1476495 - Treat overflow in contain:layout elements as ink overflow. https://reviewboard.mozilla.org/r/258908/#review265836 > In this testcase, please include something like the following: > - an "overflow:auto" div of intermediate width/height (say, 100x100) > - ...which has a child div that is tiny (say, 50x50) with a visible border and "contain:layout" > - ...which has a child div that is huge (say, 200x200) and a visible background color. > > That should result in the innermost thing's background being painted all the way up to the border (scrollbars) of the scrollable element (overflowing its own "contain:layout" parent, but in a way that doesn't create scrollable overflow, as can be seen by the fact that no scrollbars end up being drawn) > > And in the reference case, you could just have the outermost two divs without any need to set "overflow" at all, and you can put the colorful background on the outer div instead. > > > (This sort of testcase would catch the issue I brought up with emulating -moz-hidden-unscrollable.) Hmm I'm not sure if I 100% understand the cases here. In the case we have a small, layout-contained object with a visable border inside the larger box, wouldn't the border still get painted as long as the total size is smaller than the outer box, or is the border considered 'overflow' and shouldn't be painted?
No, the border isn't considered 'overflow'. The overflow here is from the larger innermost child here. That child would normally create scrollable overflow (which creates scrollbars via its overflow:auto grandparent). But since the child is layout-contained (its parent has contain:layout), it instead should just create ink overflow which doesn't cause scrollbars to show up on the grandparent. (Compare Gerard's testcase between Chrome & current Firefox to see what I mean -- Chrome gets this correct, I think, and we want to match what they're doing.)
(In reply to Daniel Holbert [:dholbert] from comment #13) > (Compare Gerard's testcase between Chrome & current Firefox to see what I > mean -- Chrome gets this correct, I think, and we want to match what they're > doing.) In my copy of chrome (67.0.3396.99) the case Gerard linked ( http://www.gtalbot.org/BrowserBugsSection/CSS3Contain/contain-layout-overflow-006-DHolbertModif.html, and the earlier ones he's written) all render with scrolllbars and extra content. Maybe this was recently fixed, if you're running dev-edition?
Flags: needinfo?(dholbert)
Sorry for not being specific -- I was referring to this testcase: http://www.gtalbot.org/BrowserBugsSection/CSS3Contain/contain-layout-overflow-006-DHolbertModif.html I am running Chrome Dev Edition (69.0.3493.3) and that ^^ testcase shows no scrollbars for me.
Flags: needinfo?(dholbert)
Comment on attachment 8994313 [details] Bug 1476495 - Treat overflow in contain:layout elements as ink overflow. https://reviewboard.mozilla.org/r/258906/#review266184 ::: layout/generic/nsFrame.cpp:9683 (Diff revision 2) > > void > nsFrame::ConsiderChildOverflow(nsOverflowAreas& aOverflowAreas, > nsIFrame* aChildFrame) > { > + if (aChildFrame->StyleDisplay()->IsContainLayout()) { I think this is what we want to check here (ie. if the child has layout containment, not just StyleDisplay()->IsContainLayout()), but in the test cases I've written, this condition doesn't get triggered. Printing 'StyleDisplay()->mContain' and 'aChildFrame->StyleDispaly()->mContain' show the contain flags empty in both cases. I set a breakpoint inside the body also, but it doesn't get hit. To check if it was a parsing issue, I also tried changing the test to have 'contain:size;' and the if condition to '...IsContainSize()' as well, but that doesn't get tripped either. Am I using the wrong accessor for IsContainLayout()? Is it possible the style attributes aren't set when this gets called? ::: layout/reftests/w3c-css/submitted/contain/contain-layout-overflow-001-ref.html:9 (Diff revision 2) > + <meta charset="utf-8"> > + <title>CSS Reftest Reference</title> > + <link rel="author" title="Morgan Rae Reschenberg" href="mailto:mreschenberg@berkeley.edu"> > + <style> > + .contain { > + contain:layout; removed ::: layout/reftests/w3c-css/submitted/contain/contain-layout-overflow-001.html:11 (Diff revision 2) > + <link rel="author" title="Morgan Rae Reschenberg" href="mailto:mreschenberg@berkeley.edu"> > + <link rel="help" href="https://drafts.csswg.org/css-contain/#containment-layout"> > + <link rel="match" href="contain-layout-overflow-001-ref.html"> > + <style> > + .contain { > + contain:size; s/size/layout (artifact from testing comment above)
Flags: needinfo?(dholbert)
(In reply to Morgan Reschenberg [:morgan] from comment #17) > > void > > nsFrame::ConsiderChildOverflow(nsOverflowAreas& aOverflowAreas, > > nsIFrame* aChildFrame) > > { > > + if (aChildFrame->StyleDisplay()->IsContainLayout()) { > > I think this is what we want to check here (ie. if the child has layout > containment, not just StyleDisplay()->IsContainLayout()), but in the test > cases I've written, this condition doesn't get triggered. I posted a reply in MozReview, but it seems to not have produced a bugzilla comment, so I'll repost it here as well: ======== I was thinking you'd want to check if *this frame*'s StyleDisplay()->IsContainLayout() is true... i.e. "if (StyleDisplay()->IsContainLayout())" here. The idea being: for the layout-contained frame, its overflow comes from its children (whether they be text frames or larger containers or whatever). And normally we'd preserve the "scrollable vs painted" distinction when merging our children's overflow areas into our own, but if we're layout-contained, then instead we want to treat things that our children believe to be "scrollable overflow" as our own painted overflow. This might not be sufficient to fix the issue you're describing though -- there may be other spots that matter here too. In particular, maybe the function "ConsiderBlockEndEdgeOfChildren" in block reflow needs a tweak to consider whether the block in question has contain:layout or not. (We don't seem to have a pointer to the block frame there right now, so we might need to pass it (or its StyleDisplay() pointer) in as an arg to that function.) ===
Flags: needinfo?(dholbert)
(In reply to Daniel Holbert [:dholbert] from comment #11) > I just noticed one thing I'd misstated in my > description (if you'd like to correct the HTML-comment-copy of my > description in the HTML comment within your test): > # That should result in the innermost thing's background being > # painted all the way up to the border (scrollbars) > > I shouldn't have mentioned "(scrollbars)" there. The point is that there > shouldn't be *any* scrollbars. :) Daniel, Maybe you should be and deserve to be the author of that test. And I would be happy to approve it once at W3C testsuite and in web platform tests. - - - - Regarding the correctness of the HTML-comment... In this testcase: - an "overflow:auto" div of intermediate width/height (say, 100x100) - ...which has a child div that is tiny (say, 50x50) with a visible border and "contain:layout" - ...which has a child div that is huge (say, 200x200) and a visible background color. That should result in the innermost thing's background being painted all the way up to the border (scrollbars) of the scrollable element (overflowing its own "contain:layout" parent, but in a way that doesn't create scrollable overflow, as can be seen by the fact that no scrollbars end up being drawn) could become (mere proposal here) In this testcase, we have: - an "overflow:auto" div (identified as div#overflow-auto-grand-parent) of intermediate width/height (say, 100x100) - ...which has a child div (identified as div#contain-tiny-parent) that is tiny (say, 50x50) with a visible border and "contain:layout" - ...which has a child div (identified as div#huge-child)that is huge (say, 200x200) and a visible background color. That should result in div#huge-child's background paint over div#contain-tiny-parent's border (div#huge-child overflows its own "contain:layout" parent, but in a way that doesn't create scrollable overflow, as can be seen by the fact that no scrollbars end up being drawn: the scrollbars are covered, painted over.)
(In reply to Gérard Talbot from comment #19) > Maybe you should be and deserve to be the author of that test. And I would > be happy to approve it once at W3C testsuite and in web platform tests. I'm not worried about attribution / deserving :) I am juggling too many things at the moment to submit it myself, so I'll defer to you & Morgan to write the real tests here, and I'll just stick to suggesting / critiquing. > [...] could become (mere proposal here) > > In this testcase, we have: > - an "overflow:auto" div (identified as div#overflow-auto-grand-parent) of > intermediate width/height (say, 100x100) > - ...which has a child div (identified as div#contain-tiny-parent) that is > tiny (say, 50x50) with a visible border and "contain:layout" > - ...which has a child div (identified as div#huge-child)that is huge (say, > 200x200) and a visible background color. > > That should result in div#huge-child's background paint over > div#contain-tiny-parent's border (div#huge-child overflows > its own "contain:layout" parent, but in a way that doesn't > create scrollable overflow, as can be seen by the fact > that no scrollbars end up being drawn: the scrollbars > are covered, painted over.) That description sounds good!
> the scrollbars are covered, painted over. Argh.. That's incorrect. There are no scrollbars. No way to reach overflowed content.
I just tweaked the test, mainly the comments accordingly, added a link rel="help" for ink overflow. > I'll defer to you & Morgan to > write the real tests here, and I'll just stick to suggesting / critiquing. Morgan, you can use the test http://www.gtalbot.org/BrowserBugsSection/CSS3Contain/contain-layout-overflow-006-DHolbertModif.html as you wish.
Comment on attachment 8994313 [details] Bug 1476495 - Treat overflow in contain:layout elements as ink overflow. https://reviewboard.mozilla.org/r/258908/#review266424 (Marking patch r- for now, per feedback in comment 18.)
Attachment #8994313 - Flags: review?(dholbert) → review-
Morgan, I adjusted and adapted your contain-layout-overflow-001.html test (from attachment 8994313 [details]) with the explanations from Daniel, split the test in 2 to cover both 'overflow: scroll' and 'overflow: auto' cases, added my own tweaks, adjustments (comments, scrollLeft, scrollTop javascript stuff to make reftests reliable, trustworthy over at wpt) and created 2 reference files: http://www.gtalbot.org/BrowserBugsSection/CSS3Contain/contain-layout-ink-overflow-019.html http://www.gtalbot.org/BrowserBugsSection/CSS3Contain/contain-layout-ink-overflow-019-ref.html http://www.gtalbot.org/BrowserBugsSection/CSS3Contain/contain-layout-ink-overflow-020.html http://www.gtalbot.org/BrowserBugsSection/CSS3Contain/contain-layout-ink-overflow-020-ref.html I can add your name as co-author to the 2 tests. No problem.
(In reply to Gérard Talbot from comment #24) > Morgan, > > I adjusted and adapted your contain-layout-overflow-001.html test (from > attachment 8994313 [details]) with the explanations from Daniel, split the > test in 2 to cover both 'overflow: scroll' and 'overflow: auto' cases, added > my own tweaks, adjustments (comments, scrollLeft, scrollTop javascript stuff > to make reftests reliable, trustworthy over at wpt) and created 2 reference > files: > > http://www.gtalbot.org/BrowserBugsSection/CSS3Contain/contain-layout-ink- > overflow-019.html > > http://www.gtalbot.org/BrowserBugsSection/CSS3Contain/contain-layout-ink- > overflow-019-ref.html > > http://www.gtalbot.org/BrowserBugsSection/CSS3Contain/contain-layout-ink- > overflow-020.html > > http://www.gtalbot.org/BrowserBugsSection/CSS3Contain/contain-layout-ink- > overflow-020-ref.html > > I can add your name as co-author to the 2 tests. No problem. Thank you for doing that! Sounds good to me.
> Do you have a rough timeline on when those will > be submitted to the W3C testsuite & be in web platform tests? 86 files submitted today; changes approved and the 11 continuous integration checks have passed: https://github.com/web-platform-tests/wpt/pull/12312 10 tests {6 on stacking contexts (contain-paint-stacking-context-034.html to -039) and 4 on size containment (contain-size-022, -024, -026, -028)} have not been submitted because they posed or exposed a situation or condition (respectively 'z-index: auto' related and baseline-alignment related) that the spec did not foresee or that is unknown right now.
Comment on attachment 8994313 [details] Bug 1476495 - Treat overflow in contain:layout elements as ink overflow. https://reviewboard.mozilla.org/r/258906/#review268650 ::: layout/generic/nsFrame.cpp:9686 (Diff revision 3) > + if (StyleDisplay()->IsContainLayout()) { > + // If our child has layout containment, we want to treat all > + // overflow as ink overflow, meaning if we, the parent, have scrollable > + // regions, they should not be affected by our child's overflow. > + // To ensure this, we union the child's scrollable and visual overflow > + // together in a new OverflowAreas struct with no scrollable overflow. This feels hack-y, and maybe a better solution is to union the visual overflows together like I did in nsBlockFrame::ComputeOverflowAreas with the nsRect method.
Comment on attachment 8994313 [details] Bug 1476495 - Treat overflow in contain:layout elements as ink overflow. https://reviewboard.mozilla.org/r/258908/#review268660 ::: layout/generic/nsBlockFrame.cpp:1808 (Diff revision 3) > + if ((aDisplay->IsContainLayout() && otype == eVisualOverflow) || > + !aDisplay->IsContainLayout()) { We don't need the first IsContainLayout call here. It's sufficient to just check: otype == eVisualOverflow ||!aDisplay->IsContainLayout() Or possibly clearer (and equivalent): if (!(otype == eScrollableOverflow && aDisplay->IsContainLayout())) { This applies to all of the checks here. ::: layout/generic/nsBlockFrame.cpp:1810 (Diff revision 3) > + // Layout containment should force all overflow to be ink overflow > + // so we only want to pass overflow from parent to child in the > + // case of visual overflow (which already includes scrollable). > - nsRect& o = aOverflowAreas.Overflow(otype); > + nsRect& o = aOverflowAreas.Overflow(otype); > - o.width = std::max(o.XMost(), aBEndEdgeOfChildren) - o.x; > + o.width = std::max(o.XMost(), aBEndEdgeOfChildren) - o.x; Nits: (1) s/ink overflow/ink (visual) overflow/ to make it clearer that those are the same thing. (2) Second and third lines need a reword -- this function isn't "passing overflow from parent to child". (I think you meant to say "child to parent" -- but setting that ordering typo aside, this function isn't really considering *child overflow* here, but rather the child block-end-edges, for some special cases around margins as far as I can tell based on how this function seems to be called/used.) Let's reword as: "...so if we're layout-contained, we only add our children's block-end edge to the ink (visual) overflow -- not to the scrollable overflow." ::: layout/generic/nsBlockFrame.cpp:1849 (Diff revision 3) > + if ((mComputedStyle->GetPseudo() == nsCSSAnonBoxes::scrolledContent || > + mComputedStyle->GetPseudo() == nsCSSAnonBoxes::buttonContent) && > + mParent->StyleDisplay()->IsContainLayout()) { > + // If we are an anonymous box and our parent has layout containment, > + // we want to pass our parent's style to ConsiderBlockEndEdgeOfChildren > + // to make sure all overflow from the layout contained element is > + // processed as ink overflow. > + aDisplay = mParent->StyleDisplay(); Two issues: (1) Nit: this chunk is over-indented. "// If we are" etc. is 4-space indented but should only be 2-space indented. (2) After some thought, I'm pretty sure you *don't* want a special case for ::buttonContent here. I think you should remove the buttonContent check, and narrow the check (and the code-comment) to be specifically about the "anonymous inner box of a scrollframe". (The ::buttonContent inner frame does hold a button's children, but it's also allowed to be as tall as it likes, regardless of the button's specified height -- so it typically doesn't have any overflow. It just grows to be tall enough to hold all of the children. So: the nsHTMLButtonControlFrame is the wrapper that should (and presumably will) apply layout containment for any children that are too tall / positioned too far down.) ::: layout/generic/nsBlockFrame.cpp:1862 (Diff revision 3) > + if (aDisplay->IsContainLayout()) { > + // If we have layout containment (or, per above, we are an anonymous box > + // and our parent has layout containment), we should only consider our s/an anonymous box/a scrollframe's inner anonymous box/ ::: layout/generic/nsFrame.cpp:9681 (Diff revision 3) > > void > nsFrame::ConsiderChildOverflow(nsOverflowAreas& aOverflowAreas, > nsIFrame* aChildFrame) > { > + if (StyleDisplay()->IsContainLayout()) { Since this is just a generic nsFrame method, this could be called on any sort of box (well, any sort that has children). So: we should be checking whether or not we are an atomic inline-level element here. ::: layout/generic/nsFrame.cpp:9681 (Diff revision 3) > + if (StyleDisplay()->IsContainLayout()) { > + // If our child has layout containment, we want to treat all > + // overflow as ink overflow, meaning if we, the parent, have scrollable "If our child" is wrong here -- we're checking if *we* have layout containment (we're calling |StyleDisplay()| on this frame, not on our child frame). (I wonder if this part needs to consider the are-we-a-scrollframe-inner-anonymous-box-in-which-case-we-don't-want-to-check-our-own-StyleDisplay() scenario, too...)
Attachment #8994313 - Flags: review?(dholbert) → review-
Comment on attachment 8994313 [details] Bug 1476495 - Treat overflow in contain:layout elements as ink overflow. https://reviewboard.mozilla.org/r/258908/#review268660 > Since this is just a generic nsFrame method, this could be called on any sort of box (well, any sort that has children). > > So: we should be checking whether or not we are an atomic inline-level element here. I found a method in TextOverflow.cpp [1] that has comments to indicate it does some checking of atomic, inline-level elements, but I'm not sure why the check it makes is !IsFrameOfType(eLineParticipant) since that feels like it might be text-specific. Looking at the nsIFrame enum [2] I didn't find any types that seemed obvious to check, either. How do I know if a frame is inline-leve atomic? [1] https://searchfox.org/mozilla-central/rev/51268dcbdff0f6f4a5cff7986df0f616efc5bcfd/layout/generic/TextOverflow.cpp#71 [2] https://searchfox.org/mozilla-central/rev/51268dcbdff0f6f4a5cff7986df0f616efc5bcfd/layout/generic/nsIFrame.h#2834
Comment on attachment 8994313 [details] Bug 1476495 - Treat overflow in contain:layout elements as ink overflow. https://reviewboard.mozilla.org/r/258908/#review268660 > I found a method in TextOverflow.cpp [1] that has comments to indicate it does some checking of atomic, inline-level elements, but I'm not sure why the check it makes is !IsFrameOfType(eLineParticipant) since that feels like it might be text-specific. Looking at the nsIFrame enum [2] I didn't find any types that seemed obvious to check, either. How do I know if a frame is inline-leve atomic? > > [1] https://searchfox.org/mozilla-central/rev/51268dcbdff0f6f4a5cff7986df0f616efc5bcfd/layout/generic/TextOverflow.cpp#71 > [2] https://searchfox.org/mozilla-central/rev/51268dcbdff0f6f4a5cff7986df0f616efc5bcfd/layout/generic/nsIFrame.h#2834 (Sorry, I said "atomic inline-level" but I meant "non-atomic inline-level") I believe !IsFrameOfType(eLineParticipant) is precisely what you want to check for here. (For frames that return true from IsFrameOfType(eLineParticipant), we should always behave as if we do not have contain:layout.)
Comment on attachment 8994313 [details] Bug 1476495 - Treat overflow in contain:layout elements as ink overflow. https://reviewboard.mozilla.org/r/258908/#review268660 > (Sorry, I said "atomic inline-level" but I meant "non-atomic inline-level") > > I believe !IsFrameOfType(eLineParticipant) is precisely what you want to check for here. (For frames that return true from IsFrameOfType(eLineParticipant), we should always behave as if we do not have contain:layout.) Ah, and it looks like dbaron is adding "eSupportsContainLayoutAndPaint" in bug 1479859, which is more specific and is probably what we should really be checking here, I guess. Maybe use eLineParticipant for now but add a comment like: // XXX Really should check eSupportsContainLayoutAndPaint from bug 1479859.
Comment on attachment 8994313 [details] Bug 1476495 - Treat overflow in contain:layout elements as ink overflow. https://reviewboard.mozilla.org/r/258908/#review268660 > "If our child" is wrong here -- we're checking if *we* have layout containment (we're calling |StyleDisplay()| on this frame, not on our child frame). > > (I wonder if this part needs to consider the are-we-a-scrollframe-inner-anonymous-box-in-which-case-we-don't-want-to-check-our-own-StyleDisplay() scenario, too...) Hmm I added the same test case as above (mComputedStyle->GetPseudo() == nsCSSAnonBoxes::scrolledContent && mParent->StyleDisplay()->IsContainLayout()) and stepped through with lldb, but unlike the case in nsBlockFrame::ComputeOverflowAreas, it looks like we don't run into the scrollframe anonymous boxes here. Looks like leaving it out, since adding it doesn't change behaviour, might be the right thing to do?
Comment on attachment 8994313 [details] Bug 1476495 - Treat overflow in contain:layout elements as ink overflow. https://reviewboard.mozilla.org/r/258908/#review268660 > Hmm I added the same test case as above (mComputedStyle->GetPseudo() == nsCSSAnonBoxes::scrolledContent && mParent->StyleDisplay()->IsContainLayout()) and stepped through with lldb, but unlike the case in nsBlockFrame::ComputeOverflowAreas, it looks like we don't run into the scrollframe anonymous boxes here. Looks like leaving it out, since adding it doesn't change behaviour, might be the right thing to do? I think we really do need it here... it looks like nsBlockFrame.cpp has a call to this function (ConsiderChildOverflow), and the scrolledContent anonymous box is a nsBlockFrame typically, so it should be possible to trigger this in a way where it'd matter, I suspect. I'm going to see if I can come up with a testcase where it matters...
Comment on attachment 8994313 [details] Bug 1476495 - Treat overflow in contain:layout elements as ink overflow. https://reviewboard.mozilla.org/r/258908/#review268858 Nearly r+ (with these nits), but leaving this r- for now since I'm not yet sure about the scrolledcontent thing in ConsiderChildOverflow (please ping me if I get distracted & haven't followed up on this in a couple hours)... ::: layout/generic/nsFrame.cpp:9689 (Diff revision 4) > + aOverflowAreas.VisualOverflow().Union( > + aChildFrame->GetOverflowAreas().VisualOverflow()); > + } else { > - aOverflowAreas.UnionWith(aChildFrame->GetOverflowAreas() + > + aOverflowAreas.UnionWith(aChildFrame->GetOverflowAreas() + > - aChildFrame->GetPosition()); > + aChildFrame->GetPosition()); > -} > + } It looks like the new chunk isn't using aChildFrame->GetPosition() (like the old chunk does). The new chunk probably should be, though. (I think aChildFrame's overflow areas are in a different coordinate space from the parent's overflow areas, which is why we need to offset by aChildFrame->GetPosition()) So I think you want to be passing the following expression into your new Union(...) call: aChildFrame->GetOverflowAreas().VisualOverflow() + aChildFrame->GetPosition() ::: layout/reftests/w3c-css/submitted/contain/contain-layout-overflow-001.html:47 (Diff revision 4) > + <!--CSS Test: Elements with contain:layout that do not produce scrollable overflow should paint as if containment were not applied. --> > + <div class="outer"> > + <div class="inner-sm contain"></div> > + </div> > + <br> > + > + <div class="outer auto"> > + <div class="inner-md contain"> > + <div class="inner-lg pass"></div> > + <div class="inner-lg fail"></div> > + </div> > + </div> > + <br> > + > + > + <!--CSS Test: Elements that overflow and have contain:layout applied should not change the scrollable overflow regions of their parent --> > + <div class="outer auto"> nit: looks like your middle example here (the "outer auto" one) could use an HTML comment explaining what it's testing.
Attachment #8994313 - Flags: review?(dholbert) → review-
Comment on attachment 8994313 [details] Bug 1476495 - Treat overflow in contain:layout elements as ink overflow. https://reviewboard.mozilla.org/r/258908/#review268660 > I think we really do need it here... it looks like nsBlockFrame.cpp has a call to this function (ConsiderChildOverflow), and the scrolledContent anonymous box is a nsBlockFrame typically, so it should be possible to trigger this in a way where it'd matter, I suspect. I'm going to see if I can come up with a testcase where it matters... OK, I do see ConsiderChildOverflow getting called for the ::scrolledcontent frame (and us getting the wrong result) if I adjust your testcase to add "float:left" on the `.inner-lg` class. That testcase-tweak doesn't change the rendering in Chrome, but it causes Firefox-with-your-patch-applied to paint a scrollbar on the second and third examples in your testcase, based on my local testing. So: please do make the ::scrolledcontent stylecontext adjustment here, and please add a second copy of your testcase with float:left on `.inner-lg`. (You could rename your current testcase to -001a and name the new copy -001b, if you like -- and the reference would keep its already-existing -001-ref naming.)
Comment on attachment 8994313 [details] Bug 1476495 - Treat overflow in contain:layout elements as ink overflow. https://reviewboard.mozilla.org/r/258908/#review268858 > nit: looks like your middle example here (the "outer auto" one) could use an HTML comment explaining what it's testing. This test was bundled into the chunk of tests Gerard submitted to WPT that we haven't pulled yet (as of yesterday, at least). Should I remove it since the identical one is upstream, or is it worth keeping for coverage? (The comment I'd add would be pretty much the same as the last test's comment; they both ensure layout-contained elements with large children have the overflow turned to visual/ink oveflow instead of scrollable overflow)
Comment on attachment 8994313 [details] Bug 1476495 - Treat overflow in contain:layout elements as ink overflow. https://reviewboard.mozilla.org/r/258908/#review268858 > This test was bundled into the chunk of tests Gerard submitted to WPT that we haven't pulled yet (as of yesterday, at least). Should I remove it since the identical one is upstream, or is it worth keeping for coverage? (The comment I'd add would be pretty much the same as the last test's comment; they both ensure layout-contained elements with large children have the overflow turned to visual/ink oveflow instead of scrollable overflow) I'd just keep it, for better test coverage at the moment this lands (better to know if passes/fails right away rather than to be surprised on the wpt merge in a few days) -- and also, for the time being, we get slightly better coverage for tests that live in layout/reftests than we do in the wpt tests. So, there's some value in keeping it around even if it's slightly duplicative. RE the prose of the comment, the key thing I'm interested in is that it should highlight the difference between the two cases (they're similar but what's the key difference that prompted us to include both?) E.g. maybe the first could be a verbose-ish explanation (using a version of your current language), and the second could be: <!-- Similar to previous, but now with [...] --->
Comment on attachment 8994313 [details] Bug 1476495 - Treat overflow in contain:layout elements as ink overflow. https://reviewboard.mozilla.org/r/258908/#review268858 > I'd just keep it, for better test coverage at the moment this lands (better to know if passes/fails right away rather than to be surprised on the wpt merge in a few days) -- and also, for the time being, we get slightly better coverage for tests that live in layout/reftests than we do in the wpt tests. So, there's some value in keeping it around even if it's slightly duplicative. > > RE the prose of the comment, the key thing I'm interested in is that it should highlight the difference between the two cases (they're similar but what's the key difference that prompted us to include both?) > > E.g. maybe the first could be a verbose-ish explanation (using a version of your current language), and the second could be: > <!-- Similar to previous, but now with [...] ---> (elaborating on the "slightly better coverage" for tests in layout/reftests -- that betterness is due to the mozilla-specific reftest harness detecting assertion failures & treating them as test-failures, and also [less relevant to this case probably] detecting fuzzy pixels and allowing us to annotate them without requiring the whole test to be expected-fail. There's a bug somewhere on updating the WPT harness to support these features as well, but I can't find it at the moment.)
Comment on attachment 8994313 [details] Bug 1476495 - Treat overflow in contain:layout elements as ink overflow. https://reviewboard.mozilla.org/r/258908/#review269010 Upgrading "r-" to "r+ with nits addressed" (punting on the scrollable anonymous-box check in nsFrame::ConsiderChildOverflow for the time being), given that we've got bug 1481951 to cover the known-incompleteness RE overflow from floats. For now, let's get the known-working parts here landed, so that we can iterate and unblock other bugs that need to call the style-struct IsContainLayout() API (added here).
Attachment #8994313 - Flags: review- → review+
Comment on attachment 8994313 [details] Bug 1476495 - Treat overflow in contain:layout elements as ink overflow. https://reviewboard.mozilla.org/r/258908/#review268660 > Ah, and it looks like dbaron is adding "eSupportsContainLayoutAndPaint" in bug 1479859, which is more specific and is probably what we should really be checking here, I guess. Maybe use eLineParticipant for now but add a comment like: > // XXX Really should check eSupportsContainLayoutAndPaint from bug 1479859. Going to rebase and change this to (StyleDisplay()->IsContainLayout() && IsFrameOfType(eSupportsContainLayoutAndPaint)) per IRC discussion and work at bug 1479859
Comment on attachment 8994313 [details] Bug 1476495 - Treat overflow in contain:layout elements as ink overflow. https://reviewboard.mozilla.org/r/258906/#review269096 ::: layout/generic/nsFrame.cpp:9681 (Diff revision 5) > + if ((StyleDisplay()->IsContainLayout() && > + IsFrameOfType(eSupportsContainLayoutAndPaint)) || > + (mComputedStyle->GetPseudo() == nsCSSAnonBoxes::scrolledContent && > + mParent->StyleDisplay()->IsContainLayout())) { I want to collapse this condition into something more readable (more like the nsBlockFrame::ComputeOverflowAreas function above), but we aren't calling any function we need to pass a new nsStyleDisplay into. Would it be more readable to leave as is or change to something like: nsStyleDisplay* display = StyleDisplay(); if (mComputedStyle->GetPseudo() == nsCSSAnonBoxes::scrolledContent) { display = mParent->StyleDisplay(); } if (display->IsContainLayout() && IsFrameOfType(eSupportsContainLayoutAndPaint)) { ... } (I think StyleDisplay() returns a const nsStyleDisplay object, so might not be able to do reassignment like this after all)
Keywords: checkin-needed
I was going to land this but noticed an issue before landing, hence removing checkin-needed. It looks like contain-layout-overflow-001b.html isn't quite right at this point -- it's only got two sections, whereas its reference case has three sections. So even if we had bug 1481951 addressed, the testcase would still fail because it doesn't have enough stuff in it.
Flags: needinfo?(mreschenberg)
Keywords: checkin-needed
Comment on attachment 8994313 [details] Bug 1476495 - Treat overflow in contain:layout elements as ink overflow. https://reviewboard.mozilla.org/r/258906/#review269180 ::: layout/reftests/w3c-css/submitted/contain/contain-layout-overflow-001b.html:43 (Diff revision 6) > +<body> > + <!--CSS Test: Layout-contained elements that overflow their container and have children who overflow should produce the same amount of scrollable overflow as if there were no children. --> > + <div class="outer auto"> > + <div class="outer contain"> > + <div class="inner-lg pass"></div> > + <div class="inner-lg fail"></div> > + </div> > + </div> > + <br> > + > + > + <!--CSS Test: Layout-contained elements that do not overflow their container, but have children who overflow, should not allow their children to affect the scrollable overflow regions of their parent. --> > + <div class="outer auto"> > + <div class="inner-sm contain border"> > + <div class="inner-lg"> > + </div> > + </div> > + </div> > +</body> Here's the problematic piece of the testcase thatI was referring to above -- it's only got 2 chunks in the <body> here, whereas the testcase has 3, so they trivially don't match & this fails for the wrong reason rather than for the right reason. :)
(In reply to Daniel Holbert [:dholbert] from comment #48) > Here's the problematic piece of the testcase thatI was referring to above > -- it's only got 2 chunks in the <body> here, whereas the testcase has 3, so > they trivially don't match & this fails for the wrong reason rather than for > the right reason. :) Added a float base case like I had in the -001a file, but split them into -001 and -002 to add some float:left conditioning to the ref case also.
Flags: needinfo?(mreschenberg)
Sounds reasonable. Thanks!
Keywords: checkin-needed
Pushed by csabou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b580e5b9f9e4 Treat overflow in contain:layout elements as ink overflow. r=dholbert
Keywords: checkin-needed
Backed out changeset for wpt reftest failures on contain-layout-ink-overflow. Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&fromchange=b580e5b9f9e43851d654fc93c6777a655c875c62&filter-searchStr=Linux%20opt%20Web%20platform%20tests%20with%20e10s%20test-linux32%2Fopt-web-platform-tests-reftests-e10s-5%20W-e10s(Wr5)&tochange=29b6e333a0ed5322c4387e412d54c8437fae013b&selectedJob=193213137 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=193213137&repo=autoland&lineNumber=12651 Backout link: https://hg.mozilla.org/integration/autoland/rev/29b6e333a0ed5322c4387e412d54c8437fae013b [task 2018-08-10T04:22:12.452Z] 04:22:12 INFO - TEST-START | /css/css-contain/contain-layout-ink-overflow-013.html [task 2018-08-10T04:22:12.459Z] 04:22:12 INFO - PID 5797 | 1533874932452 Marionette INFO Testing http://web-platform.test:8000/css/css-contain/contain-layout-ink-overflow-013.html == http://web-platform.test:8000/css/css-contain/reference/contain-layout-ink-overflow-013-ref.html [task 2018-08-10T04:22:12.460Z] 04:22:12 INFO - PID 5797 | 1533874932455 Marionette DEBUG [6442450945] Waiting for page load of http://web-platform.test:8000/css/css-contain/reference/contain-layout-ink-overflow-013-ref.html [task 2018-08-10T04:22:12.476Z] 04:22:12 INFO - PID 5797 | 1533874932469 Marionette DEBUG [6442450945] flushRendering true [task 2018-08-10T04:22:12.485Z] 04:22:12 INFO - PID 5797 | 1533874932478 Marionette DEBUG [6442450945] Waiting for page load of http://web-platform.test:8000/css/css-contain/contain-layout-ink-overflow-013.html [task 2018-08-10T04:22:12.500Z] 04:22:12 INFO - PID 5797 | 1533874932492 Marionette DEBUG [6442450945] flushRendering true [task 2018-08-10T04:22:12.540Z] 04:22:12 INFO - TEST-UNEXPECTED-PASS | /css/css-contain/contain-layout-ink-overflow-013.html | Testing http://web-platform.test:8000/css/css-contain/contain-layout-ink-overflow-013.html == http://web-platform.test:8000/css/css-contain/reference/contain-layout-ink-overflow-013-ref.html [task 2018-08-10T04:22:12.541Z] 04:22:12 INFO - REFTEST IMAGE 1 (TEST): data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAlgAAAJYCAYAAAC+ZpjcAAAOsUlEQVR4nO3dS27jSBZAUe9/oTnu6g10jwooFCwpSF3b7xnnADFIiZ9g0BAvpEF+/A8AgNTHT08AAOC3EVgAADGBBQAQE1gAADGBBQAQE1gAADGBBQAQE1gAADGBBQAQE1gAADGBBQAQE1gAADGBBQAQE1gAADGBBQAQE1gAADGBBQAQE1i/xMeHW/k3awHATzt6En18fLwcXzI5D8pjj9bq9D692n+TbfP9ahvvIcB2x4F15d8VD4X3XLlPd99jB/cQ4HsJrF/syvoJrN/NPQT4Xrc+dV99WJ/8LPXqZ8arP0X+8/2r+7wzz9NtyvM92ufucV6tdb2up/fqzs/Q796jO/Ms51ysx2fbCSyA75UH1qOH28k2734z9uzhejKnO/N851rurstnTq/7yv7P5vDOWj875tXjPJrvnfM/OlZ97VfOc/XY71wrAJ00sE7D63T/O4F1572r5796LfW6nG5TBtbJ61eOcfdefdV9vHq+6nre3f/O3xkAX+/HAuvdh8475z/d5mSep9ucvPfONw0C6/m5ynV9dI6rxyj2F1gAM+WB9Ww82/bKea7O67P3TuZ4ZZ5XjvHuupxc36PXruz/6r3TNX10nc/Od3qc0/mW6/rZ618dWCfrIbAA5vi2b7BOjnnloXX1/CcPotMH4dVvtep1ebTdyWtX9n/13jv36ytjoLqPz4415Rus+hgANH7sJ8KT/X86sE62e2ebu/M63WZSYFVBcmct6nW9co53z3PlPYEFMEcaWI/ee/RT0sl2p+e9ety7r/37eHev5XRej/Y92eadwLobfu+89u4+j7ar1/XK/Ipvn94937PrF14AX+Pyp+vfH8qvIuvVdv/e5mS7k7ndPf8/X7s6zzvX8u66PNrnyrmuHufVtqfnPp1bdQ2fHevuuj57vZhzsR5X/7YB6P2qT1cPCwBggl9VJAILAJjg1xTJ1Z9lAAC+ihoBAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLFjoz58/RwOAnyGwYCGBBTCbwIKFBBbAbAILFhJYALMJLFhIYAHMJrBgIYEFMJvAgoUEFsBsAgsWElgAswksWEhgAcwmsGAhgQUwm8CChQQWwGwCCxYSWACzCSxYSGABzCawYCGBBTCbwIKFBBbAbAILFhJYALMJLFhIYAHMJrBgIYEFMJvAgoUEFsBsAgsWElgAswksWEhgAcwmsGAhgQUwm8CChQQWwGwCCxYSWACzCSxYSGABzCawYCGBBTCbwIKFBBbAbAILFhJYALMJLFhIYAHMJrBgIYEFMJvAgoUEFsBsAgsWElgAswksWEhgAcwmsGAhgQUwm8CChQQWwGwCCxYSWACzCSxYSGABzCawYCGBBTCbwIKFBBbAbAILFhJYALMJLFhIYAHMJrBgIYEFMJvAgoUEFsBsAgsWElgAswksWEhgAcwmsGAhgQUwm8CChQQWwGwCCxYSWACzCSxYSGABzCawYCGBBTCbwIKFBBbAbAILFhJYALMJLFhIYAHMJrBgIYEFMJvAgoUEFsBsAgsWElgAswksWEhgAcwmsGAhgQUwm8CChQQWwGwCCxYSWACzCSxYSGABzCawYCGBBTCbwIKFBBbAbAILFhJYALMJLFhIYAHMJrBgIYEFMJvAgoUEFsBsAgsWElgAswksWEhgAcwmsGAhgQUwm8CChQQWwGwCCxYSWACzCSxYSGABzCawYCGBBTCbwIKFBBbAbAILFhJYALMJLFhIYAHMJrBgIYEFMJvAgoUEFsBsAgsWElgAswksWEhgAcwmsGAhgQUwm8CChQQWwGwCCxYSWACzCSxYSGABzCawYCGBBTCbwIKFBBbAbAILFhJYALMJLFhIYAHMJrBgIYEFMJvAgoUEFsBsAgsWElgAswksWEhgAcwmsGAhgQUwm8CChQQWwGwCCxYSWACzCSxYSGABzCawYCGBBTCbwIKFBBbAbAILFhJYALMJLFhIYAHMJrBgIYEFMJvAgoUEFsBsAgsWElgAswksWEhgAcwmsGAhgQUwm8CChQQWwGwCCxYSWACzCSxYSGABzCawYCGBBTCbwIKFBBbAbAILFhJYALMJLFhIYAHMJrBgIYEFMJvAgoUEFsBsAgsWElgAswksWEhgAcwmsGAhgQUwm8CChQQWwGwCCxYSWACzCSxYSGABzCawYCGBBTCbwIKFBBbAbAILFhJYALMJLFhIYAHMJrBgIYEFMJvAgoUEFsBsAgsWElgAswksWEhgAcwmsGAhgQUwm8CChQQWwGwCCxYSWACzCSxYSGABzCawYCGBBTCbwIKFBBbAbAILFhJYALMJLFhIYAHMJrBgIYEFMJvAgoUEFsBsAgsWElgAswksWEhgAcwmsGAhgQUwm8CChQQWwGwCCxYSWACzCSxYSGABzCawYCGBBTCbwIKFBBbAbAILFhJYALMJLFhIYAHMJrBgIYEFMJvAgoUEFsBsAgsWElgAswksWEhgAcwmsGAhgQUwm8CChQQWwGwCCxYSWACzCSxYSGABzCawYCGBBTDby8A6/SA3DOP7xn//+s/RAOBnCCzDWDgEFsBsAsswFg6BBTCbwDKMhUNgAcwmsAxj4RBYALMJLMNYOAQWwGwCyzAWDoEFMJvAMoyFQ2ABzCawDGPhEFgAswksw1g4BBbAbP6rHACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACA2P8BwWm3pr+h+WcAAAAASUVORK5CYII= [task 2018-08-10T04:22:12.542Z] 04:22:12 INFO - REFTEST IMAGE 2 (REFERENCE): data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAlgAAAJYCAYAAAC+ZpjcAAAOsUlEQVR4nO3dS27jSBZAUe9/oTnu6g10jwooFCwpSF3b7xnnADFIiZ9g0BAvpEF+/A8AgNTHT08AAOC3EVgAADGBBQAQE1gAADGBBQAQE1gAADGBBQAQE1gAADGBBQAQE1gAADGBBQAQE1gAADGBBQAQE1gAADGBBQAQE1gAADGBBQAQE1i/xMeHW/k3awHATzt6En18fLwcXzI5D8pjj9bq9D692n+TbfP9ahvvIcB2x4F15d8VD4X3XLlPd99jB/cQ4HsJrF/syvoJrN/NPQT4Xrc+dV99WJ/8LPXqZ8arP0X+8/2r+7wzz9NtyvM92ufucV6tdb2up/fqzs/Q796jO/Ms51ysx2fbCSyA75UH1qOH28k2734z9uzhejKnO/N851rurstnTq/7yv7P5vDOWj875tXjPJrvnfM/OlZ97VfOc/XY71wrAJ00sE7D63T/O4F1572r5796LfW6nG5TBtbJ61eOcfdefdV9vHq+6nre3f/O3xkAX+/HAuvdh8475z/d5mSep9ucvPfONw0C6/m5ynV9dI6rxyj2F1gAM+WB9Ww82/bKea7O67P3TuZ4ZZ5XjvHuupxc36PXruz/6r3TNX10nc/Od3qc0/mW6/rZ618dWCfrIbAA5vi2b7BOjnnloXX1/CcPotMH4dVvtep1ebTdyWtX9n/13jv36ytjoLqPz4415Rus+hgANH7sJ8KT/X86sE62e2ebu/M63WZSYFVBcmct6nW9co53z3PlPYEFMEcaWI/ee/RT0sl2p+e9ety7r/37eHev5XRej/Y92eadwLobfu+89u4+j7ar1/XK/Ipvn94937PrF14AX+Pyp+vfH8qvIuvVdv/e5mS7k7ndPf8/X7s6zzvX8u66PNrnyrmuHufVtqfnPp1bdQ2fHevuuj57vZhzsR5X/7YB6P2qT1cPCwBggl9VJAILAJjg1xTJ1Z9lAAC+ihoBAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLFjoz58/RwOAnyGwYCGBBTCbwIKFBBbAbAILFhJYALMJLFhIYAHMJrBgIYEFMJvAgoUEFsBsAgsWElgAswksWEhgAcwmsGAhgQUwm8CChQQWwGwCCxYSWACzCSxYSGABzCawYCGBBTCbwIKFBBbAbAILFhJYALMJLFhIYAHMJrBgIYEFMJvAgoUEFsBsAgsWElgAswksWEhgAcwmsGAhgQUwm8CChQQWwGwCCxYSWACzCSxYSGABzCawYCGBBTCbwIKFBBbAbAILFhJYALMJLFhIYAHMJrBgIYEFMJvAgoUEFsBsAgsWElgAswksWEhgAcwmsGAhgQUwm8CChQQWwGwCCxYSWACzCSxYSGABzCawYCGBBTCbwIKFBBbAbAILFhJYALMJLFhIYAHMJrBgIYEFMJvAgoUEFsBsAgsWElgAswksWEhgAcwmsGAhgQUwm8CChQQWwGwCCxYSWACzCSxYSGABzCawYCGBBTCbwIKFBBbAbAILFhJYALMJLFhIYAHMJrBgIYEFMJvAgoUEFsBsAgsWElgAswksWEhgAcwmsGAhgQUwm8CChQQWwGwCCxYSWACzCSxYSGABzCawYCGBBTCbwIKFBBbAbAILFhJYALMJLFhIYAHMJrBgIYEFMJvAgoUEFsBsAgsWElgAswksWEhgAcwmsGAhgQUwm8CChQQWwGwCCxYSWACzCSxYSGABzCawYCGBBTCbwIKFBBbAbAILFhJYALMJLFhIYAHMJrBgIYEFMJvAgoUEFsBsAgsWElgAswksWEhgAcwmsGAhgQUwm8CChQQWwGwCCxYSWACzCSxYSGABzCawYCGBBTCbwIKFBBbAbAILFhJYALMJLFhIYAHMJrBgIYEFMJvAgoUEFsBsAgsWElgAswksWEhgAcwmsGAhgQUwm8CChQQWwGwCCxYSWACzCSxYSGABzCawYCGBBTCbwIKFBBbAbAILFhJYALMJLFhIYAHMJrBgIYEFMJvAgoUEFsBsAgsWElgAswksWEhgAcwmsGAhgQUwm8CChQQWwGwCCxYSWACzCSxYSGABzCawYCGBBTCbwIKFBBbAbAILFhJYALMJLFhIYAHMJrBgIYEFMJvAgoUEFsBsAgsWElgAswksWEhgAcwmsGAhgQUwm8CChQQWwGwCCxYSWACzCSxYSGABzCawYCGBBTCbwIKFBBbAbAILFhJYALMJLFhIYAHMJrBgIYEFMJvAgoUEFsBsAgsWElgAswksWEhgAcwmsGAhgQUwm8CChQQWwGwCCxYSWACzCSxYSGABzCawYCGBBTCbwIKFBBbAbAILFhJYALMJLFhIYAHMJrBgIYEFMJvAgoUEFsBsAgsWElgAswksWEhgAcwmsGAhgQUwm8CChQQWwGwCCxYSWACzCSxYSGABzCawYCGBBTCbwIKFBBbAbAILFhJYALMJLFhIYAHMJrBgIYEFMJvAgoUEFsBsAgsWElgAswksWEhgAcwmsGAhgQUwm8CChQQWwGwCCxYSWACzCSxYSGABzCawYCGBBTDby8A6/SA3DOP7xn//+s/RAOBnCCzDWDgEFsBsAsswFg6BBTCbwDKMhUNgAcwmsAxj4RBYALMJLMNYOAQWwGwCyzAWDoEFMJvAMoyFQ2ABzCawDGPhEFgAswksw1g4BBbAbP6rHACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACAmMACAIgJLACA2P8BwWm3pr+h+WcAAAAASUVORK5CYII= [task 2018-08-10T04:22:12.543Z] 04:22:12 INFO - TEST-INFO expected FAIL | took 72ms [task 2018-08-10T04:22:12.565Z] 04:22:12 INFO - PID 5797 | 1533874932564 Marionette INFO Stopped listening on port 2828 [task 2018-08-10T04:22:12.663Z] 04:22:12 INFO - PID 5797 | [Parent 5797, Gecko_IOThread] WARNING: pipe error (97): Connection reset by peer: file /builds/worker/workspace/build/src/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 353 [task 2018-08-10T04:22:12.990Z] 04:22:12 INFO - Browser exited with return code 0 [task 2018-08-10T04:22:12.993Z] 04:22:12 WARNING - u'runner_teardown': () [task 2018-08-10T04:22:13.009Z] 04:22:13 INFO - Setting up ssl [task 2018-08-10T04:22:13.033Z] 04:22:13 INFO - certutil | [task 2018-08-10T04:22:13.069Z] 04:22:13 INFO - certutil | [task 2018-08-10T04:22:13.091Z] 04:22:13 INFO - certutil | [task 2018-08-10T04:22:13.092Z] 04:22:13 INFO - Certificate Nickname Trust Attributes [task 2018-08-10T04:22:13.092Z] 04:22:13 INFO - SSL,S/MIME,JAR/XPI [task 2018-08-10T04:22:13.092Z] 04:22:13 INFO - [task 2018-08-10T04:22:13.093Z] 04:22:13 INFO - web-platform-tests CT,, [task 2018-08-10T04:22:13.093Z] 04:22:13 INFO - [task 2018-08-10T04:22:13.109Z] 04:22:13 INFO - Application command: /builds/worker/workspace/build/application/firefox/firefox --marionette about:blank -profile /tmp/tmpSGvVX0.mozrunner [task 2018-08-10T04:22:13.125Z] 04:22:13 INFO - Starting runner [task 2018-08-10T04:22:15.204Z] 04:22:15 INFO - PID 6051 | 1533874935199 Marionette INFO Listening on port 2828 [task 2018-08-10T04:22:15.325Z] 04:22:15 INFO - PID 6051 | 1533874935322 Marionette DEBUG [2147483649] Frame script loaded [task 2018-08-10T04:22:15.325Z] 04:22:15 INFO - PID 6051 | 1533874935323 Marionette DEBUG [2147483649] Frame script registered [task 2018-08-10T04:22:15.356Z] 04:22:15 INFO - PID 6051 | 1533874935345 Marionette DEBUG [2147483649] Received DOM event beforeunload for about:blank [task 2018-08-10T04:22:15.373Z] 04:22:15 INFO - PID 6051 | 1533874935367 Marionette DEBUG [2147483649] Received DOM event pagehide for about:blank [task 2018-08-10T04:22:15.374Z] 04:22:15 INFO - PID 6051 | 1533874935368 Marionette DEBUG [2147483649] Received DOM event unload for about:blank [task 2018-08-10T04:22:15.391Z] 04:22:15 INFO - PID 6051 | 1533874935383 Marionette DEBUG [2147483649] Received DOM event DOMContentLoaded for http://web-platform.test:8000/testharness_runner.html [task 2018-08-10T04:22:15.428Z] 04:22:15 INFO - PID 6051 | 1533874935415 Marionette DEBUG [2147483649] Received DOM event pageshow for http://web-platform.test:8000/testharness_runner.html [task 2018-08-10T04:22:15.444Z] 04:22:15 INFO - TEST-START | /css/css-contain/contain-layout-ink-overflow-014.html [task 2018-08-10T04:22:15.804Z] 04:22:15 INFO - PID 6051 | 1533874935797 Marionette DEBUG [6442450945] Frame script loaded [task 2018-08-10T04:22:15.805Z] 04:22:15 INFO - PID 6051 | 1533874935798 Marionette DEBUG [6442450945] Frame script registered [task 2018-08-10T04:22:15.808Z] 04:22:15 INFO - PID 6051 | 1533874935803 Marionette INFO Testing http://web-platform.test:8000/css/css-contain/contain-layout-ink-overflow-014.html == http://web-platform.test:8000/css/css-contain/reference/contain-layout-ink-overflow-013-ref.html [task 2018-08-10T04:22:15.829Z] 04:22:15 INFO - PID 6051 | 1533874935823 Marionette DEBUG [6442450945] Waiting for page load of http://web-platform.test:8000/css/css-contain/reference/contain-layout-ink-overflow-013-ref.html [task 2018-08-10T04:22:15.849Z] 04:22:15 INFO - PID 6051 | 1533874935845 Marionette DEBUG [6442450945] flushRendering false [task 2018-08-10T04:22:15.865Z] 04:22:15 INFO - PID 6051 | 1533874935855 Marionette DEBUG [6442450945] Waiting for page load of http://web-platform.test:8000/css/css-contain/contain-layout-ink-overflow-014.html [task 2018-08-10T04:22:15.873Z] 04:22:15 INFO - PID 6051 | 1533874935867 Marionette DEBUG [6442450945] flushRendering true
Flags: needinfo?(mreschenberg)
Removed (now passing) WPT .ini file :) looks like they got updated
Flags: needinfo?(mreschenberg)
Triggered an additional try run to see if the rest of the contain-layout-ink-overflow-0**.html tests were passing, too, looks like they are. Removed those .ini files. I'm going to wait til the active try run finishes and will review and attempt to land again tomorrow :)
The most recent Try run that I'm seeing from your username is: https://treeherder.mozilla.org/#/jobs?repo=try&revision=08378a456743798a8d3b2cc8fd96974efd4ef91d ...which has some orange because it doesn't seem to have the .ini removals that you've (correctly) got in the latest revision here. So: just in case, I triggered a try run as well with the latest version of the patches here (all platforms, WPT-only): https://treeherder.mozilla.org/#/jobs?repo=try&revision=fabfe1873e592354ce0efdc22e998bac4ba11119
Keywords: checkin-needed
Pushed by cbrindusan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/be0650692abc Treat overflow in contain:layout elements as ink overflow. r=dholbert
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/be0650692abc
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Firefox 63.0a1 buildID=20180810220115 passes almost perfectly http://test.csswg.org/harness/test/css-contain-1_dev/single/contain-layout-ink-overflow-013/format/html5/ but not entirely. It is not perfectly clear if the rendered layout is acceptable. I see an active vertical scrollbar while the reference file anticipated an inactive vertical scrollbar (with nowhere to scroll to vertically). This looks to me as a side effect of generating an horizontal scrollbar. Often, Firefox generates a vertical scrollbar in order to reach the content which was reduced due to a generated horizontal scrollbar. Other browsers now use all kinds of tricks (transient, fugitive scrollbars like in Safari and Epiphany; very slim, "razor"-thin scrollbars like Chrome) avoiding or compensating this. Other contain-layout-ink-overflow-0[14-20] tests are passed by Firefox 63.0a1 buildID=20180810220115 without an issue.
Depends on: 1483946
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: