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

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Alice0775 White]

Attachment: 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

Comment 1

[Boris Zbarsky [:bzbarsky]]

Requesting tracking for the regression.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yeah, probably *psd->mBaseline needs to be adjusted somewhere in PlaceTopBottomFrames.

Comment 3

[Vitor Menezes]

Attachment: Proposed patch

We were retrieving a FrameProperty in different conditions from which it was set, causing it to 0. This should rectify that.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: additional testcases

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

You're right about the location of the bug, though -- comment 2 was nonsense.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 8

[solcroft]

Is it possible to temporarily work around this problem with Stylish scripts?

Comment 9

[Vitor Menezes]

Attachment: 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.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 11

[Vitor Menezes]

Attachment: Proposed patch rev 2

Updated to undo the frame offset accumulated in the special vertical-align case. Passes reftests and the proposed test case.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 14

[Mounir Lamouri (:mounir)]

Merged:
http://hg.mozilla.org/mozilla-central/rev/d7960f9e273c
http://hg.mozilla.org/mozilla-central/rev/23a7c8800e3f

Comment 15

[Virgil Dicu [:virgil] [QA]]

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?

Comment 16

[Virgil Dicu [:virgil] [QA]]

Attachment: Screenshot of test case nr. 2 layout on ubuntu

Comment 17

[Virgil Dicu [:virgil] [QA]]

Attachment: Screenshot of test case nr. 2 layout on Mac OS

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 19

[Virgil Dicu [:virgil] [QA]]

(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?

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I didn't do the vertical positioning with much care, and it obviously depends on the font metrics used.  It's not a problem.

Comment 21

[Virgil Dicu [:virgil] [QA]]

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.