Closed
Bug 969460
Opened 9 years ago
Closed 9 years ago
<html> (root element) won't accept 'display:flex'
Categories
(Core :: Layout, defect, P4)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: lcid-fire, Assigned: dholbert)
References
()
Details
Attachments
(5 files, 2 obsolete files)
532 bytes,
text/html
|
Details | |
559 bytes,
text/html
|
Details | |
2.99 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
4.99 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
9.80 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/33.0.1750.58 Safari/537.36 Steps to reproduce: 1. Open https://abergmeier.makes.org/thimble/thesis 2. Compare the page output with Chrome Actual results: The content is centered to the left. Expected results: The content should be centered in the middle.
Assignee | ||
Updated•9 years ago
|
Component: Untriaged → Layout
OS: Linux → All
Product: Firefox → Core
Hardware: x86_64 → All
Version: 26 Branch → Trunk
Assignee | ||
Comment 1•9 years ago
|
||
So the centering is accomplished with "margin: 0 auto" (i.e. auto margins on the left and right). It's having no effect in Firefox because the <body> element is sizing to the full width of its parent, possibly due to a UA stylesheet rule on <body>, or something.
Summary: flex layout is ignored for html → <html> styled as a flex container doesn't center <body> child (with 'margin: 0 auto' to achieve centering)
Assignee | ||
Comment 2•9 years ago
|
||
Here's a reduced testcase. The black-outline is the <html> (the flex container). The lime outline is its child, the <body>. And the blue outline is the actual content (the visible "pages" in the original URL). As you can see, we *are* in fact centering the lime outlined area. It's just that that area is the full width of its parent, so centering is trivial. The question is, why is the <body> taking the full width of its parent, and is that a bug.
Assignee | ||
Comment 3•9 years ago
|
||
(er, by "blue outline" I meant "blue region")
Assignee | ||
Comment 4•9 years ago
|
||
(For comparison, Chrome and Opera (12.16, with Presto) size the <body> (the lime-bordered region) to be 100px wide -- the size of the blue square. Firefox sizes it to be 400px wide -- the size of the <html>.)
Assignee | ||
Updated•9 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 5•9 years ago
|
||
For reference, here's the exact same testcase but now with the classes applied to <div> elements. We do the right thing here - no extra width on the (lime-bordered) flex item.
Assignee | ||
Updated•9 years ago
|
Attachment #8372591 -
Attachment description: reduced testcase → [wrong file]
Attachment #8372591 -
Attachment is obsolete: true
Assignee | ||
Comment 6•9 years ago
|
||
[sorry, attached the wrong file in previous comment; here's what I meant to attach, as a reference case with the styling just applied to divs.]
Assignee | ||
Comment 7•9 years ago
|
||
Ah, the reporter had it right with the bug's original summary -- <html> simply won't accept "display:flex". It sticks to "display:block". (maybe due to an !important rule somewhere) (At first, I thought the style was taking effect & this was just a <body> sizing thing, because if I set a fixed width on the body, I got the expected (centering) behavior. However, I was only getting that result because the centering technique ("margin: 0 auto") happens to work in blocks as well as flexboxes.)
Summary: <html> styled as a flex container doesn't center <body> child (with 'margin: 0 auto' to achieve centering) → <html> won't accept a different 'display'
Assignee | ||
Comment 8•9 years ago
|
||
Interesting... <html> *will* take "display:table", but won't take "display:flex". For example, this testcase reports that the computed display is "block" (i.e. "flex" is ignored): data:text/html,<html style="display:flex"><script>alert(window.getComputedStyle(document.documentElement,"").display)</script> ...but this testcase reports that the computed style is "table", so at least *that* non-block value works: data:text/html,<html style="display:table"><script>alert(window.getComputedStyle(document.documentElement,"").display)</script> "none" works, too: data:text/html,<html style="display:none"><script>alert(window.getComputedStyle(document.documentElement,"").display)</script> 'inline', 'table-cell', and 'list-item' get converted to "block", though. So it looks like we're applying something like nsRuleNode::EnsureBlockDisplay, but not exactly that, because that would preserve 'list-item' and 'flex'.
Summary: <html> won't accept a different 'display' → <html> won't accept 'display:flex'
Assignee | ||
Comment 9•9 years ago
|
||
OK, this conversion happens in this code: >333 // CSS2.1 section 9.2.4 specifies fixups for the 'display' property of >334 // the root element. [...] >340 if (!mParent) { >341 if (disp->mDisplay != NS_STYLE_DISPLAY_NONE && >342 disp->mDisplay != NS_STYLE_DISPLAY_BLOCK && >343 disp->mDisplay != NS_STYLE_DISPLAY_TABLE) { [code to convert 'display' to 'block', except 'inline-table' becomes 'table'] >357 } >358 } Link: http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleContext.cpp#333 The chunk of spec referenced in the code-comment there is: # For the root element, the computed value [of 'display'] is # changed as described in the section on the relationships # between 'display', 'position', and 'float'. http://www.w3.org/TR/CSS21/visuren.html#display-prop ...and that points to this section (about the relationships between 'display' etc): http://www.w3.org/TR/CSS21/visuren.html#dis-pos-flo ...which does have a specific exception, allowing "list-item" to be converted to block on the root element. (which explains why we're not using EnsureBlockDisplay here.) So we either need to: (A) Update the code quoted at the top of this comment to allow 'flex' (and to convert 'inline-flex' to 'flex'). ...or: (B) Replace the code quoted at the top of this comment with 'EnsureBlockDisplay', possibly with a new boolean argument that tells EnsureBlockDisplay to stomp on 'list-item'. (to preserve existing behavior for that display value) I prefer (B). That will be nicer going forward, so we don't run into this again for new display types like 'grid' and 'inline-grid'.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → dholbert
Comment 10•9 years ago
|
||
Note also that nsCSSFrameConstructor::ConstructDocElementFrame makes assumptions... http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#2369
Assignee | ||
Comment 11•9 years ago
|
||
I've got a patch to do (B), and it fixes the computed style (so, the first data URI in comment 8 will now report "flex"). However, as Mats says, it doesn't fix the layout, because nsCSSFrameConstructor::ConstructDocElementFrame only supports tables and blocks. We probably need some more logic there for other 'display' types.
Assignee | ||
Updated•9 years ago
|
Priority: -- → P4
![]() |
||
Updated•9 years ago
|
Summary: <html> won't accept 'display:flex' → <html> (root element) won't accept 'display:flex'
Assignee | ||
Comment 12•9 years ago
|
||
This patch adds a (not-yet-used) argument to EnsureBlockDisplay, to determine the "list-item" behavior. (The next patch will add a usage of this argument.)
Attachment #8375244 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 13•9 years ago
|
||
This patch... (a) ...makes us use EnsureBlockDisplay (with the new arg) in the root-node chunk of ApplyStyleFixups. (So, now that code benefits from the knowledge that "inline-flex" becomes "flex", and "flex" is fine as-is). (b) ...adds a clause to ConstructDocElementFrame that invokes NS_NewFlexContainerFrame. (The new clause is shamelessly cribbed from the XUL chunk right above it, which I think is fine, but let me know if I'm missing something. The other clauses (for SVG & table & block at least) are more idiosyncratic due to having custom nsCSSFrameConstructor functions (ConstructOuterSVG/ConstructTable/ConstructBlock))
Attachment #8375250 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 14•9 years ago
|
||
[meant to say: the patches here are based on top of the patch in bug 972119, which touches the same code (and should be landing soon)]
Depends on: 972119
Assignee | ||
Comment 15•9 years ago
|
||
Still need to add reftests, too. (I intend to get to that tomorrow morning.)
Flags: in-testsuite?
![]() |
||
Comment 16•9 years ago
|
||
Comment on attachment 8375244 [details] [diff] [review] part 1: Make EnsureBlockDisplay behavior on 'list-item' customizable > (so that we don't have to convert the root node) You mean "so that we don't have to support a list-item root node"? r=me if so.
Attachment #8375244 -
Flags: review?(bzbarsky) → review+
![]() |
||
Comment 17•9 years ago
|
||
Comment on attachment 8375250 [details] [diff] [review] part 2: Use EnsureBlockDisplay for root-node chunk of ApplyStyleFixups, & teach nsCSSFrameConstructor::ConstructDocElementFrame about 'flex' So given a root element like so: <svg xmlns="http://www.w3.org/2000/svg" style="display: flex"> this patch creates a flex container frame, right? That seems wrong to me. I think this new case should go after the SVG case. We should also assert in the "else" case at the end that display is in fact block. r=me with the above fixed assuming the ProcessChildren call will do any anonymous frame stuff needed.
Attachment #8375250 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 18•9 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #16) > Comment on attachment 8375244 [details] [diff] [review] > part 1: Make EnsureBlockDisplay behavior on 'list-item' customizable > > > (so that we don't have to convert the root node) > > You mean "so that we don't have to support a list-item root node"? > > r=me if so. Correct. (I'll fix the comment to say that.)
Assignee | ||
Comment 19•9 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #17) > Comment on attachment 8375250 [details] [diff] [review] > part 2: Use EnsureBlockDisplay for root-node chunk of ApplyStyleFixups, & > teach nsCSSFrameConstructor::ConstructDocElementFrame about 'flex' > > So given a root element like so: > > <svg xmlns="http://www.w3.org/2000/svg" style="display: flex"> > > this patch creates a flex container frame, right? That seems wrong to me. > I think this new case should go after the SVG case. Good point. Fixed in this version. > We should also assert in the "else" case at the end that display is in fact > block. Fixed. > r=me with the above fixed assuming the ProcessChildren call will do any > anonymous frame stuff needed. It appears to -- at least, raw text inside the <html> node seems to successfully create a flex item, so I think we're good.
Attachment #8376599 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Attachment #8375250 -
Attachment is obsolete: true
Assignee | ||
Comment 20•9 years ago
|
||
This adds... - A mochitest to test that we're modifying "display" in the same way on the root node and floated elements (except for with "display:list-item"). - A reftest for <svg style="display:flex"> (which fails with the old version of my "part 2" patch). - Two reftests in w3c-css/submitted to check that we honor "display:flex" on the root <html> node (using "justify-content:center" as a 'canary' to make sure we're actually rendering as a flex container). The mochitest (combined with the assert you suggested, in the "else" case) should help us catch this sort of bugs more proactively in the future, for new "display" types.
Attachment #8376600 -
Flags: review?(bzbarsky)
![]() |
||
Comment 21•9 years ago
|
||
Comment on attachment 8376600 [details] [diff] [review] part 3: tests r=me
Attachment #8376600 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 22•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=1fb08e4394a0b
Assignee | ||
Comment 23•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddac6a90ceb6 https://hg.mozilla.org/integration/mozilla-inbound/rev/90c1ec4ae807 https://hg.mozilla.org/integration/mozilla-inbound/rev/509cfc636986
Flags: in-testsuite? → in-testsuite+
Comment 24•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ddac6a90ceb6 https://hg.mozilla.org/mozilla-central/rev/90c1ec4ae807 https://hg.mozilla.org/mozilla-central/rev/509cfc636986
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in
before you can comment on or make changes to this bug.
Description
•