512988 - Text-shadow blur and underline messes up with padding-left

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect, P3)

Description

[Joseph Cresencia]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2

Having text-shadow with a blur and text-decoration underline messes up the shadow of the underline if there is a padding-left. This issue will affect like CSS horizontal menus.

I will provide an HTML attachment with some tests.

Reproducible: Always

Steps to Reproduce:
1. See following attachment
Actual Results:  
Underline is missing shadow depending on the amount of padding-left

Expected Results:  
Underline should be shadowed as well

Comment 1

[Joseph Cresencia]

Attachment: Test cases of the issue

Comment 2

[Mardeg]

Confirmed on trunk

Comment 3

[Michael Ventnor]

Attachment: Patch

We were unnecessarily trimming the underline box.

I can't really test this, it's impossible to do a reftest with a dynamic blur :(

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

There's no dynamic blur here, right? Seems to me a reftest could easily be done.

In fact I think the bug is that shadowRect needs to be offset by the border+padding top and left, then this would work. But this patch is fine too, since shrinking the blur box to account for padding and borders is an optimization that we don't really need, and removing it is simpler.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 397246 [details] [diff] [review]
Patch

Doesn't need sr. Does need a reftest.

Comment 6

[Michael Ventnor]

Actually, we should make the shadow rect as small as possible so the blurring is as fast as possible.
In fact, we can do this more easily if I can correct our misuse of gfxRect that I noticed once I got more acquainted with the code.

Comment 7

[Michael Ventnor]

Attachment: Patch 2

The problem is we were using gfxRect to represent app-units, which caused a lot of messy hacks outside of nsTextFrameThebes (where this problem is rife). If we fix this, it will remove quite a bit of code and also makes fixing this bug easier without needing to remove that optimization.

This has a reftest.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

   nsMargin bp = f->GetUsedBorderAndPadding();
   nscoord innerWidthInAppUnits = (mFrame->GetSize().width - bp.LeftRight());
+  nsPoint pt = aBuilder->ToReferenceFrame(mFrame) +
+               nsPoint(mOffset.x, mOffset.y);
 
-  gfxRect shadowRect = gfxRect(pt.x, pt.y, innerWidthInAppUnits, mFrame->GetSize().height);
+  // The rect must be offset by the padding, otherwise we won't paint all
+  // of the text decoration if the frame has padding applied to it (bug 512988)
+  nsRect shadowRect(pt.x + bp.left, pt.y + bp.top,
+                    innerWidthInAppUnits, mFrame->GetSize().height);

You should subtract the border+padding TopBottom from the frame height here. In fact, all we're trying to do here is get the frame's content rect. So you can just use mFrame->GetContentRect() - mFrame->GetPosition() + aBuilder->ToReferenceFrame(mFrame) + mOffset

You should change mOffset from a gfxPoint to an nsPoint.

Comment 9

[Michael Ventnor]

Attachment: Patch 3

Thanks. This removes even more code.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually, I think this patch is incorrect. It doesn't account for text decorations that overflow the content-rect.

Masayuki-san, have you got examples that show standards-mode text decorations overflowing the content-rect of the frame?

Comment 11

[Dão Gottwald [:dao]]

(In reply to comment #10)
> Actually, I think this patch is incorrect.

Next time, please remove checkin-needed if you think it shouldn't land...
This landed now. Please back it out if it's incorrect enough to not stay on trunk.

http://hg.mozilla.org/mozilla-central/rev/1e6d52401dd2

Comment 12

[Dão Gottwald [:dao]]

backed out

REFTEST TEST-UNEXPECTED-FAIL | file:///builds/moz2_slave/mozilla-central-macosx-unittest-everythingelse/build/reftest/tests/layout/reftests/box-shadow/boxshadow-rounding.html

Comment 13

[Markus Stange [:mstange]]

Just FTR, this had also regressed Txul on Vista.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #11)
> Next time, please remove checkin-needed if you think it shouldn't land...

Yes, sorry, my bad.

(In reply to comment #13)
> Just FTR, this had also regressed Txul on Vista.

That makes no sense at all. Do we use text-shadow anywhere in the UI in Vista?

Comment 15

[Markus Stange [:mstange]]

(In reply to comment #14)
> That makes no sense at all.

Er, right... I misread Beltzner's explanation in m.d.tree-management; he was blaming the backout, which makes even less sense. Sorry.

Comment 16

[Nochum Sossonko [:Natch]]

(In reply to comment #14)
> That makes no sense at all. Do we use text-shadow anywhere in the UI in Vista?

http://mxr.mozilla.org/mozilla-central/search?string=text-shadow&find=\.css&findi=\.css&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

"Yes".

Comment 17

[Markus Stange [:mstange]]

(In reply to comment #16)
> "Yes".

Rather, "only for glass panels, with lightweight themes or for disabled text in Classic", none of which are present in Vista Txul, AFAIK.

Comment 18

[Michael Ventnor]

Attachment: Patch 4

This fixes the test failure, and also uses the padding rect rather than the content rect. I also updated the ascent code to match whats in the text decorations paint function.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: testcase

Changing from content-rect to padding-rect doesn't fix the problem I mentioned in comment #10. It's quite possible for the text decorations to overflow arbitrarily far, as seen in this testcase. This is actually an existing bug on trunk but you may as well fix it here while you change this code.

And we'll need tests for this problem, and the first-letter thing.

Comment 20

[Michael Ventnor]

I can't reftest the first-letter problem, for it to show up you need to set float:left and some padding-top on :first-letter. Unless you can think of a way, I can't find a way to equivalently position the text in the reference file, and have it work reliably on all platforms' fonts.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

ok

Comment 22

[Michael Ventnor]

Attachment: Patch 5

Phew. This does it; it has more code but it should be more future-proof. We base the shadow surface size on the text decoration rect, so the surface is now as small as it can possibly get. We now actually use the given nsLineBox, and we just work on a lot more cases.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    NS_FOR_CSS_SIDES(side) {
+      if (skip & (1 << side)) {
+        bp.side(side) = 0;
+      }
+    }

Do we really need to do this? Can't you use GetContentRect for inline frames?

But I wonder if this complexity is really worth it. Why don't we just use the frame's overflow area, since that must contain any overflowing decorations?

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

More precisely, what if we used the line combined area (i.e. line overflow area) for lines and the frame overflow area for inlines?

Comment 25

[Michael Ventnor]

Overflow rects can get insanely large, complexity for the sake of performance is not necessarily a bad thing if the complexity is not out of control.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Do you have a realistic example where the overflow rect would be much larger than necessary?

Comment 27

[Michael Ventnor]

The shadow is offset by a large amount.

I can use the content rect for inline frames, but I'd really rather not get rid of these optimizations.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, that makes sense. But I think you can use the content rect for non-line frames, it should be equivalent to what you already have.

Comment 29

[Michael Ventnor]

Attachment: Patch 6

Comment 30

[Dão Gottwald [:dao]]

this doesn't apply on trunk

Comment 31

[Michael Ventnor]

Attachment: Patch 6 rebased

Comment 32

[Michael Ventnor]

Dao, I rebased the patch. Could you please try again? Thanks in advance.

Comment 33

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/03707acbc593

Comment 34

[Michael Ventnor]

Comment on attachment 401441 [details] [diff] [review]
Patch 6 rebased

This fixes layout bugs so it really should go on 1.9.2 (although you might need to use Patch 6 rather than the rebased one)

Comment 35

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

Comment on attachment 401441 [details] [diff] [review]
Patch 6 rebased

a1.9.2=dbaron

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/efbced394ad5