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

RESOLVED FIXED in mozilla30

Status

()

Product:
Core
Component:
Layout
Type:
defect
Importance:
P4
normal
Status:
RESOLVED FIXED
Opened:
6 years ago
Updated:
5 years ago

People

(Reporter: lcid-fire, Assigned: dholbert)

Tracking

Version:
Trunk
Target:
mozilla30
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(
URL
)

Attachments

(5 attachments, 2 obsolete attachments)

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
Reporter

Description

•
6 years ago 
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

•
6 years ago
Component: Untriaged → Layout
OS: Linux → All
Product: Firefox → Core
Hardware: x86_64 → All
Version: 26 Branch → Trunk
 Assignee

Comment 1

•
6 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.
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)
 Assignee

Comment 2

•
6 years ago
Posted 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.
 Assignee

Comment 3

•
6 years ago 
(er, by "blue outline" I meant "blue region")
 Assignee

Comment 4

•
6 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

•
6 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
 Assignee

Comment 5

•
6 years ago
Posted 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.
 Assignee

Updated

•
6 years ago
Attachment #8372591 - Attachment description: reduced testcase → [wrong file]
Attachment #8372591 - Attachment is obsolete: true
 Assignee

Comment 6

•
6 years ago
Posted 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.]
 Assignee

Comment 7

•
5 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

•
5 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

•
5 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

•
5 years ago
Assignee: nobody → dholbert

Comment 10

•
5 years ago 
Note also that nsCSSFrameConstructor::ConstructDocElementFrame makes assumptions...
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#2369
 Assignee

Comment 11

•
5 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

•
5 years ago
Priority: -- → P4
Summary: <html> won't accept 'display:flex' → <html> (root element) won't accept 'display:flex'
 Assignee

Comment 12

•
5 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

•
5 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

•
5 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

•
5 years ago 
Still need to add reftests, too. (I intend to get to that tomorrow morning.)
Flags: in-testsuite?

Comment 16

•
5 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

•
5 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

•
5 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

•
5 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

•
5 years ago
Attachment #8375250 - Attachment is obsolete: true
 Assignee

Comment 20

•
5 years ago
Posted 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 21

•
5 years ago 
Comment on attachment 8376600 [details] [diff] [review]
part 3: tests

r=me
Attachment #8376600 - Flags: review?(bzbarsky) → review+

Updated

•
5 years ago
Depends on: 973390
You need to log in before you can comment on or make changes to this bug.