Implement the CSS Display 'display-box' property

RESOLVED WONTFIX

Status

()

enhancement
RESOLVED WONTFIX
6 years ago
5 years ago

People

(Reporter: mats, Unassigned)

Tracking

({css3, feature})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [css-display])

Attachments

(10 attachments, 9 obsolete attachments)

18.74 KB, patch
mats
: review+
Details | Diff | Splinter Review
2.53 KB, patch
mats
: review+
Details | Diff | Splinter Review
10.76 KB, patch
mats
: review+
Details | Diff | Splinter Review
4.03 KB, patch
mats
: review+
Details | Diff | Splinter Review
36.54 KB, patch
mats
: review+
Details | Diff | Splinter Review
7.00 KB, patch
mats
: review+
Details | Diff | Splinter Review
19.32 KB, patch
mats
: review+
Details | Diff | Splinter Review
16.75 KB, patch
mats
: review+
Details | Diff | Splinter Review
63.22 KB, patch
Details | Diff | Splinter Review
220.05 KB, patch
Details | Diff | Splinter Review
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.
I tend to think this isn't ready for implementation, spec-wise, unless we have a real need for it.
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.
Posted patch Style system changes. (obsolete) — Splinter Review
Attachment #808133 - Flags: review?(dbaron)
Posted patch Flexbox changes. (obsolete) — Splinter Review
Attachment #808134 - Flags: review?(dholbert)
My apologies for wasting your time reviewing this in bug 907396 only to
remove it here.
Attachment #808135 - Flags: review?(dholbert)
I've divided the frame construction changes into two patches, this part mostly
deals with moving display:contents to box:contents.
Attachment #808136 - Flags: review?(bzbarsky)
and this part changes some display:none checks to box:none checks.
(there's a one-line editor change here as well)
Attachment #808137 - Flags: review?(bzbarsky)
This is an automated 'sed -i' change so no need review that deeply.
Attachment #808138 - Flags: review?(bzbarsky)
Trivial white-space changes only.
Attachment #808139 - Flags: review?(bzbarsky)
(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.
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.
Attachment #808135 - Flags: review?(dholbert) → review+
Attachment #808134 - Flags: review?(dholbert) → review+
Comment on attachment 808136 [details] [diff] [review]
Frame constructor changes for display:contents -> box:contents.

r=me
Attachment #808136 - Flags: review?(bzbarsky) → review+
Comment on attachment 808137 [details] [diff] [review]
Additional changes to implement the 'box' property.

r=me
Attachment #808137 - Flags: review?(bzbarsky) → review+
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
Attachment #808138 - Flags: review?(bzbarsky) → review+
Comment on attachment 808139 [details] [diff] [review]
A few manual indentation fixups after renaming DisplayContents -> BoxContents.

r=me
Attachment #808139 - Flags: review?(bzbarsky) → review+
Comment on attachment 808140 [details] [diff] [review]
Replace 'display:contents' w. 'box:contents' in comments/asserts.

r=me
Attachment #808140 - Flags: review?(bzbarsky) → review+
Keywords: feature
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?
Attachment #808133 - Flags: review?(dbaron) → review+
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.
Summary: Implement the CSS Display 'box' property → Implement the CSS Display 'display-box' property
Attachment #808133 - Attachment is obsolete: true
Attachment #8392956 - Flags: review+
(folded in some changes from bug 907396)
Attachment #808134 - Attachment is obsolete: true
Attachment #8392961 - Flags: review+
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)
Attachment #808135 - Attachment is obsolete: true
Attachment #808136 - Attachment is obsolete: true
Attachment #8392963 - Flags: review+
Attachment #808141 - Attachment is obsolete: true
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.
Assignee: mats → nobody
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.
Summary: Implement the CSS Display 'display-box' property → Implement the CSS Display 'box-suppress' property
Actually, I filed a new bug 1070507 for the new property instead.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX
Summary: Implement the CSS Display 'box-suppress' property → Implement the CSS Display 'display-box' property
You need to log in before you can comment on or make changes to this bug.