Skip to content

[css-lists] Should list marker affects line height? #2418

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
kojiishi opened this issue Mar 7, 2018 · 10 comments
Open

[css-lists] Should list marker affects line height? #2418

kojiishi opened this issue Mar 7, 2018 · 10 comments
Labels
css-lists-3 Current Work css-lists-4

Comments

@kojiishi
Copy link
Contributor

kojiishi commented Mar 7, 2018

This is for info-sharing as we worked on re-implementing lists in Blink.

Test@codepen

At least from this test, Edge/WebKit/Blink includes the list marker to the line height, while Gecko does not.

/cc @FremyCompany @upsuper @cathiechen
@kojiishi kojiishi added the css-lists-3 Current Work label Mar 7, 2018
aarongable pushed a commit to chromium/chromium that referenced this issue Mar 8, 2018
@kojiishi
[LayoutNG] Fix list marker positioning when the first child is block
02b2c6f 
This patch fixes list marker positioning when the first child of
the list item is block-level, and the child does not produce a
baseline. Before this patch, list marker is positioned at the
synthesized baseline. This is not well-defined, but 3 browsers
positions the list marker at the first child that has a baseline.
This is filed to CSS WG at
w3c/csswg-drafts#2417.

By doing so, a new case appears where there are no children that
has a baseline. This case is also not well-defined and non-
interoperable. Blink/Edge/WebKit creates an empty line box in
this case, but this isn't easy as we have to insert an empty
line box before the children we have already laid out. This
patch follows Gecko behavior to position the list marker at the
top of the list item.

Also, whether list marker should affect block size or not is not
defined and not interoperable. For now, this patch includes the
list marker block size into list item's block size because it
looks more reasonable to me, and Blink/Edge/WebKit do so. This
is filed as:
w3c/csswg-drafts#2418

SetListMarkerPosition() is a left over in the last patch and
therefore this patch removed it.

Some tests turn to good, though they need to rebaseline due to
list marker image differences.

Bug: 725277
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_layout_ng
Change-Id: If47ab7daea214c0593a917a0c8f505274dae5ca2
Reviewed-on: https://chromium-review.googlesource.com/952843
Commit-Queue: Koji Ishii <kojii@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541864}
@FremyCompany
Copy link
Contributor

FremyCompany commented Mar 10, 2018 •
edited
Loading

I'd say yes. The css 2.1 spec says it should.

The only case that doesn't make sense is when Edge is creating a line just to host the bullet if there is no existing line where the place the bullet and we encounter a list-item content that is a bfc and inside which we cannot insert the bullet. The empty line is very weird and not expected by authors.

@kojiishi
Copy link
Contributor Author

I'd say yes. The css 2.1 spec says it should.

I found in the outside value of list-style-position:

The size or contents of the marker box may affect the height of the principal block box and/or the height of its first line box, and in some cases may cause the creation of a new line box.

but this is "may". Is there other definitions?

The only case that doesn't make sense is...

Is this the case about the last example of tests@codepen in #2417, or is this different?

@FremyCompany
Copy link
Contributor

Is there other definitions?

Except the small design doc I presented to the group, no unfortunately.

Is this the case about the last example of tests@codepen in #2417, or is this different?

The last case indeed. I propose it renders the more or less as it does in Edge now for the bullet position (1. exactly where it is now in Edge) but there would be no white space for the unnecessary line above the blue div.

@cathiechen
Copy link

Changed Koji's test case a bit https://jsbin.com/cenibuvawe/3/edit?html,css,output. The behavior is wierd except Gecko.

If we let marker affect the line height of the content block. I'm afraid this might make the behavior of the content block unpredictable. And this might confuse web developers.

Back to the in-flow or out-of-flow issue:
If we treat list marker as in-flow: Accord to the behavior of in-flow, marker should be affected by float object, and it could affect the height of list item but not the content block of list item.
If out-of-flow: marker shouldn't be affected by float object, and it shouldn't change the height of list item or the content block.

Looks like:
Gecko is like out-of-flow more, but let float objects affect the position of marker.
Edge/WebKit/Blink is like in-flow more, but let marker affect the content block.

@cathiechen
Copy link

I personally think the ideal result is that:

  1. Marker is in-flow.
  2. It could be affected by float objects.
  3. It doesn't affect the content block. But it could affect the height of list item.
  4. It always could be displayed.

@FremyCompany
Copy link
Contributor

Changed Koji's test case a bit https://jsbin.com/cenibuvawe/3/edit?html,css,output. The behavior is wierd except Gecko.

Maybe, but the layout constraints here are not very realistic. This is seriously over-constrained, and uses font sizes differing by an order of magnitude.

I would rather have a solution that looks like what we have now and makes sense in the reasonable cases than try to solve an infinite amount of cases and create a difficult-to-maintain code.

We already have something that is solid in most cases, with little developer complaints and very few broken sites. So, from Edge point of view, we just want to move the needle a bit towards an even more interoperable solution, and write down those interoperable guidelines in the spec.

@kojiishi
Copy link
Contributor Author

So, @FremyCompany, do you want to start writing down the guidelines? I think we've got enough experiments to review them and see what's possible and what's not for us, and hopefully adopt our new impls to it as much as possible.

@fantasai
Copy link
Collaborator

I think inside list markers should behave as regular inline boxes would, and outside list markers should not affect the line height, as it throws off the rhythm. However, they could force the creation of a strut in certain cases. In CSS2.1 these two things would be equivalent, but with ::marker styling they are not.

@FremyCompany
Copy link
Contributor

@kojiishi I can start building a bunch of test cases, and see what we can agree on

@kojiishi
Copy link
Contributor Author

@FremyCompany sounds great. I believe wpt has some methods to add "tentative" tests, that tests proposed behavior but not really agreed yet. Probably this one:
https://github.com/w3c/web-platform-tests/blob/master/docs/_writing-tests/file-names.md
Maybe we can use this feature?

@fantasai I'm not sure about the line height yet. Not affecting is probably easier to implement, but I personally prefer "avoid overlap by default" and rhythm being opt-in. As far as I understand, that's the policy CSS2 has today since its beginning, and while I'm a big fan of rhythm, I'm even bigger fan of "avoid overlap by default." Rhythm is a professional feature where authors can opt-in, while avoiding overlap is a basic feature for non-professionals. From that perspective, it should affect the line height.

aarongable pushed a commit to chromium/chromium that referenced this issue Apr 11, 2018
@kojiishi
[LayoutNG] Fix list markers not to overlap with previous blocks
b713616 
This patch fixes when list marker is taller than the content of the
list item, list markers not to intrude previous blocks.

This behavior is not defined, but Blink, WebKit, and Edge do this
by including list marker height into the first line box. This turned
out to require a bit complex propagation. Instead, this patch
includes list marker height into the list item.

This is being tracked in
w3c/csswg-drafts#2418
but I think this patch has close enough behavior and turns following
tests good enough to rebaseline.

  fast/css/first-child-pseudo-class.html
  fast/css/first-of-type-pseudo-class.html
  fast/css/last-child-pseudo-class.html
  fast/css/last-of-type-pseudo-class.html
  fast/css/only-child-pseudo-class.html
  fast/css/only-of-type-pseudo-class.html

Bug: 725277
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_layout_ng
Change-Id: Ic911a96f7804a91d6dccf2bf64bc6fd49169e8cb
Reviewed-on: https://chromium-review.googlesource.com/994915
Reviewed-by: cathie chen <cathiechen@tencent.com>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Commit-Queue: Koji Ishii <kojii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549843}
@frivoal frivoal mentioned this issue Mar 20, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
css-lists-3 Current Work css-lists-4
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@FremyCompany @fantasai @kojiishi @cathiechen