691 bytes, text/html
313 bytes, text/html
15.63 KB, patch
|Details | Diff | Splinter Review|
In bug 812822 comment 23, I mixed up pseudo-elements and pseudo-classes, and as a result we ended up building in an assumption that generated content from ::before & ::after shouldn't be treated as a flex item. That assumption is incorrect -- generated content *can* exist as a child of a flex item, and as-such, it should be treated as a flex item (and it should be able to be repositioned with e.g. "order" & "align-self" like other flex items can be).
Created attachment 743957 [details] testcase 1 Here's a testcase. EXPECTED RESULTS: The blue region should be at the far-left of the dotted flex container, and should stretch to fill it vertically. ACTUAL RESULTS: The blue region is at the bottom-right of the dotted flex container, and it doesn't stretch vertically. (It's also merged into the anonymous flex item of the raw text before it.) Nightly gives ACTUAL RESULTS; Chrome 28.0.1485.0 dev and Opera 12.15 yield EXPECTED RESULTS. Mozilla/5.0 (X11; Linux x86_64; rv:23.0) Gecko/20130430 Firefox/23.0
(In reply to Daniel Holbert [:dholbert] from comment #0) > In bug 812822 comment 23, I mixed up pseudo-elements and pseudo-classes [scratch that -- there was no such mixup -- first-line and first-letter *are* pseudo-elements. They just happen to be ignored in flex containers, whereas :before and :after are not ignored.] So the real problem was that the pseudo-element-related patch in bug 812822 comment 23 should've had an exception for :before and :after.
Created attachment 743998 [details] [diff] [review] fix v1 I think this takes care of it, though I want to do a bit more testing (& throw in some automated tests) before requesting review.
Created attachment 744282 [details] [diff] [review] fix v2 (Tweaked the comments a bit and fixed an issue in the earlier patch where the two *PseudoElementStyle() methods weren't consistent). I think this is basically what we want. Still need to add a reftest; added crashtests for dependent bugs, though. Also: this slightly breaks the rendering of any ::after content on e.g. <button style="display:flex">, by blockifying the ::after display value; I'm not sure that's a use case that's worth worrying about, though. [Testcase coming up to demonstrate this]
Comment on attachment 744282 [details] [diff] [review] fix v2 I guess we can't use the frame version of nsLayoutUtils::DoCompareTreePosition because we actually reorder our frames? This looks reasonable to me...
Created attachment 744285 [details] testcase that this patch breaks (but I'm not sure it's worth worrying about) So per end of previous comment, here's a testcase that this patch "breaks" -- a <button> with a "display:flex" style, which doesn't actually change the frame-type but which *does* make us blockify its ::after content now. So -- the expected rendering is that the button's text should all be on one line -- that's how nightly currently renders it. But with the patch, the ::after chunk "(including this part)" gets treated as a block and renders on a second line, inside the button. (as if I put "display:block" in the ::after declaration) I think this is a bit awkward to fix -- we'd need to add an extra parameter to nsStyleSet::ProbePseudoElementStyle and nsStyleSet::ResolvePseudoElementStyle, and make all the callers check whether we're actually in a flex container frame or e.g. a nsHTMLButtonControlFrame as is the case here. (if they even have that information available) Given that this isn't dangerous, I'm inclined to think that it's fine for ::after content on <button style="display:flex"> to be incorrectly blockified... It's an extreme edge case, and it's workaroundable by simply dropping the (ineffective) "display:flex" on the <button>.
> the expected rendering is that the button's text should all be on one line Why, necessarily? What does WebKit do? The real expected rendering sort of depends on how the spec defines rendering of buttons, which hasn't happened yet...
Oh, and I'm OK with whatever behavior we want for buttons.
(In reply to Boris Zbarsky (:bz) from comment #5) > I guess we can't use the frame version of > nsLayoutUtils::DoCompareTreePosition because we actually reorder our frames? Correct. If we used that function, that'd break a situation where e.g. we set "order: 1" on the first flex item to make it shift to the end, and then cleared that "order" property to make it jump back to the beginning. When "order" gets set to 1, we'd move the frame to the end of the flex container's mFrames list -- but later, when "order" gets cleared, we need to fall back to the DOM ordering to get the child frame's new correct position (at the beginning). (In reply to Boris Zbarsky (:bz) from comment #7) > Why, necessarily? What does WebKit do? WebKit and Opera both render the text all on one line, matching current Nightly. My reasoning behind why that's the correct behavior is as follows: we don't let the "display:flex" affect the styling of the button's other (anonymous) children, or let it affect anything else about the button. This would be the one exceptional case where an otherwise-completely-ignored "display:flex" value "leaks through" in a user-visible way. (This applies to any other HTML tag with a "display"-independent mandatory frame type which an author puts some ::before/::after content onto. e.g. <fieldset>, <input>, ...) > The real expected rendering sort of depends on how the spec defines > rendering of buttons, which hasn't happened yet... True. (In reply to Boris Zbarsky (:bz) from comment #8) > Oh, and I'm OK with whatever behavior we want for buttons. Cool -- I'll add a reftest and some ordering tests and then request official review. Thanks!
Created attachment 744802 [details] [diff] [review] fix v2a (now w/ reftests) Same code patch, just added some reftests. Green try run: https://tbpl.mozilla.org/?tree=Try&rev=ad3b1bdcf5db
Oops, didn't meant to include the s/MOZ_ASSERT/NS_ASSERTION/ change. I'll take that out.
Created attachment 744804 [details] [diff] [review] fix v2b (now with reftests & with assertion-change removed)
Comment on attachment 744804 [details] [diff] [review] fix v2b (now with reftests & with assertion-change removed) r=me
Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/1949afc76980 When this makes it to m-c, we can resolve the dependent bugs as FIXED by this one, and mark them in-testsuite, since I included their tests as crashtests. Tagging needinfo=me to remind myself to do that.
fryn points out that this patch's flexbox-with-pseuedo-* reftests have a bonus "e" in their spelling of "pseuedo" (should be "pseudo"). I'll push a renaming followup after my local build completes and I verify that they still run correctly.
Landed followup to rename the tests: https://hg.mozilla.org/integration/mozilla-inbound/rev/f47c18bd70e3 [also: restoring needinfo=me per end of comment 14]
[/me resolved dependent bugs as well. Clearing my self-needinfo.]
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/23 and https://developer.mozilla.org/en-US/docs/Web/CSS/align-self https://developer.mozilla.org/en-US/docs/Web/CSS/flex-basis https://developer.mozilla.org/en-US/docs/Web/CSS/flex-grow https://developer.mozilla.org/en-US/docs/Web/CSS/flex-shrink https://developer.mozilla.org/en-US/docs/Web/CSS/flex