917313 - Implement the CSS Display 'display-box' property

Status: RESOLVED WONTFIX

(Core :: Layout, enhancement)

Description

[Mats Palmgren (inactive)]

Implement the CSS Display 'box' property per:
http://dev.w3.org/csswg/css-display-3/#the-display-box

The 'box:contents' part is implemented in bug 907396 so that's mostly
just renaming stuff.  The 'box:none' part is slightly trickier in the
case 'display' is not 'none' since we need to change some existing
'display' checks to instead check 'box'.  But overall it seems quite
easy to implement after bug 907396 so I think I'll do this first (before
overflow:fragments) so it can be shipped as a feature.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I tend to think this isn't ready for implementation, spec-wise, unless we have a real need for it.

Comment 2

[Mats Palmgren (inactive)]

OK, it will still be disabled by default of course.  I do think it's a cleaner way
to implement the display:contents feature though since we can avoid messing with 'display'
code internally, e.g. EnsureBlockDisplay(), since 'box' is orthogonal to that (the only
interaction is the other way, display:none implies box:none), at least that's how I
understand the intent of the spec so far.

Comment 3

[Mats Palmgren (inactive)]

Attachment: Style system changes.

Comment 4

[Mats Palmgren (inactive)]

Attachment: Flexbox changes.

Comment 5

[Mats Palmgren (inactive)]

Attachment: Add "layout.css.box.enabled" bool pref to control support for the 'box' property (disabled by default). Remove the pref for obsoleted 'display:contents'.

My apologies for wasting your time reviewing this in bug 907396 only to
remove it here.

Comment 6

[Mats Palmgren (inactive)]

Attachment: Frame constructor changes for display:contents -> box:contents.

I've divided the frame construction changes into two patches, this part mostly
deals with moving display:contents to box:contents.

Comment 7

[Mats Palmgren (inactive)]

Attachment: Additional changes to implement the 'box' property.

and this part changes some display:none checks to box:none checks.
(there's a one-line editor change here as well)

Comment 8

[Mats Palmgren (inactive)]

Attachment: Automated renaming DisplayContents -> BoxContents.

This is an automated 'sed -i' change so no need review that deeply.

Comment 9

[Mats Palmgren (inactive)]

Attachment: A few manual indentation fixups after renaming DisplayContents -> BoxContents.

Trivial white-space changes only.

Comment 10

[Mats Palmgren (inactive)]

Attachment: Replace 'display:contents' w. 'box:contents' in comments/asserts.

Comment 11

[Mats Palmgren (inactive)]

Attachment: Updated reftests.

https://tbpl.mozilla.org/?tree=Try&rev=26491426f5c5
https://tbpl.mozilla.org/?tree=Try&rev=9881769dd206

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #5)
> My apologies for wasting your time reviewing this in bug 907396 only to
> remove it here.

No worries; it's just 2 lines. :)

Question on that topic, though -- given that most of the patches over in bug 907396 still have yet to be reviewed, and their added code is (I think?) largely being rewritten in this bug, I wonder whether it'd make more sense to just skip that intermediate step of bug 907396 entirely?  Sometimes intermediate steps can add value and reduce complexity, but I don't see that being the case here, given that the code-rewriting patches are already done and ready for review.

(Effectively, that'd mean just folding those patches into these ones, I guess...  Might be a bit tricky to make sure everything ends up in the write patch, but hopefully not too bad.)

Might not be a big enough problem to worry about - just seems like a question worth asking.

Comment 13

[Mats Palmgren (inactive)]

I figured it wouldn't matter much since the hard part to review is the actual mechanism
of display:contents, not under which name it's implemented.  The patches in this bug
is mostly just naming changes, and a little bit extra to also implement box:none.
(granted, the preference part was a bit back-and-forth)

But sure, if the reviewers would like any of the patches folded together in some way
then I will be happy to do that.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 808136 [details] [diff] [review]
Frame constructor changes for display:contents -> box:contents.

r=me

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 808137 [details] [diff] [review]
Additional changes to implement the 'box' property.

r=me

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 808138 [details] [diff] [review]
Automated renaming DisplayContents -> BoxContents.

>+    for (nsIContent* p = aParent; p && fm->GetBoxContents(p);

GetBoxContents still needs a better name, like I said in the other bug.

r=me

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 808139 [details] [diff] [review]
A few manual indentation fixups after renaming DisplayContents -> BoxContents.

r=me

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 808140 [details] [diff] [review]
Replace 'display:contents' w. 'box:contents' in comments/asserts.

r=me

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 808133 [details] [diff] [review]
Style system changes.

r=dbaron (and sorry again for the delay) -- I presume there are more changes from checking mDisplay to checking mBox in other patches here?

Comment 20

[Mats Palmgren (inactive)]

New set of patches coming up, primarily changing the property name from 'box' to 'display-box',
and addressing a couple of naming/documentation nits from bug 907396.  Also, adding a whole
bunch of additional tests.  No need for new reviews unless you want to.

Comment 21

[Mats Palmgren (inactive)]

Attachment: Style system changes.

Comment 22

[Mats Palmgren (inactive)]

Attachment: Flexbox changes.

(folded in some changes from bug 907396)

Comment 23

[Mats Palmgren (inactive)]

Comment on attachment 808135 [details] [diff] [review]
Add "layout.css.box.enabled" bool pref to control support for the 'box' property (disabled by default).  Remove the pref for obsoleted 'display:contents'.

(the pref now lives in the "Style system changes" patch, the rest is obsolete)

Comment 24

[Mats Palmgren (inactive)]

Attachment: Frame constructor changes for display-box:contents.

Comment 25

[Mats Palmgren (inactive)]

Attachment: Additional changes to implement the 'display-box' property.

Comment 26

[Mats Palmgren (inactive)]

Attachment: Automated renaming DisplayContents -> DisplayBoxContents.

Comment 27

[Mats Palmgren (inactive)]

Attachment: A few manual indentation fixups after renaming DisplayContents -> DisplayBoxContents.

Comment 28

[Mats Palmgren (inactive)]

Attachment: Rename GetDisplayBoxContents to GetDisplayBoxContentsStyleFor.

Comment 29

[Mats Palmgren (inactive)]

Attachment: Replace 'display:contents' w. 'display-box:contents' in comments/asserts.

Comment 30

[Mats Palmgren (inactive)]

Attachment: Tests for the CSS 'display-box' property.

Comment 31

[Mats Palmgren (inactive)]

Attachment: Rollup patch, including changes in bug 907396. (wdiff)

https://tbpl.mozilla.org/?tree=Try&rev=0884a4357271
https://tbpl.mozilla.org/?tree=Try&rev=ea86adde5713

Comment 32

[Mats Palmgren (inactive)]

There's a proposal from fantasai/Tab to rename 'display-box' and move
'contents' to 'display-outside' instead:
http://lists.w3.org/Archives/Public/www-style/2014Jul/0023.html

Assuming that proposal is accepted, we should probably split up
these patches so that the 'contents' related parts goes to
bug 907396 and create a new bug for the 'box-suppress' part
(and close this bug).

It doesn't seem worth pursuing this until the spec situation is clear.

Comment 33

[Mats Palmgren (inactive)]

It's now called 'box-suppress':
http://dev.w3.org/csswg/css-display/#box-suppress

The patches on this bug are obsolete.
I intend to implement and land display:contents first, in bug 907396.

Comment 34

[Mats Palmgren (inactive)]

Actually, I filed a new bug 1070507 for the new property instead.