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

RESOLVED FIXED in mozilla1.9.3a1

Status

()

Core
Layout: Text
P3
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Joseph Cresencia, Assigned: Michael Ventnor)

Tracking

Trunk
mozilla1.9.3a1
Points:
---

Firefox Tracking Flags

(status1.9.2 beta1-fixed)

Details

Attachments

(3 attachments, 6 obsolete attachments)

(Reporter)

Description

9 years ago
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
(Reporter)

Comment 1

9 years ago
Created attachment 397016 [details]
Test cases of the issue

Updated

9 years ago
Component: General → Layout: Text
Product: Firefox → Core
QA Contact: general → layout.fonts-and-text
Version: unspecified → Trunk

Comment 2

9 years ago
Confirmed on trunk
Blocks: 10713
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
OS: Windows XP → All
Hardware: x86 → All
(Assignee)

Comment 3

9 years ago
Created attachment 397246 [details] [diff] [review]
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 :(
Attachment #397246 - Flags: review?(roc)
(Assignee)

Updated

9 years ago
Attachment #397246 - Flags: superreview?(roc)
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 on attachment 397246 [details] [diff] [review]
Patch

Doesn't need sr. Does need a reftest.
Attachment #397246 - Flags: superreview?(roc)
Attachment #397246 - Flags: review?(roc)
Attachment #397246 - Flags: review+
(Assignee)

Comment 6

9 years ago
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.
(Assignee)

Comment 7

9 years ago
Created attachment 397428 [details] [diff] [review]
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.
Attachment #397428 - Flags: superreview?(roc)
Attachment #397428 - Flags: review?(roc)
   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.
(Assignee)

Comment 9

9 years ago
Created attachment 397574 [details] [diff] [review]
Patch 3

Thanks. This removes even more code.
Attachment #397246 - Attachment is obsolete: true
Attachment #397428 - Attachment is obsolete: true
Attachment #397574 - Flags: superreview?(roc)
Attachment #397574 - Flags: review?(roc)
Attachment #397428 - Flags: superreview?(roc)
Attachment #397428 - Flags: review?(roc)
Assignee: nobody → ventnor.bugzilla
Attachment #397574 - Flags: superreview?(roc)
Attachment #397574 - Flags: superreview+
Attachment #397574 - Flags: review?(roc)
Attachment #397574 - Flags: review+
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
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?
(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
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
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
Status: RESOLVED → REOPENED
Flags: in-testsuite+
Resolution: FIXED → ---
Just FTR, this had also regressed Txul on Vista.
(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?
(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.
(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".
(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.
(Assignee)

Comment 18

9 years ago
Created attachment 400332 [details] [diff] [review]
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.
Attachment #397574 - Attachment is obsolete: true
Attachment #400332 - Flags: superreview?(roc)
Attachment #400332 - Flags: review?(roc)
Created attachment 400393 [details]
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.
(Assignee)

Comment 20

9 years ago
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.
(Assignee)

Comment 22

9 years ago
Created attachment 400454 [details] [diff] [review]
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.
Attachment #400332 - Attachment is obsolete: true
Attachment #400454 - Flags: superreview?(roc)
Attachment #400454 - Flags: review?(roc)
Attachment #400332 - Flags: superreview?(roc)
Attachment #400332 - Flags: review?(roc)
+    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?
More precisely, what if we used the line combined area (i.e. line overflow area) for lines and the frame overflow area for inlines?
(Assignee)

Comment 25

9 years ago
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.
Do you have a realistic example where the overflow rect would be much larger than necessary?
(Assignee)

Comment 27

9 years ago
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.
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.
(Assignee)

Comment 29

9 years ago
Created attachment 400472 [details] [diff] [review]
Patch 6
Attachment #400454 - Attachment is obsolete: true
Attachment #400472 - Flags: superreview?(roc)
Attachment #400472 - Flags: review?(roc)
Attachment #400454 - Flags: superreview?(roc)
Attachment #400454 - Flags: review?(roc)
Attachment #400472 - Flags: superreview?(roc)
Attachment #400472 - Flags: review?(roc)
Attachment #400472 - Flags: review+
this doesn't apply on trunk
(Assignee)

Comment 31

9 years ago
Created attachment 401441 [details] [diff] [review]
Patch 6 rebased
Attachment #400472 - Attachment is obsolete: true
(Assignee)

Comment 32

9 years ago
Dao, I rebased the patch. Could you please try again? Thanks in advance.
http://hg.mozilla.org/mozilla-central/rev/03707acbc593
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(Assignee)

Comment 34

9 years ago
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)
Attachment #401441 - Flags: approval1.9.2?
Comment on attachment 401441 [details] [diff] [review]
Patch 6 rebased

a1.9.2=dbaron
Attachment #401441 - Flags: approval1.9.2? → approval1.9.2+
Keywords: checkin-needed
Whiteboard: [needs 192 landing]
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/efbced394ad5
status1.9.2: --- → beta1-fixed
Keywords: checkin-needed
Whiteboard: [needs 192 landing]
You need to log in before you can comment on or make changes to this bug.