Closed Bug 1105868 Opened 5 years ago Closed 8 months ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: xidorn, Assigned: mats)

References

()

Details

(Keywords: dev-doc-complete)

Attachments

(5 files, 3 obsolete files)

Per CSS Display 3, list-item should be able to be inlinized to inline-list-item like other block-level box.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → quanxunzhen
Attachment #8531880 - Flags: review?(cam)
Attached file MozReview Request: bz://1105868/xidorn (obsolete) —
/r/1301 - Bug 1105868 - Add inline-list-item support.

Pull down this commit:

hg pull review -r 47405827e464aacd3eb88d30cf5d7bbb7869bfee
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?
(dbaron: question in comment 4; reviewboard should give more context.)
Flags: needinfo?(dbaron)
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.
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.
Flags: needinfo?(dbaron)
Attachment #8531880 - Flags: review?(cam)
Attachment #8533135 - Attachment is obsolete: true
This specification has been defined in detail.

https://drafts.csswg.org/css-lists/#valdef-display-inline-list-item
Attachment #8618754 - Attachment is obsolete: true
Assignee: quanxunzhen → nobody
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.
Depends on: 1038294
Summary: Support display: inline-list-item → Support display: inline list-item
(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.
(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.
Attachment #8531880 - Attachment is obsolete: true

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

Assignee: nobody → mats
Type: defect → enhancement
Component: Layout → CSS Parsing and Computation
Priority: -- → P3
Summary: Support display: inline list-item → Support 'display: inline list-item' and 'display: inline flow-root list-item'
Pushed by mpalmgren@mozilla.com:
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/18424 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Depends on: 1574060
Regressions: 1574123
You need to log in before you can comment on or make changes to this bug.