Bullet lists does not display correctly, works in FF 2

VERIFIED FIXED

Status

()

VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: marcia, Assigned: dev-null)

Tracking

({regression, testcase, verified1.9.0.2})

Trunk
regression, testcase, verified1.9.0.2
Points:
---
Bug Flags:
blocking1.9.0.1 -
wanted1.9.0.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 4 obsolete attachments)

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
(Reporter)

Updated

11 years ago
Keywords: regression
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Mac OS X → All
Hardware: PC → All

Comment 2

11 years ago
Created attachment 322990 [details]
Minimized testcase

Comment 3

11 years ago
This only occurs in quirks mode.
(Assignee)

Comment 6

11 years ago
Created attachment 323060 [details]
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.
(Assignee)

Comment 7

11 years ago
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.
Flags: blocking1.9.0.1?
(Assignee)

Comment 8

11 years ago
Created attachment 323706 [details] [diff] [review]
Patch v1.0

This patch should restore old behavior.

Nakano-san, how do you think about this?
Attachment #323706 - Flags: review?(masayuki)
Should we check the values of textMetrics.mAscent and textMetrics.mDescent instead of the content length?
You probably should check transformedCharsFit.
Created attachment 323819 [details]
testcase with zwnj

I worry about that the third case is fixed by the patch.
Attachment #322990 - Attachment is obsolete: true
Attachment #323060 - Attachment is obsolete: true
(Assignee)

Comment 12

11 years ago
(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.
(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.

Two possibly related bugs?  Bug 439738 and bug 439957.

Comment 15

11 years ago
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.  
(Assignee)

Comment 16

11 years ago
Created attachment 325730 [details] [diff] [review]
Patch v1.1

Addressed comment #10

This patch seems to work fine,
though I don't understand what transformedCharsFit is.
Attachment #323706 - Attachment is obsolete: true
Attachment #325730 - Flags: superreview?(roc)
Attachment #325730 - Flags: review?(roc)
Attachment #323706 - Flags: review?(masayuki)
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.
Attachment #325730 - Flags: superreview?(roc)
Attachment #325730 - Flags: superreview+
Attachment #325730 - Flags: review?(roc)
Attachment #325730 - Flags: review+
(Assignee)

Comment 18

11 years ago
Created attachment 326066 [details] [diff] [review]
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.
Attachment #326066 - Flags: superreview?(roc)
Attachment #326066 - Flags: review?(roc)
Comment on attachment 326066 [details] [diff] [review]
Patch for adding reftests

Fantastic!
Attachment #326066 - Flags: superreview?(roc)
Attachment #326066 - Flags: superreview+
Attachment #326066 - Flags: review?(roc)
Attachment #326066 - Flags: review+
Assignee: nobody → dev-null
(Assignee)

Comment 20

11 years ago
Created attachment 326488 [details] [diff] [review]
Patch for adding reftests

Updated to latest trunk.
Carrying over review.
Attachment #326066 - Attachment is obsolete: true
Attachment #326488 - Flags: review+
(Assignee)

Updated

11 years ago
Blocks: 421353
please nominate these for approval1.9.0.1 if they're ready to go on trunk; have they landed/baked on mozilla-central yet?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.1?
Flags: blocking1.9.0.1-
(Assignee)

Comment 22

11 years ago
(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.
Attachment #326488 - Flags: approval1.9.0.1?
Attachment #325730 - Flags: approval1.9.0.1?
(Assignee)

Comment 23

11 years ago
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?
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.
Attachment #325730 - Flags: approval1.9.0.1? → approval1.9.0.2?
Attachment #326488 - Flags: approval1.9.0.1? → approval1.9.0.2?
sorry for the delay, checked-in to trunk.

Thank you, Sakai-san, for your work!
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Keywords: checkin-needed

Updated

11 years ago
Duplicate of this bug: 444720

Updated

11 years ago
Duplicate of this bug: 445787

Updated

11 years ago
Duplicate of this bug: 445900
Flags: in-testsuite+
Comment on attachment 325730 [details] [diff] [review]
Patch v1.1

Approved for 1.9.0.2. Please land in CVS. a=ss
Attachment #325730 - Flags: approval1.9.0.2? → approval1.9.0.2+
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
Attachment #326488 - Flags: approval1.9.0.2? → approval1.9.0.2+
landed to 1.9.0 branch.
Keywords: fixed1.9.0.2
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.
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.0.2 → verified1.9.0.2
You need to log in before you can comment on or make changes to this bug.