Last Comment Bug 676538 - After landing Bug 403524, CSS { text-decoration:underline; vertical-align:top; } drawn overline
: After landing Bug 403524, CSS { text-decoration:underline; vertical-align:top...
Status: VERIFIED FIXED
[qa!]
: regression
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla8
Assigned To: Vitor Menezes
:
: Jet Villegas (:jet)
Mentors:
Depends on: 750917
Blocks: 403524
  Show dependency treegraph
 
Reported: 2011-08-04 07:48 PDT by Alice0775 White
Modified: 2012-05-01 16:37 PDT (History)
14 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
sample htlm (192 bytes, text/html)
2011-08-04 07:48 PDT, Alice0775 White
no flags Details
Proposed patch (1.57 KB, patch)
2011-08-05 18:49 PDT, Vitor Menezes
dbaron: review-
Details | Diff | Splinter Review
additional testcases (689 bytes, text/html)
2011-08-05 19:51 PDT, David Baron :dbaron: ⌚️UTC-10
no flags Details
PRoposed patch rev 1 (5.04 KB, patch)
2011-08-08 19:25 PDT, Vitor Menezes
dbaron: review-
Details | Diff | Splinter Review
Proposed patch rev 2 (5.32 KB, patch)
2011-08-10 12:07 PDT, Vitor Menezes
dbaron: review+
Details | Diff | Splinter Review
Screenshot of test case nr. 2 layout on ubuntu (27.44 KB, image/png)
2011-10-05 05:24 PDT, Virgil Dicu [:virgil] [QA]
no flags Details
Screenshot of test case nr. 2 layout on Mac OS (33.51 KB, image/png)
2011-10-05 05:25 PDT, Virgil Dicu [:virgil] [QA]
no flags Details

Description Alice0775 White 2011-08-04 07:48:36 PDT
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
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2011-08-04 07:58:01 PDT
Requesting tracking for the regression.
Comment 2 David Baron :dbaron: ⌚️UTC-10 2011-08-04 11:46:20 PDT
Yeah, probably *psd->mBaseline needs to be adjusted somewhere in PlaceTopBottomFrames.
Comment 3 Vitor Menezes 2011-08-05 18:49:58 PDT
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.
Comment 4 David Baron :dbaron: ⌚️UTC-10 2011-08-05 19:41:59 PDT
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-10 2011-08-05 19:51:20 PDT
Created attachment 551225 [details]
additional testcases
Comment 6 David Baron :dbaron: ⌚️UTC-10 2011-08-05 19:51:54 PDT
You're right about the location of the bug, though -- comment 2 was nonsense.
Comment 7 David Baron :dbaron: ⌚️UTC-10 2011-08-05 19:53:02 PDT
(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 2011-08-08 04:28:11 PDT
Is it possible to temporarily work around this problem with Stylish scripts?
Comment 9 Vitor Menezes 2011-08-08 19:25:43 PDT
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.
Comment 10 David Baron :dbaron: ⌚️UTC-10 2011-08-09 11:37:10 PDT
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 2011-08-10 12:07:57 PDT
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.
Comment 12 David Baron :dbaron: ⌚️UTC-10 2011-08-10 12:28:34 PDT
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-10 2011-08-10 12:46:49 PDT
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 15 Virgil Dicu [:virgil] [QA] 2011-10-05 05:22:47 PDT
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] 2011-10-05 05:24:07 PDT
Created attachment 564802 [details]
Screenshot of test case nr. 2 layout on ubuntu
Comment 17 Virgil Dicu [:virgil] [QA] 2011-10-05 05:25:09 PDT
Created attachment 564803 [details]
Screenshot of test case nr. 2 layout on Mac OS
Comment 18 David Baron :dbaron: ⌚️UTC-10 2011-10-05 07:41:39 PDT
(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] 2011-10-06 06:29:57 PDT
(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-10 2011-10-06 09:08:43 PDT
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] 2011-10-07 07:58:16 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.