Height of bullet is overestimated

REOPENED
Unassigned

Status

()

Core
Layout
P4
normal
REOPENED
4 years ago
9 months ago

People

(Reporter: xidorn, Unassigned)

Tracking

(Depends on: 1 bug, {testcase})

Trunk
testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Created attachment 8336542 [details]
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.

Updated

4 years ago
Assignee: nobody → matspal
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: testcase
Priority: -- → P4
Created attachment 8341128 [details] [diff] [review]
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).
Attachment #8341128 - Flags: review?(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.
Attachment #8341128 - Flags: review?(jfkthame) → review+
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.
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".
(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.
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.
Well, that was a preparation for that bug, not a bug itself... I don't know what is the proper flow for such case.
(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.
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.
https://hg.mozilla.org/mozilla-central/rev/fe119a83b1f2
https://hg.mozilla.org/mozilla-central/rev/3c63decb4e7e
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Status: RESOLVED → VERIFIED
Depends on: 986098
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.
(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.
Bug 986098 turned into a mess, so I separated the line-height regression into bug 989130.
Depends on: 989130
No longer depends on: 986098
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.
Reopening due to backout in bug 989130 (although it hasn't hit mozilla-central yet).
Status: VERIFIED → REOPENED
Resolution: FIXED → ---

Updated

4 years ago
Assignee: matspal → nobody

Updated

4 years ago
Attachment #8341128 - Attachment is obsolete: true
Flags: in-testsuite+
Summary: Height of LI may be affected by bullet → Height of bullet is overestimated
Target Milestone: mozilla28 → ---
Depends on: 1358377
You need to log in before you can comment on or make changes to this bug.