Closed Bug 436356 Opened 12 years ago Closed 12 years ago

Bullet lists does not display correctly, works in FF 2

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

VERIFIED FIXED

People

(Reporter: marcia, Assigned: dev-null)

References

()

Details

(Keywords: regression, testcase, verified1.9.0.2)

Attachments

(3 files, 4 obsolete files)

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
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
Attached file Minimized testcase (obsolete) —
This only occurs in quirks mode.
Attached file testcase (obsolete) —
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.
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?
Attached patch Patch v1.0 (obsolete) — Splinter Review
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.
Attached file 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
(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.
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.  
Attached patch Patch v1.1Splinter Review
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+
Attached patch Patch for adding reftests (obsolete) — Splinter Review
(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+
Updated to latest trunk.
Carrying over review.
Attachment #326066 - Attachment is obsolete: true
Attachment #326488 - Flags: review+
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-
(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?
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
Closed: 12 years ago
Resolution: --- → FIXED
Duplicate of this bug: 444720
Duplicate of this bug: 445787
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+
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
You need to log in before you can comment on or make changes to this bug.