Explicit line-height on <li> makes the list-style-image not affect the spacing between list items

VERIFIED FIXED in Firefox 29

Status

()

defect
P2
normal
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

(Blocks 1 bug, {regression})

Trunk
mozilla31
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox28 wontfix, firefox29+ verified, firefox30+ verified, firefox31+ verified, b2g-v1.3 ?, b2g-v1.4 fixed, b2g-v2.0 fixed)

Details

Attachments

(1 attachment)

This is separated out from bug 986098, which is a mess because it ended up being a discussion of two bugs.

This covers bug 986098 comment 1, bug 986098 comment 2, bug 986098 comment 4, bug 986098 comment 8, bug 986098 comment 12, bug 986098 comment 16, and bug 986098 comment 22.

Testcase is attachment 8397868 [details].

(In reply to Mats Palmgren (:mats) from bug 986098 comment #22)
> (In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from bug 986098 comment #17)
> > I think we should just back out bug 942017.
> 
> That would re-introduce the parity with IE and Chrome for the test in that
> bug.

"parity with IE and Chrome"?  Could you explain?  Do you mean lack of parity?

I still think the fix is absolutely the wrong change to make.
Flags: needinfo?(matspal)
This is a regression that causes overlapping lines, which are pretty bad; we should track.
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #0)
> "parity with IE and Chrome"?  Could you explain?  Do you mean lack of parity?

By "parity" I meant "differently than other UAs".  Backing out the patch in bug 942017
will make us display the test in that bug differently than IE and Chrome.
Currently, all three UAs are compatible for that test.

> I still think the fix is absolutely the wrong change to make.

I think we should display the test in bug 942017 the same as IE and Chrome.
I have no opinion on *how* we do that though.

It seems to me the least risky fix for this bug is to make an exception in that
code for image bullets.
Flags: needinfo?(matspal)

Comment 3

5 years ago
As advised I attempted to use tables for current and older browser support, however the results were the same with unbalanced rows inside of IE.  I also attempted to use absolute positioning with the same unbalanced effect.

I've researched bug 942017 and it seems like <li> and bullets are having numerous issues.  I've never used bugtracker until last week so I'm unsure what it means as to back out on the line height bugs.  Does this mean that this section of code applied to the current update will be reversed back to the previous patch which was unaffected?  If so, is there an estimated time that this would occur possibly?  I've always used FF as a baseline when building and testing new web pages so it has been a complete nightmare the last week not having the same reliable browser to build from.  Most of us know that IE and Chrome have stability and structure issues so I'm just hoping to get a time frame that this will be possibly reversed or fixed.  Again thank you for all of the input and help!!  

Respectfully,
   Eric
Re comment 3:  I'll try to get the change reverted for Firefox 29.


Try run of the backout (needed to check that the test fails across all platforms):
https://tbpl.mozilla.org/?tree=Try&rev=c4e739da6f90
This should be backed out for two reasons:

 (1) It caused a serious regression with bullets from list-style-image,
     which no longer affect the line box height and thus overlap.

 (2) The dependency on 'line-height: normal' doesn't make sense; there's
     no reason for whether line-height is 'normal' or not to affect the
     behavior of bullets.

I don't currently have time to figure out why Gecko behavior differs
from other browsers on bug 942017, but that should be re-analyzed and
the bug fixed in a different way.  At first glance, the code should
already be leading to the expected behavior, since the bullet ought to
have the same metrics and alignment as the block's influence on the line
box's height.  Why this isn't the case should be investigated.
Attachment #8399681 - Flags: review?(jfkthame)
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Comment on attachment 8399681 [details] [diff] [review]
Back out changeset fe119a83b1f2 (bug 942017) while leaving the corresponding test from changeset 3c63decb4e7e

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

OK, you've made a convincing case - out it comes.
Attachment #8399681 - Flags: review?(jfkthame) → review+
https://hg.mozilla.org/mozilla-central/rev/eb9f5016ba72

(I think the use of "back out" in the commit message meant this didn't get marked fixed.)
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Priority: -- → P2
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Comment on attachment 8399681 [details] [diff] [review]
Back out changeset fe119a83b1f2 (bug 942017) while leaving the corresponding test from changeset 3c63decb4e7e

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 942017
User impact if declined: overlapping list marker images
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): low risk; other than the test changes it's a backout of bug 942017
String or IDL/UUID changes made by this patch: none
Attachment #8399681 - Flags: approval-mozilla-beta?
Attachment #8399681 - Flags: approval-mozilla-aurora?
Attachment #8399681 - Flags: approval-mozilla-beta?
Attachment #8399681 - Flags: approval-mozilla-beta+
Attachment #8399681 - Flags: approval-mozilla-aurora?
Attachment #8399681 - Flags: approval-mozilla-aurora+
Is this something we should land on b2g28?
Reproduced the issue on Firefox 28 using the test case https://bugzilla.mozilla.org/attachment.cgi?id=8397868 and the page http://www.plumbtechplumbingandheating.com/index.html.

Verified as fixed on Firefox 29 beta 5 (20140403132807), latest Aurora 30.0a2 (20140404004001) and latest Nigthlty (20140403030201) under Win 7 64-bit, Ubuntu 32-bit an MAC OSX 10.8.5.
Status: RESOLVED → VERIFIED
Keywords: verifyme
QA Contact: petruta.rasa
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #11)
> Is this something we should land on b2g28?

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #12)
> Probably, yes.

Although the approval flag says it's for security issues only.
You need to log in before you can comment on or make changes to this bug.