Daniel Holbert [:dholbert] Assignee
	Description • 11 years ago

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).

	Daniel Holbert [:dholbert] Assignee
	Comment 1 • 11 years ago

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

	Daniel Holbert [:dholbert] Assignee
	Comment 2 • 11 years ago

(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.

	Daniel Holbert [:dholbert] Assignee
	Comment 3 • 11 years ago

I think this takes care of it, though I want to do a bit more testing (& throw in some automated tests) before requesting review.

	Daniel Holbert [:dholbert] Assignee
	Comment 4 • 11 years ago

(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]

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 11 years ago

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...

	Daniel Holbert [:dholbert] Assignee
	Comment 6 • 11 years ago

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>.

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 11 years ago

> 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...

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 11 years ago

Oh, and I'm OK with whatever behavior we want for buttons.

	Daniel Holbert [:dholbert] Assignee
	Comment 9 • 11 years ago

(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!

	Daniel Holbert [:dholbert] Assignee
	Comment 10 • 11 years ago

Same code patch, just added some reftests.

Green try run: https://tbpl.mozilla.org/?tree=Try&rev=ad3b1bdcf5db

	Daniel Holbert [:dholbert] Assignee
	Comment 11 • 11 years ago

Oops, didn't meant to include the s/MOZ_ASSERT/NS_ASSERTION/ change. I'll take that out.

	Boris Zbarsky [:bzbarsky]
	Comment 13 • 11 years ago

Comment on attachment 744804 [details] [diff] [review]
fix v2b (now with reftests & with assertion-change removed)

r=me

	Daniel Holbert [:dholbert] Assignee
	Comment 14 • 11 years ago

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.

	Daniel Holbert [:dholbert] Assignee
	Comment 15 • 11 years ago

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.

	Daniel Holbert [:dholbert] Assignee
	Comment 16 • 11 years ago

Landed followup to rename the tests:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/f47c18bd70e3

[also: restoring needinfo=me per end of comment 14]

	Ed Morley [:emorley]
	Comment 17 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/1949afc76980
https://hg.mozilla.org/mozilla-central/rev/f47c18bd70e3

	Daniel Holbert [:dholbert] Assignee
	Comment 18 • 11 years ago

[/me resolved dependent bugs as well. Clearing my self-needinfo.]

	Jean-Yves Perrier [:teoli]
	Comment 19 • 11 years ago

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