436356 - Bullet lists does not display correctly, works in FF 2

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Marcia Knous [:marcia]]

Reported from hendrix. Seen using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9) Gecko/2008052903 Firefox/3.0.

STR:
1. Visit site. Observe bullets are misplaced.

Bullets are shown properly using Safari as well as Firefox 2.0.0.14

Comment 1

[Steven Michaud [:smichaud] (Retired)]

Bizarre.  The bullets appear in the center of each line, instead of at
the left (as they should).

This also happens with Firefox 3 RC 1 on both Windows and Linux.

Comment 2

[Brian Polidoro]

Attachment: Minimized testcase

Comment 3

[Brian Polidoro]

This only occurs in quirks mode.

Comment 4

[Brian Polidoro]

Regression range:
20080329 ok
20080330 broken

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-03-29+04%3A00%3A00&maxdate=2008-03-30+04%3A00%3A00&cvsroot=%2Fcvsroot

Comment 5

[Atsushi Sakai]

Refined regression range:
2008-03-29 14:25:00 ok
2008-03-29 14:26:00 broken

http://bonsai.mozilla.org/cvsquery.cgi?module=all&date=explicit&mindate=2008-03-29+14%3A25%3A00&maxdate=2008-03-29+14%3A26%3A00&cvsroot=%2Fcvsroot

Comment 6

[Atsushi Sakai]

Attachment: testcase

If the first child of <li> is a block element, the bullet appears at the left, which is same as Gecko 1.8.
But if the first child of <li> is a space text node, the bullet appear at the center, which is not same as Gecko 1.8.

Comment 7

[Atsushi Sakai]

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsBlockFrame.cpp&rev=3.954&mark=1081#1067

When this problem occurs, |0 == mLines.front()->mBounds.height| is false.
But I think it should be true for such a case, and it was true before Bug 421353 landed.

Comment 8

[Atsushi Sakai]

Attachment: Patch v1.0

This patch should restore old behavior.

Nakano-san, how do you think about this?

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Should we check the values of textMetrics.mAscent and textMetrics.mDescent instead of the content length?

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You probably should check transformedCharsFit.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: testcase with zwnj

I worry about that the third case is fixed by the patch.

Comment 12

[Atsushi Sakai]

(In reply to comment #11)
> I worry about that the third case is fixed by the patch.

The layout of zwnj case will not be changed by this approach even if
textMetrics.{mAscent,mDescent} is checked
because textMetrics.{mAscent,mDescent} have value like normal character.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #12)
> (In reply to comment #11)
> > I worry about that the third case is fixed by the patch.
> 
> The layout of zwnj case will not be changed by this approach even if
> textMetrics.{mAscent,mDescent} is checked
> because textMetrics.{mAscent,mDescent} have value like normal character.

ah, ok. we should ignore the Zero-width issue here. please check the comment 10.

Comment 14

[Steven Michaud [:smichaud] (Retired)]

Two possibly related bugs?  Bug 439738 and bug 439957.

Comment 15

[Brian Polidoro]

I downloaded the pages from those bugs and gave them Doctypes to put them in standards mode and those bug are still there.  This bug is quirks mode only.  But that could be irrelevant.  

Comment 16

[Atsushi Sakai]

Attachment: Patch v1.1

Addressed comment #10

This patch seems to work fine,
though I don't understand what transformedCharsFit is.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 325730 [details] [diff] [review]
Patch v1.1

This is good.

transformedCharsFit means the number of characters in this text frame, *after* applying whitespace trimming (at the start or end of the line) and whitespace collapsing. I.e. the number of characters that will actually be displayed.

One more thing though; it would be great if you could make a reftest for this.

Comment 18

[Atsushi Sakai]

Attachment: Patch for adding reftests

(In reply to comment #17)
> One more thing though; it would be great if you could make a reftest for this.

Adding 2 tests, where 436356-1 is in quirks mode, and 436356-2 is in standards mode.

I've confirmed 436356-1 fails without this bug fixed, and passes with this bug fixed.
436356-2 passes regardless of this bug fixed; just make sure it.

Is this good?

> transformedCharsFit means the number of characters in this text frame, *after*

Thanks for explanation.
I understood that that's exactly what I was seeking.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 326066 [details] [diff] [review]
Patch for adding reftests

Fantastic!

Comment 20

[Atsushi Sakai]

Attachment: Patch for adding reftests

Updated to latest trunk.
Carrying over review.

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

please nominate these for approval1.9.0.1 if they're ready to go on trunk; have they landed/baked on mozilla-central yet?

Comment 22

[Atsushi Sakai]

(In reply to comment #21)
> please nominate these for approval1.9.0.1 if they're ready to go on trunk;

They're already ready to go, but approval1.9.0.1 is grayed out and can't be changed...

> have
> they landed/baked on mozilla-central yet? 

Not yet.
I don't have write permission to the repository.
I'd appreciate it if someone could check in.

Comment 23

[Atsushi Sakai]

Thanks for requesting approval.

Please note that "Patch for adding reftests" may not be applied to 1.9.0 branch cleanly, though it's easy to merge.
Should I create a patch for branch?

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Wouldn't hurt. Also keep in mind that these patches won't be allowed to land on the 1.9.0 branch until after they've landed successfully in mozilla-central.

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

sorry for the delay, checked-in to trunk.

Thank you, Sakai-san, for your work!

Comment 29

[Samuel Sidler (old account; do not CC)]

Comment on attachment 325730 [details] [diff] [review]
Patch v1.1

Approved for 1.9.0.2. Please land in CVS. a=ss

Comment 30

[Samuel Sidler (old account; do not CC)]

Comment on attachment 326488 [details] [diff] [review]
Patch for adding reftests

Approved for 1.9.0.2. Please land these tests in CVS when the patch lands. a=ss

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

landed to 1.9.0 branch.

Comment 32

[Marcia Knous [:marcia]]

verified fixed on the 1.9.0 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.2pre) Gecko/2008072204 GranParadiso/3.0.2pre. The site in the URL now displays correctly.

verified fixed on the trunk using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1a1pre) Gecko/2008072202 Minefield/3.1a1pre.