Closed Bug 969460 Opened 7 years ago Closed 7 years ago

<html> (root element) won't accept 'display:flex'

Categories

(Core :: Layout, defect, P4)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla30

People

(Reporter: lcid-fire, Assigned: dholbert)

References

(
URL
)

Details

Attachments

(5 files, 2 obsolete files)

reduced testcase
532 bytes, text/html
Details
reference case
559 bytes, text/html
Details
part 1: Make EnsureBlockDisplay behavior on 'list-item' customizable
2.99 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
part 2 (addressed review comments)
4.99 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
part 3: tests
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.
Component: Untriaged → Layout
OS: Linux → All
Product: Firefox → Core
Hardware: x86_64 → All
Version: 26 Branch → Trunk
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.
URL: https://abergmeier.makes.org/thimble/...
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)
Attached file reduced testcase — 
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.
(er, by "blue outline" I meant "blue region")
(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>.)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached file [wrong file] (obsolete) — 
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.
Attachment #8372591 - Attachment description: reduced testcase → [wrong file]
Attachment #8372591 - Attachment is obsolete: true
Attached file reference case — 
[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.]
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'
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'
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: nobody → dholbert
Note also that nsCSSFrameConstructor::ConstructDocElementFrame makes assumptions...
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#2369
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.
Priority: -- → P4
Summary: <html> won't accept 'display:flex' → <html> (root element) won't accept 'display:flex'
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)
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)
[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
Still need to add reftests, too. (I intend to get to that tomorrow morning.)
Flags: in-testsuite?
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 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+
(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.)
(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+
Attachment #8375250 - Attachment is obsolete: true
Attached patch part 3: tests — — Splinter Review 
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 on attachment 8376600 [details] [diff] [review]
part 3: tests

r=me
Attachment #8376600 - Flags: review?(bzbarsky) → review+
Depends on: 973390
You need to log in before you can comment on or make changes to this bug.