Closed Bug 1038294 Opened 7 years ago Closed 2 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: mats, Assigned: mats)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete, DevAdvocacy, Whiteboard: [DevRel:P1] [layout:backlog][wptsync upstream])

Attachments

(17 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
52 bytes, text/x-github-pull-request
Details | Review
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'.)
No longer blocks: 907396
> 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
As asked in comment 1, make this a meta-bug and file individual ones for the different parts it covers?

Sebastian
Flags: needinfo?(mats)
Blocks: 1105868
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.
Flags: needinfo?(mats)
Summary: [css-display] Implement 'display-outside', 'display-inside' and 'display-extras' properties. Make 'display' a shorthand for those. → [css-display] Implement the multi-keyword syntax for the 'display' property
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.
Keywords: DevAdvocacy
Whiteboard: [DevRel:P1]
Triaging to get on our backlog for review.
Priority: -- → P3
(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
Whiteboard: [DevRel:P1] → [DevRel:P1] [layout:backlog]

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

Assignee: nobody → mats
Blocks: 1557825

Depends on D39753

Depends on D39754

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)

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

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...

Blocks: 1569825

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

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.

Flags: needinfo?(mats)

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.

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.

Flags: needinfo?(mats)
Pushed by mpalmgren@mozilla.com:
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/18422 for changes under testing/web-platform/tests
Whiteboard: [DevRel:P1] [layout:backlog] → [DevRel:P1] [layout:backlog][wptsync upstream]
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

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.

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).

You need to log in before you can comment on or make changes to this bug.