1038294 - [css-display] Implement the multi-keyword syntax for the 'display' property

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Mats Palmgren (inactive)]

This bug is only intended for making 'display' a shorthand, not add any
new values we don't currently support.  (Bug 907396 will add support
for 'contents'.)

Comment 1

[Sebastian Zartner [:sebo]]

> This bug is only intended for making 'display' a shorthand, not add any new values we don't currently support.

Then the title is misleading. How about turning this into a meta-issue and creating new ones for turning 'display' into a shorthand and the new properties blocking this issue?

Sebastian

Comment 2

[Sebastian Zartner [:sebo]]

As asked in comment 1, make this a meta-bug and file individual ones for the different parts it covers?

Sebastian

Comment 3

[Mats Palmgren (inactive)]

The spec has changed again since this bug was filed, re-summarizing accordingly.
'display' is now still a longhand, but it now accepts multiple-keyword values.

Comment 4

[Jen Simmons [:jensimmons]]

CSS Display Level 3 has just gone back to CR.

I would like to see Mozilla implement the new "full display" values fairly quickly — the ones that are simple aliases to preexisting properties. Which is hopefully a fairly trivial amount of work. (There are two new display values, `inline list-item`, aka `inline flow list-item` and `block ruby`. Those can be handled in separate Bugzilla tickets, as they will require more work.)

To experts on CSS, it might not seem like a big deal, to offer the ability to write `display: block grid` instead of `display: grid` in their code, but I'm very excited about this change to CSS. It will make teaching layout much easier. Conceptually, thinking of display-outside and display-inside as two different concepts to master — this is huge. 

It's an especially good time to have this change, since the industry is just now figuring out how to do layout in vanilla CSS, now that we have CSS Grid, Flexbox, et al, and people are letting go of layout frameworks like Bootstrap and Foundation. Many, many people are in the process of learning CSS for layout for the first time. 

I'd love to see this shipped in 2019, and not 2022. 

I plan to teach layout using `display:	[ <display-outside> || <display-inside> ]` immediately, letting people know it doesn't work in any browser yet, but will. It'd be great if that story could change to "works in Firefox".

I would also be helpful if we could write tests for the other browsers to use, speeding up adoption by others.

Comment 5

[Sean Voisen (:svoisen)]

Triaging to get on our backlog for review.

Comment 6

[Sebastian Zartner [:sebo]]

(In reply to Jen Simmons [:jensimmons] from comment #4)
> CSS Display Level 3 has just gone back to CR.
> 
> I would like to see Mozilla implement the new "full display" values fairly
> quickly — the ones that are simple aliases to preexisting properties. Which
> is hopefully a fairly trivial amount of work.

For reference, the spec. has a table with a line-up of single-keyword and multi-keyword values:

https://drafts.csswg.org/css-display/#display-value-summary

> I plan to teach layout using `display:	[ <display-outside> ||
> <display-inside> ]` immediately, letting people know it doesn't work in any
> browser yet, but will. It'd be great if that story could change to "works in
> Firefox".

For what it's worth, that is also what https://developer.mozilla.org/en-US/docs/Web/CSS/display is already doing for quite some time.

Sebastian

Comment 7

[Mats Palmgren (inactive)]

I have a WIP implementing this, so I might as well take it...

Comment 8

[Mats Palmgren (inactive)]

Attachment: Bug 1038294 part 1 - Make nsCSSKTableEntry use uint16_t to represent the value. r=emilio

Comment 9

[Mats Palmgren (inactive)]

Attachment: Bug 1038294 part 2 - Remove FCDATA_FOR_DISPLAY. r=emilio

Depends on D39753

Comment 10

[Mats Palmgren (inactive)]

Attachment: Bug 1038294 part 3 - Remove UNREACHABLE_FCDATA. r=emilio

Depends on D39754

Comment 11

[Mats Palmgren (inactive)]

Attachment: Bug 1038294 part 4 - Move an assertion about display:contents to a place where it's not dead code. r=emilio

Depends on D39755

Comment 12

[Mats Palmgren (inactive)]

Attachment: Bug 1038294 part 5 - Move an assertion about '-moz-{inline-}box'. r=emilio

Depends on D39756

Comment 13

[Mats Palmgren (inactive)]

Attachment: Bug 1038294 part 6 - Simplify FindDisplayData by removing a now unnecessary lambda. r=emilio

Depends on D39757

Comment 14

[Mats Palmgren (inactive)]

Attachment: Bug 1038294 part 7 - Move XUL variants of '-moz-{inline-}box' from FindXULDisplayData to FindDisplayData. r=emilio

Depends on D39758

Comment 15

[Mats Palmgren (inactive)]

Attachment: Bug 1038294 part 8 - Move block box creation inside the switch together with all the other display types. r=emilio

Depends on D39759

Comment 16

[Mats Palmgren (inactive)]

Attachment: Bug 1038294 part 9 - Move scroll box suppression inside the switch. r=emilio

Depends on D39760

Comment 17

[Mats Palmgren (inactive)]

Attachment: Bug 1038294 part 10 - Remove dead code that handles display:none/contents which shouldn't reach FindDisplayData. r=emilio

Depends on D39761

Comment 18

[Mats Palmgren (inactive)]

Attachment: Bug 1038294 part 11 - Remove FindXULDisplayData and handle all display types in FindDisplayData instead. r=emilio

Depends on D39762

Comment 19

[Mats Palmgren (inactive)]

Attachment: Bug 1038294 part 12 - Implement multi-keyword 'display' values for Gecko. r=emilio

Depends on D39763

Comment 20

[Mats Palmgren (inactive)]

Attachment: Bug 1038294 part 13 - Add a few accessors to query the parts of a 'display' value. r=emilio

Depends on D39764

Comment 21

[Mats Palmgren (inactive)]

Attachment: Bug 1038294 part 14 - Use only the DisplayInside() value to decide which type of frame to create. r=emilio

Depends on D39765

Comment 22

[Mats Palmgren (inactive)]

Attachment: Bug 1038294 part 15 - Change a few existing nsStyleDisplay accessors to use DisplayInside()/DisplayOutside(). r=emilio

Depends on D39766

Comment 23

[Mats Palmgren (inactive)]

Attachment: Bug 1038294 part 16 - WPT updates for new 'display' values. r=emilio

Depends on D39767

Comment 24

[Mats Palmgren (inactive)]

FYI, most of these parts are idempotent, but I figured it'd be easier to review them in smaller steps...
Part 12/14/15 is where the meat is. Also, these patches adds no new display values per se, only new syntax aliases for existing box types. (new values will be added separately in the blocked bugs)

Comment 25

[Mats Palmgren (inactive)]

... also, the bugs that adds inline list-items and 'block ruby' will cleanup the parse/toCSS code a bit, FYI.

Comment 26

[Mats Palmgren (inactive)]

BTW, I've tried to mark all the Rust changes with the appropriate #[cfg(feature = "gecko"/"servo")]but I haven't tried building Servo with any of these changes...

Comment 27

[ExE Boss]

Can parts 1 through 11 land now that they’re accepted, before parts 12 through 16 get finished?

Comment 28

[Rachel Andrew]

I've been assigned the dev docs task for this, as something that will ship in 69. Just wanted to check before starting that it is shipping in 69, and whether there are any parts that might not, anything I need to flag as a note, or against the BCD for the feature.

Comment 29

[Sean Voisen (:svoisen)]

Hi Rachel: This should be landing in 70, not 69. We are not planning to uplift it to the 69 beta. I'll leave it to Mats to comment on parts that may not make it.

Comment 30

[Mats Palmgren (inactive)]

This and bug 1105868 and bug 1557825 are for v70. I'll land these in a bit and send an intent-to-ship.
I don't know what BCD means, but the changes follows the spec and are complete with tests. No particular notes. Please let me know if anything is unclear.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/17ca7eaa9dba
part 1 - Make nsCSSKTableEntry use uint16_t to represent the value.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/a101b9693ad6
part 2 - Remove FCDATA_FOR_DISPLAY.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/fd0c240fd715
part 3 - Remove UNREACHABLE_FCDATA.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/f3c8731da1e5
part 4 - Move an assertion about display:contents to a place where it's not dead code.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/98aff703294e
part 5 - Move an assertion about '-moz-{inline-}box'.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/6a1f7235c5a9
part 6 - Simplify FindDisplayData by removing a now unnecessary lambda.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/f4280b135e7a
part 7 - Move XUL variants of '-moz-{inline-}box' from FindXULDisplayData to FindDisplayData.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/6bb15997aba7
part 8 - Move block box creation inside the switch together with all the other display types.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/127c7dfc011a
part 9 - Move scroll box suppression inside the switch.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/62b047361030
part 10 - Remove dead code that handles display:none/contents which shouldn't reach FindDisplayData.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/ec8f17f71b28
part 11 - Remove FindXULDisplayData and handle all display types in FindDisplayData instead.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/965099bbb4a6
part 12 - Implement multi-keyword 'display' values for Gecko.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/6a674140cb8f
part 13 - Add a few accessors to query the parts of a 'display' value.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/6c19f13a6082
part 14 - Use only the DisplayInside() value to decide which type of frame to create.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/f919a62c86d1
part 15 - Change a few existing nsStyleDisplay accessors to use DisplayInside()/DisplayOutside().  r=emilio
https://hg.mozilla.org/integration/autoland/rev/57bcf2eea109
part 16 - WPT updates for new 'display' values.  r=emilio

Comment 32

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/18422 for changes under testing/web-platform/tests

Comment 33

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 34

[Mats Palmgren (inactive)]

Intent-to-ship:
https://groups.google.com/forum/#!topic/mozilla.dev.platform/ddztUGUpYbE

Comment 35

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/17ca7eaa9dba
https://hg.mozilla.org/mozilla-central/rev/a101b9693ad6
https://hg.mozilla.org/mozilla-central/rev/fd0c240fd715
https://hg.mozilla.org/mozilla-central/rev/f3c8731da1e5
https://hg.mozilla.org/mozilla-central/rev/98aff703294e
https://hg.mozilla.org/mozilla-central/rev/6a1f7235c5a9
https://hg.mozilla.org/mozilla-central/rev/f4280b135e7a
https://hg.mozilla.org/mozilla-central/rev/6bb15997aba7
https://hg.mozilla.org/mozilla-central/rev/127c7dfc011a
https://hg.mozilla.org/mozilla-central/rev/62b047361030
https://hg.mozilla.org/mozilla-central/rev/ec8f17f71b28
https://hg.mozilla.org/mozilla-central/rev/965099bbb4a6
https://hg.mozilla.org/mozilla-central/rev/6a674140cb8f
https://hg.mozilla.org/mozilla-central/rev/6c19f13a6082
https://hg.mozilla.org/mozilla-central/rev/f919a62c86d1
https://hg.mozilla.org/mozilla-central/rev/57bcf2eea109

Comment 36

[ExE Boss]

Attachment: MDN: Browser compatibility table update

Comment 37

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Nit: this patch stack seems to have left some files in a non-clang-format-clean state, which means some downstream commit can end up inadvertently incorporating the reformatting cleanup automatically (and if it's even noticed, it can be hard to prevent that from happening -- see e.g. bug 1556088 for some of the trouble that this can cause).

In this case, https://hg.mozilla.org/integration/autoland/rev/4d8eb840fc2eaa37f6e75e5a5f2e8863beeccf19 ended up auto-reformatting to clean up some nsCSSFrameConstructor.cpp formatting from this patch-stack here. That happened when I imported/rebased a patch, without me noticing, as part of a local commit hook set up by our mercurial extensions/tools. This is partly my fault for not noticing the reformatting leaking in there, but it's also a good reminder to keep an eye out for reviewbot notes like https://phabricator.services.mozilla.com/D39758#1263097 and to apply this reformatting up-front (or soon after the fact) so that latent reformatting changes don't automatically leak into later patches by accident.

Comment 38

[Daniel Holbert [:dholbert] (PTO until 2/24)]

BTW, I ended up backing out the inadvertently-reformatting-laden commit referenced in comment 37, and I flushed all latent reformatting in layout in bug 1574310. So everything should be good now (at least in the layout directory).