Closed Bug 512988 Opened 13 years ago Closed 13 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: joseph, Assigned: ventnor.bugzilla)

References

Details

Attachments

(3 files, 6 obsolete files)

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
Component: General → Layout: Text
Product: Firefox → Core
QA Contact: general → layout.fonts-and-text
Version: unspecified → Trunk
Confirmed on trunk
Blocks: text-shadow
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
OS: Windows XP → All
Hardware: x86 → All
Attached patch Patch (obsolete) — Splinter Review
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)
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+
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.
Attached patch Patch 2 (obsolete) — Splinter Review
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.
Attached patch Patch 3 (obsolete) — Splinter Review
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+
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
Closed: 13 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.
Attached patch Patch 4 (obsolete) — Splinter Review
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)
Attached file 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.
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.
Attached patch Patch 5 (obsolete) — Splinter Review
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?
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?
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.
Attached patch Patch 6 (obsolete) — Splinter Review
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
Attached patch Patch 6 rebasedSplinter Review
Attachment #400472 - Attachment is obsolete: true
Dao, I rebased the patch. Could you please try again? Thanks in advance.
http://hg.mozilla.org/mozilla-central/rev/03707acbc593
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
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]
You need to log in before you can comment on or make changes to this bug.