942017 - Height of bullet is overestimated

Status: RESOLVED FIXED

(Core :: Layout, defect, P4)

Description

[Xidorn Quan [:xidorn] UTC+11]

Attachment: testcase

As mentioned in bug 934072 comment 79, there is an odd behavior that height of content box of LI element may be enlarged by its bullet.

This appears when the max height of the font used in bullet is larger than the line-height of the corresponding LI element.

I cannot confirm whether it is a bug or an intended behavior.

Comment 1

[Mats Palmgren (inactive)]

Attachment: fix

FYI, PFD_ISBULLET is only set for outer bullets.  I'll file a bug separately
about caching GetType() on the pfd struct to avoid doing a virtual call here
(and elsewhere).

Comment 2

[Mats Palmgren (inactive)]

Attachment: reftest

https://tbpl.mozilla.org/?tree=Try&rev=86c70f6b92c8
https://tbpl.mozilla.org/?tree=Try&rev=3123e4e11b58

Comment 3

[Jonathan Kew [:jfkthame]]

Comment on attachment 8341128 [details] [diff] [review]
fix

Review of attachment 8341128 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine to me, as far as the line-height issue is concerned.

However, the reftest appears to fail for me locally on OS X. Vertically, it's fine, but there's a discrepancy in the horizontal width of the boxes. I've pushed a tryserver job to check whether it's just something I've broken locally in my tree or profile... https://tbpl.mozilla.org/?tree=Try&rev=004d05e9110a.

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

The reftest failed for me locally as well. I guess that this patch depends on his patches for bug 737785 and bug 943918 which can be seen in the tbpl link he posted.

Comment 5

[Mats Palmgren (inactive)]

The reason the test now fails is that bug 934072 apparently changed the layout for
<li> with list-style-type:decimal too.  Was that really intentional?  That bug's
title seems to indicate it should only affect "East Asian counter styles".

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Mats Palmgren (:mats) from comment #5)
> The reason the test now fails is that bug 934072 apparently changed the layout for <li> with list-style-type:decimal too.  Was that really intentional?  That bug's title seems to indicate it should only affect "East Asian counter styles".

The only impact to decimal style is that, my patch changed the style of ::-moz-list-number from "-moz-margin-end: 8px;" to "-moz-padding-end: 0.5em;" which can be found here: https://bugzilla.mozilla.org/attachment.cgi?id=8340758&action=diff#a/layout/style/ua.css_sec2 . This change is intentional, and you may alter your reftest to fit this change.

Comment 7

[Mats Palmgren (inactive)]

OK, I'll update my test and make sure it pass on Try before landing.

I think it would have been better if the ua.css and other non-CJK changes had been done
as a separate bug, with an appropriate title.

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

Well, that was a preparation for that bug, not a bug itself... I don't know what is the proper flow for such case.

Comment 9

[Jonathan Kew [:jfkthame]]

(In reply to Mats Palmgren (:mats) from comment #7)
> I think it would have been better if the ua.css and other non-CJK changes
> had been done
> as a separate bug, with an appropriate title.

Yes, perhaps I should have suggested that. We were aware that the CSS change would have a (beneficial!) side-effect on the precise rendering of existing styles, but I felt the change was sufficiently minor and well-understood that it wasn't necessary to split off into a separate bug. That may have been the wrong call - sorry.

Comment 10

[Mats Palmgren (inactive)]

Don't get me wrong, I think it's a good change, but it shouldn't
have slipped in as a minor "preparatory patch" in a bug with a
different subject matter.

<li> is widely used on the web, so changing the default horizontal
distance between the list marker and its block *universally*, i.e.
for bullets, decimal and everything else, is a pretty big deal.
Such changes should always be done in separate bugs, so that they
get the attention they deserve (review/test/QA/dev-doc/tracking/etc)
and to minimize risk.

Don't worry about it though; I just wanted to explain my earlier
remark.

Comment 11

[Mats Palmgren (inactive)]

Attachment: reftest, v2

Updated reftest is green on Try:
https://tbpl.mozilla.org/?tree=Try&rev=af0bfffd5841
https://tbpl.mozilla.org/?tree=Try&rev=cad54542db04

Comment 12

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe119a83b1f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c63decb4e7e

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fe119a83b1f2
https://hg.mozilla.org/mozilla-central/rev/3c63decb4e7e

Comment 14

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

I think this was intended behavior, and the patch should be backed out.  I don't see why we wouldn't want the bullet to contribute to the line's height just like anything else in the line -- doing so prevents overlap by default, and the line-height property allows the effect to be adjusted.

I'd also note that making line-height:normal behave differently from other line-heights is even more broken.

See also bug 986098.

Comment 15

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

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #14)
> I'd also note that making line-height:normal behave differently from other
> line-heights is even more broken.

I'd further note here that what we do for text is justifiable in that it's a workaround for 'normal' line-height being derived only from the first available font, and not all fonts used (as it probably should be).  That justification does not apply to bullets.

Comment 16

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

Bug 986098 turned into a mess, so I separated the line-height regression into bug 989130.

Comment 17

[Xidorn Quan [:xidorn] UTC+11]

I believe the true problem uncovered by this bug is the overestimate of bullet height, for example "1. 123" is taller than just "123". The reason is that we estimate height of text using the actual height, however the height of bullet is the max possible height in the given font.

Comment 18

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

Reopening due to backout in bug 989130 (although it hasn't hit mozilla-central yet).

Comment 19

[Mats Palmgren (inactive)]

This was fixed by removing our legacy code path for ::markers (nsBulletFrame) in bug 1542807.

Taking to add a reftest for this...

Comment 20

[Mats Palmgren (inactive)]

Attachment: Bug 942017 - Add a reftest to check the line height of 'decimal' ::markers. r=#layout-reviewers

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a5b9a53c428c
Add a reftest to check the line height of 'decimal' ::markers.  r=dholbert

Comment 22

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

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

Comment 23

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/a5b9a53c428c

Comment 24

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

Upstream PR merged by moz-wptsync-bot