After landing Bug 403524, CSS { text-decoration:underline; vertical-align:top; } drawn overline

VERIFIED FIXED in Firefox 8

Status

()

Core
Layout
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: Alice0775 White, Assigned: Vitor Menezes)

Tracking

(Depends on: 1 bug, {regression})

Trunk
mozilla8
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox8+ fixed)

Details

(Whiteboard: [qa!])

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 550691 [details]
sample htlm

Build Identifier:
http://hg.mozilla.org/integration/mozilla-inbound/rev/cfb447e2f21f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110803 Firefox/8.0a1 ID:20110803114009
and
Recent m-c hourly build.
http://hg.mozilla.org/mozilla-central/rev/5d742d2e4304
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110803 Firefox/8.0a1 ID:20110804022245

After landing Bug 403524,
CSS { text-decoration:underline; vertical-align:top; }  drawn overline.

This does not happen on Google Chrome13.0.782.107, Opera11.50 and IE9.

Reproducible: Always

Steps to Reproduce:

1. Start browser
2. Open Sample html
3.

Actual Results: 
  drawn overline

Expected Results: 
  Should be underline
Requesting tracking for the regression.
tracking-firefox8: --- → ?
Keywords: regression
Yeah, probably *psd->mBaseline needs to be adjusted somewhere in PlaceTopBottomFrames.
(Assignee)

Updated

6 years ago
Assignee: nobody → vmenezes
(Assignee)

Comment 3

6 years ago
Created attachment 551214 [details] [diff] [review]
Proposed patch

We were retrieving a FrameProperty in different conditions from which it was set, causing it to 0. This should rectify that.
Attachment #551214 - Flags: review?(dbaron)
So, reading the code, I actually think it's off a bit more than this.

The current code sets |firstBlock| when we're about to handle the frame that is the *child* of the first block.  Then it sets baselineOffset at that point, and never updates it again.

I think, really, we want |baselineOffset| to be different when handling the child of the block (where it should be set by the normal !nearestBlockFound codepath than when handling the first block or any of its ancestors (when it should be set based on the calculation you have in the firstBlock ... case).

So I think the loop, instead of tracking f and fParent, should track f and fChild (where fChild was the previous f), set firstBlock when *f* is the first block, and then things should work.

I think other bugs should be relatively easy to demonstrate with tests.
Created attachment 551225 [details]
additional testcases
You're right about the location of the bug, though -- comment 2 was nonsense.
(In reply to David Baron [:dbaron] from comment #5)
> Created attachment 551225 [details]
> additional testcases

To be clear:  these cases should all show three distinct underlines, just like they did before your patches landed.
Attachment #551214 - Flags: review?(dbaron) → review-

Comment 8

6 years ago
Is it possible to temporarily work around this problem with Stylish scripts?
(Assignee)

Comment 9

6 years ago
Created attachment 551660 [details] [diff] [review]
PRoposed patch rev 1

This actually makes more sense if we track f and fChild. I did have to change the what we store, though, for the sake of simplifying extraction; the alternative is some awkward "undoing" of the accumulation of the offset.
Attachment #551214 - Attachment is obsolete: true
Attachment #551660 - Flags: review?(dbaron)
Comment on attachment 551660 [details] [diff] [review]
PRoposed patch rev 1

>We also instead store "offset from the block's top" rather than "offset from the
>frame's top" for each frame, to simplify the retrieval.

This is incorrect.

Storing an offset from the block's top is incorrect since it's possible to move a line within a block without reflowing it again (and thus without executing this code again and fixing the value).

Likewise, storing an offset from the top of the line would be useless since the whole point of storing this information is that we don't know where the line is; we only know where frames are.

So I think the change to what is being stored is incorrect -- you should get the loop working with the old piece of information that we were storing.

>diff --git a/layout/generic/nsLineLayout.cpp b/layout/generic/nsLineLayout.cpp
>--- a/layout/generic/nsLineLayout.cpp
>+++ b/layout/generic/nsLineLayout.cpp
>+    const nscoord offset = baselineY - rootPFD.mBounds.y;

I'm also not sure if this does what you think it does -- I don't see where we set the mBounds of the root PFD -- so it looks (although I could be wrong) like rootPFD.mBounds.y is always 0.
Attachment #551660 - Flags: review?(dbaron) → review-
(Assignee)

Comment 11

6 years ago
Created attachment 552164 [details] [diff] [review]
Proposed patch rev 2

Updated to undo the frame offset accumulated in the special vertical-align case. Passes reftests and the proposed test case.
Attachment #551660 - Attachment is obsolete: true
Attachment #552164 - Flags: review?(dbaron)
Attachment #552164 - Flags: review?(dbaron) → review+
But I'm going to adjust the first line of the commit message to:

Bug 676538: Fix regression that caused text-decorations on inline *child* of block to draw at the offset for the block rather than the inline.  r=dbaron

and wrap the rest more evenly.
http://hg.mozilla.org/integration/mozilla-inbound/rev/d7960f9e273c
and I also landed the first chunk of attachment 551225 [details] as a reftest:
http://hg.mozilla.org/integration/mozilla-inbound/rev/23a7c8800e3f
OS: Windows XP → All
Hardware: x86 → All
Target Milestone: --- → mozilla8
Merged:
http://hg.mozilla.org/mozilla-central/rev/d7960f9e273c
http://hg.mozilla.org/mozilla-central/rev/23a7c8800e3f
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED

Updated

6 years ago
status-firefox8: --- → fixed
tracking-firefox8: ? → +
Checked on Ubuntu 11.04, Mac OS 10.6, Windows XP, Windows 7

The characters are no longer drawn overline. However, shouldn't the underlines match the character width (as Google Chrome behavior, for example)? Currently, the last character underline matches the width sum of all the characters in the set.

Also, in the third character set in the test case, the middle character is overlaped by the first character's underline. This occurs on Ubuntu 11.04, Windows Xp and Windows 7. On Mac OS, no overlaps occur. Is this expected?
Created attachment 564802 [details]
Screenshot of test case nr. 2 layout on ubuntu
Created attachment 564803 [details]
Screenshot of test case nr. 2 layout on Mac OS
(In reply to Virgil Dicu [QA] from comment #15)
> Checked on Ubuntu 11.04, Mac OS 10.6, Windows XP, Windows 7
> 
> The characters are no longer drawn overline. However, shouldn't the
> underlines match the character width (as Google Chrome behavior, for
> example)? Currently, the last character underline matches the width sum of
> all the characters in the set.

No; there are supposed to be multiple underlines.  That's the point of the test.  Chrome's behavior isn't remotely close to the spec.
(In reply to David Baron [:dbaron] from comment #18)
> (In reply to Virgil Dicu [QA] from comment #15)
> > Checked on Ubuntu 11.04, Mac OS 10.6, Windows XP, Windows 7
> > 
> > The characters are no longer drawn overline. However, shouldn't the
> > underlines match the character width (as Google Chrome behavior, for
> > example)? Currently, the last character underline matches the width sum of
> > all the characters in the set.
> 
> No; there are supposed to be multiple underlines.  That's the point of the
> test.  Chrome's behavior isn't remotely close to the spec.

Thanks for clarifying that. 

The second question in comment 15 still remains however, regarding the overlap in the third set of characters on Ubuntu and Windows, issue that does not occur on Mac (see attachments for comparison between Mac OS and Ubuntu). Is this problem related to this fix? Is it known and expected somehow?
I didn't do the vertical positioning with much care, and it obviously depends on the font metrics used.  It's not a problem.
Mozilla/5.0 (X11; Linux x86_64; rv:8.0) Gecko/20100101 Firefox/8.0
Mozilla/5.0 (X11; Linux x86_64; rv:9.0a2) Gecko/20111007 Firefox/9.0a2
Mozilla/5.0 (X11; Linux x86_64; rv:10.0a1) Gecko/20111007 Firefox/10.0a1

Verified in Firefox 8, 9 and 10 on Ubuntu 11.04, Windows XP, Windows 7 and Mac OS 10.6 using the 2 test cases from the attachments. The characters are no longer drawn overline.
Status: RESOLVED → VERIFIED
Whiteboard: [qa!]
(Reporter)

Updated

5 years ago
Depends on: 750917
You need to log in before you can comment on or make changes to this bug.