Last Comment Bug 867454 - Support ::before, ::after as flex items
: Support ::before, ::after as flex items
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla23
Assigned To: Daniel Holbert [:dholbert]
:
Mentors:
Depends on: 812822
Blocks: 826483 866547 873172
  Show dependency treegraph
 
Reported: 2013-04-30 16:51 PDT by Daniel Holbert [:dholbert]
Modified: 2013-08-29 04:09 PDT (History)
4 users (show)
dholbert: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase 1 (691 bytes, text/html)
2013-04-30 17:02 PDT, Daniel Holbert [:dholbert]
no flags Details
fix v1 (4.79 KB, patch)
2013-04-30 18:28 PDT, Daniel Holbert [:dholbert]
no flags Details | Diff | Review
fix v2 (7.53 KB, patch)
2013-05-01 14:25 PDT, Daniel Holbert [:dholbert]
bzbarsky: feedback+
Details | Diff | Review
testcase that this patch breaks (but I'm not sure it's worth worrying about) (313 bytes, text/html)
2013-05-01 14:32 PDT, Daniel Holbert [:dholbert]
no flags Details
fix v2a (now w/ reftests) (16.52 KB, patch)
2013-05-02 13:52 PDT, Daniel Holbert [:dholbert]
no flags Details | Diff | Review
fix v2b (now with reftests & with assertion-change removed) (15.63 KB, patch)
2013-05-02 13:55 PDT, Daniel Holbert [:dholbert]
bzbarsky: review+
Details | Diff | Review

Description Daniel Holbert [:dholbert] 2013-04-30 16:51:46 PDT
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).
Comment 1 Daniel Holbert [:dholbert] 2013-04-30 17:02:36 PDT
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
Comment 2 Daniel Holbert [:dholbert] 2013-04-30 18:19:47 PDT
(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.
Comment 3 Daniel Holbert [:dholbert] 2013-04-30 18:28:10 PDT
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.
Comment 4 Daniel Holbert [:dholbert] 2013-05-01 14:25:11 PDT
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 5 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-05-01 14:30:59 PDT
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...
Comment 6 Daniel Holbert [:dholbert] 2013-05-01 14:32:18 PDT
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>.
Comment 7 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-05-01 14:37:16 PDT
> 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...
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-05-01 14:37:36 PDT
Oh, and I'm OK with whatever behavior we want for buttons.
Comment 9 Daniel Holbert [:dholbert] 2013-05-01 14:48:03 PDT
(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!
Comment 10 Daniel Holbert [:dholbert] 2013-05-02 13:52:13 PDT
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
Comment 11 Daniel Holbert [:dholbert] 2013-05-02 13:54:23 PDT
Oops, didn't meant to include the s/MOZ_ASSERT/NS_ASSERTION/ change. I'll take that out.
Comment 12 Daniel Holbert [:dholbert] 2013-05-02 13:55:40 PDT
Created attachment 744804 [details] [diff] [review]
fix v2b (now with reftests & with assertion-change removed)
Comment 13 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-05-02 13:57:57 PDT
Comment on attachment 744804 [details] [diff] [review]
fix v2b (now with reftests & with assertion-change removed)

r=me
Comment 14 Daniel Holbert [:dholbert] 2013-05-02 14:06:44 PDT
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.
Comment 15 Daniel Holbert [:dholbert] 2013-05-02 14:36:31 PDT
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.
Comment 16 Daniel Holbert [:dholbert] 2013-05-02 15:49:20 PDT
Landed followup to rename the tests:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/f47c18bd70e3

[also: restoring needinfo=me per end of comment 14]
Comment 18 Daniel Holbert [:dholbert] 2013-05-03 09:28:18 PDT
[/me resolved dependent bugs as well. Clearing my self-needinfo.]

Note You need to log in before you can comment on or make changes to this bug.