1105868 - Support 'display: inline list-item' and 'display: inline flow-root list-item'

Status: RESOLVED FIXED

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

Description

[Xidorn Quan [:xidorn] UTC+11]

Per CSS Display 3, list-item should be able to be inlinized to inline-list-item like other block-level box.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: bz://1105868/xidorn

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

/r/1301 - Bug 1105868 - Add inline-list-item support.

Pull down this commit:

hg pull review -r 47405827e464aacd3eb88d30cf5d7bbb7869bfee

Comment 4

[Cameron McCormack (:heycam)]

https://reviewboard.mozilla.org/r/1299/#review1191

r=me for the layout/style/ parts, with the comment added and perhaps moving the list-item-position computation adjustment to nsRuleNode, depending on what dbaron thinks.  The rest should be reviewed by roc or dbaron.

::: layout/style/nsRuleNode.cpp
(Diff revision 1)
> +    display = aConvertListItem ?

Can you update the comment at the top of this function to say that aConvertListItem affects inline-list-item too?

::: layout/style/nsStyleContext.cpp
(Diff revision 1)
> +  // Correct inline list item

I think this is OK but I wonder whether this would be better in nsRuleNode::ComputeListData, setting canStoreInRuleTree to false if it looks up display and finds it is inline-list-item.  But there there are no other display-based computed value adjustments currently done in nsRuleNode::ComputeBlahData, so I might be missing something.  dbaron?

Comment 5

[Cameron McCormack (:heycam)]

(dbaron: question in comment 4; reviewboard should give more context.)

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

https://reviewboard.mozilla.org/r/1299/#review1193

> I think this is OK but I wonder whether this would be better in nsRuleNode::ComputeListData, setting canStoreInRuleTree to false if it looks up display and finds it is inline-list-item.  But there there are no other display-based computed value adjustments currently done in nsRuleNode::ComputeBlahData, so I might be missing something.  dbaron?

I guess it is necessary to put this here, because the display value could just be changed in this function (list-item <=> inline-list-item in either direction). In those cases, I suspect nsRuleNode::ComputeListData could miss the change.

Comment 7

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

So, a general comment:  this patch looks like it's taking the wrong approach.  Te spec looks like it defines inline-list-item as an inline with a bullet, but this patch appears to implement it as an inline-block with a bullet.  Implementing what the spec says is a good bit more work, since we don't support bullets on inlines.  (It would probably be a good bit less work if we do bug 288704 first.)

I tend to think that work probably isn't a good use of time right now, especially since it's not clear that either of these specs (http://dev.w3.org/csswg/css-display-3/ or http://dev.w3.org/csswg/css-lists/ ) are in a ready-to-implement state.

(In reply to Cameron McCormack (:heycam) from comment #4)
> I think this is OK but I wonder whether this would be better in
> nsRuleNode::ComputeListData, setting canStoreInRuleTree to false if it looks
> up display and finds it is inline-list-item.  But there there are no other
> display-based computed value adjustments currently done in
> nsRuleNode::ComputeBlahData, so I might be missing something.  dbaron?

I can't think of anything *wrong* with it other than introducing a slightly increased potential for a struct computation cycle.  But I think it's probably ok for list to depend on display, since I think it's unlikely for display to depend on list.

There is a big disadvantage, though, which is that canStoreInRuleTree would have to be set to false whenever 'list-style-position' was 'outside', which is the initial value, because that would be needed to force recomputation (for a potentially different display value on a different style context) for a different style context sharing the same rule node.

So I think it's probably best as it is.  The test does happen on every style context creation, but it tests on a rare condition.

(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #6)
> I guess it is necessary to put this here, because the display value could
> just be changed in this function (list-item <=> inline-list-item in either
> direction). In those cases, I suspect nsRuleNode::ComputeListData could miss
> the change.

I don't see how a change could be missed.  If you set canStoreInRuleTree to false, then the struct will be cached on the style context.  Since style contexts are immutable, there's no later change that could happen.  However, there is the problem above about another style context with a different display value sharing the same rule node.

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1105868 - Add inline-list-item support.

Comment 10

[yisibl]

This specification has been defined in detail.

https://drafts.csswg.org/css-lists/#valdef-display-inline-list-item

Comment 11

[yisibl]

Update MDN docs https://developer.mozilla.org/en-US/docs/Web/CSS/display

Comment 12

[Oriol Brufau [:Oriol]]

inline-list-item has been dropped from the spec per CSSWG resolution.
https://github.com/w3c/csswg-drafts/commit/94b7b00c51be31196521f73543ed5b4bd63486d2
https://lists.w3.org/Archives/Public/www-style/2017Jul/0015.html

Comment 13

[Mats Palmgren (inactive)]

For clarity, "display:inline list-item" is still in the spec.
https://drafts.csswg.org/css-display/#propdef-display
It was only the single-value 'inline-list-item' that was dropped.
Which means this bug depends on bug 1038294.

Comment 14

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Mats Palmgren (:mats) from comment #13)
> For clarity, "display:inline list-item" is still in the spec.
> https://drafts.csswg.org/css-display/#propdef-display
> It was only the single-value 'inline-list-item' that was dropped.
> Which means this bug depends on bug 1038294.

FWIW, I don't think it's necessary to have display subproperties as a prerequisite for inline list-item.

We can implement its layout part and only expose it via inlinification for now. Since ruby base / text are atomic themselves anyway, inline list-item inside them shouldn't really create anything useful.

Comment 15

[Mats Palmgren (inactive)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #14)
> FWIW, I don't think it's necessary to have display subproperties as a
> prerequisite for inline list-item.

I think it's required because inlinification changes the computed value
to 'inline list-item' so that's how the value should be serialized,
but we shouldn't serialize values that we can't parse.

Comment 16

[Mats Palmgren (inactive)]

I have a WIP for this built on top of my WIP for bug 1038294, so I might as well take this...

Comment 17

[Mats Palmgren (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7591fb2fd319a0925fc2ff7daaada3499af61ba

Comment 18

[Mats Palmgren (inactive)]

Attachment: Bug 1105868 part 1 - Use nsStyleDisplay::IsListItem() in a few places to prepare for inline list-items. r=emilio

Comment 19

[Mats Palmgren (inactive)]

Attachment: Bug 1105868 part 2 - Use nsStyleDisplay::DisplayInside() in a few places to prepare for inline list-items. r=emilio

Depends on D39830

Comment 20

[Mats Palmgren (inactive)]

Attachment: Bug 1105868 part 3 - Implement 'inline list-item' and 'inline flow-root list-item' values for the 'display' property. r=emilio

Depends on D39831

Comment 21

[Mats Palmgren (inactive)]

Attachment: Bug 1105868 part 4 - Accessibility support for inline list-items. r=emilio

Depends on D39832

Comment 22

[Mats Palmgren (inactive)]

Attachment: Bug 1105868 part 5 - Add / update tests for inline list-item 'display' values. r=emilio

Depends on D39833

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9651955468f8
part 1 - Use nsStyleDisplay::IsListItem() in a few places to prepare for inline list-items.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/7f1a906285fc
part 2 - Use nsStyleDisplay::DisplayInside() in a few places to prepare for inline list-items.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/b517c3b51b9a
part 3 - Implement 'inline list-item' and 'inline flow-root list-item' values for the 'display' property.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/8647528a4e9d
part 4 - Accessibility support for inline list-items.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/bc6ca73b757e
part 5 - Add / update tests for inline list-item 'display' values.  r=emilio

Comment 24

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

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

Comment 25

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

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

Comment 26

[Mats Palmgren (inactive)]

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

Comment 27

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/9651955468f8
https://hg.mozilla.org/mozilla-central/rev/7f1a906285fc
https://hg.mozilla.org/mozilla-central/rev/b517c3b51b9a
https://hg.mozilla.org/mozilla-central/rev/8647528a4e9d
https://hg.mozilla.org/mozilla-central/rev/bc6ca73b757e