Closed
Bug 1223568
Opened 9 years ago
Closed 9 years ago
Flex hit MOZ_CRASH(Unexpected justify-content value)
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: jruderman, Assigned: dholbert)
References
Details
(Keywords: assertion, testcase)
Attachments
(3 files, 1 obsolete file)
Hit MOZ_CRASH(Unexpected justify-content value) at layout/generic/nsFlexContainerFrame.cpp:2537 Looks like the switch in MainAxisPositionTracker::MainAxisPositionTracker wasn't expecting NS_STYLE_JUSTIFY_STRETCH. This code was all added in https://hg.mozilla.org/mozilla-central/rev/076d87bf30d0. (Should NS_STYLE_JUSTIFY_* and the other things in nsStyleConsts.h be enum classes, so they can have compiler-checked exhaustive switches?)
Reporter | ||
Comment 1•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → dholbert
(In reply to Jesse Ruderman from comment #0) > (Should NS_STYLE_JUSTIFY_* and the other things in nsStyleConsts.h be enum > classes, so they can have compiler-checked exhaustive switches?) I filed bug 1223653 on starting this, and if it's a success we can file more to continue.
Assignee | ||
Comment 3•9 years ago
|
||
brief history: * The reason "stretch" isn't handled here is that our original "justify-content" impl didn't include "stretch" as an option -- it was based off of flexbox's "justify-content" property-definition, here: https://drafts.csswg.org/css-flexbox-1/#justify-content-property * Mats recently landed support for new "justify-content" values, including "stretch", in bug 1151214. * That landing was the most proximal cause of this -- it was known to be not-entirely-compatible with flexbox, per the existence of followup bug 1207698. (Bug 1151214 included shims to avoid aborts, but we must've missed one.)
Assignee | ||
Comment 4•9 years ago
|
||
Actually, it's not the "stretch" that's causing us trouble here; it's stretch *combined with* a fallback value. Minimal testcase that triggers the abort: data:text/html,<div style="display:flex; justify-content: stretch end">a
Assignee | ||
Comment 5•9 years ago
|
||
The problem is, this fixup code is failing to activate (due to the fallback value messing up the equality comparison): > 1742 switch (aDisplay->mDisplay) { > 1743 case NS_STYLE_DISPLAY_FLEX: > 1744 case NS_STYLE_DISPLAY_INLINE_FLEX: > 1745 // XXX maybe map 'auto' too? (ISSUE 8 in the spec) > 1746 // https://drafts.csswg.org/css-align-3/#content-distribution > 1747 if (mJustifyContent == NS_STYLE_JUSTIFY_STRETCH) { > 1748 return NS_STYLE_JUSTIFY_FLEX_START; > 1749 } > 1750 break; https://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.cpp?rev=5ae04c1c341c#1739 (also: in comment 3, when I talked about bug 1151214, I really meant bug 1176782)
Blocks: 1176782
Assignee | ||
Comment 6•9 years ago
|
||
I'm actually not sure the special-case in comment 5 should be there. It seems to be assuming that "stretch" should be treated as "flex-start" for flexbox, but it's not clear to me that this is true. The css-align spec says stretch *falls back* to 'flex-start', but the fallback alignment only matters when there's negative excess space. In cases where there's *positive* excess space (even after the flexbox algorithm has run), I think the spec currently requires that we honor "stretch". (Such cases are a bit edge-casey; the only scenario I can think of is with fractional "flex-grow" values, which limit how much growing the flexbox algorithm will do. E.g. <div style="display:flex; width:150px"> <div style="flex: 0.1 0 50px"></div> </div> In this case, the flex item's "0.1" flex-grow value means it only gets 10% of the available free space (100px), so it'll end up getting +10px = 60px and the remaining of the free space (90px) will go unused. But if we add "justify-content:stretch" to the container, I think the spec requires all of the remaining free space (90px) to be distributed in a second pass, as long as we don't violate max-width values.) Anyway; we can sort this out in bug 1207698 (and I should probably post to www-style to clarify the desired behavior in this scenario). For now, I'll go with the spirit of the existing code, to work around this crash.
Assignee | ||
Comment 7•9 years ago
|
||
This broadens the code quoted in comment 5. This isn't quite correct (if there's a fallback value, really we should use the fallback value instead of 'flex-start') -- but I'm not sure this special-case itself is really correct, per comment 6. So, this patch just extends the current code to treat "stretch [anything]" as "flex-start" for the moment, to avoid this crash, and then we can sort out how to actually handle "stretch" & its fallback values in bug 1207698.
Attachment #8686179 -
Flags: review?(mats)
Assignee | ||
Comment 8•9 years ago
|
||
(Sorry, previous fix had a misleading commit message from when I thought the problem was something different. Updated commit message here.)
Attachment #8686179 -
Attachment is obsolete: true
Attachment #8686179 -
Flags: review?(mats)
Attachment #8686181 -
Flags: review?(mats)
Comment 9•9 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #6) > The css-align spec says stretch *falls back* to 'flex-start', That text doesn't apply to 'justify-content' for Flexbox containers because: https://drafts.csswg.org/css-align-3/#propdef-justify-content "The 'justify-content' property ... 'stretch' computes to 'flex-start'." E.g. for 'stretch end' we can't make the computed value 'flex-start end' because that's an illegal value (can't have two <content-position>). The only other interpretation I see is that it's only literally 'stretch' that computes to 'flex-start', and that 'stretch <fallback>' computes to itself and "behaves as" <fallback> in layout (for 'justify-content'). But I don't see any support for that interpretation in the spec. https://drafts.csswg.org/css-align-3/#propdef-justify-content It seems to me what the editor is trying to say is that 'stretch' does not apply at all to 'justify-content' (since flexing is controlled by 'flex'). Might be worth checking what Chrome Canary does though and/or asking on www-style@ what the computed value is supposed to be for 'stretch end safe' for example.
Comment 10•9 years ago
|
||
Comment on attachment 8686181 [details] [diff] [review] fix v2 (now w/ more accurate commit message) I think this might be the correct fix actually.
Attachment #8686181 -
Flags: review?(mats) → review+
Assignee | ||
Comment 11•9 years ago
|
||
(In reply to Mats Palmgren (:mats) from comment #9) > That text doesn't apply to 'justify-content' for Flexbox containers because: > https://drafts.csswg.org/css-align-3/#propdef-justify-content > "The 'justify-content' property ... 'stretch' computes to 'flex-start'." Yeah, I realized that after the fact. (right after I posted to www-style, d'oh. :)) > Might be worth checking what Chrome Canary does though and/or > asking on www-style@ what the computed value is supposed to be for > 'stretch end safe' for example. Yup, just asked on www-style about "stretch end" (and mentioned safe|true in a parenthetical): https://lists.w3.org/Archives/Public/www-style/2015Nov/0214.html Thanks for review! I'll drop the XXXdholbert comment when landing, too, since you & I agree the new code is likely-correct (disagreeing with me-from-comment-6 who hadn't yet found the flexbox special case in the spec).
Assignee | ||
Comment 12•9 years ago
|
||
Actually, rather than dropping my XXX comment, I replaced it with an explanation so I don't get confused about this in the future. :) + // For flex containers, css-align-3 says the justify-content value + // "'stretch' computes to 'flex-start'." + // https://drafts.csswg.org/css-align-3/#propdef-justify-content Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5abdb4af2442
Comment 14•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/93f6fc8cc5d0
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in
before you can comment on or make changes to this bug.
Description
•