Closed
Bug 791434
Opened 12 years ago
Closed 12 years ago
Text shadow with transparency is rendered too light
Categories
(Core :: Layout: Text and Fonts, defect)
Tracking
()
VERIFIED
FIXED
mozilla18
Tracking | Status | |
---|---|---|
firefox16 | --- | unaffected |
firefox17 | + | fixed |
firefox18 | + | verified |
People
(Reporter: base12, Assigned: heycam)
References
Details
(Keywords: regression, testcase)
Attachments
(3 files, 2 obsolete files)
1.14 KB,
text/html
|
Details | |
4.50 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
4.51 KB,
patch
|
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0 SeaMonkey/2.15a1 Build ID: 20120914003005 Steps to reproduce: I have a page that uses a 20% opaque text shadow. Actual results: In recent trunk nightly builds I notice that the shadow is almost invisible, much lighter than it used to be, and much lighter than in Safari and Opera. Also much lighter than a corresponding box shadow. Expected results: The text shadow should be as dark as the corresponding box shadow, and as rendered by other browsers. See the attached HTML document, containing examples of text shadow and box shadow of the same color but rendered differently.
Updated•12 years ago
|
Attachment #661470 -
Attachment mime type: text/plain → text/html
Comment 1•12 years ago
|
||
I see this too. Andy, can you narrow down the regression range? Which build did you first see this?
Updated•12 years ago
|
I downloaded Windows Firefox nightlies until I could identify the first build where the bug shows up. The build is 20120809030541, 17.0a1 (2012-08-09). Previous nightly is OK.
Comment 3•12 years ago
|
||
Andy, go to about:buildconfig and let me know what "built from" is e.g. Built from http://hg.mozilla.org/mozilla-central/rev/4e150530c925
Here you go. Built from http://hg.mozilla.org/mozilla-central/rev/9fff2012b66c
(In reply to Andy Boze from comment #4) > Here you go. > > Built from http://hg.mozilla.org/mozilla-central/rev/9fff2012b66c Oops, never mind. I realized the nightly just updated itself. I'll reinstall and be back in a bit.
Here's the right info. Built from http://hg.mozilla.org/mozilla-central/rev/3199bc043da4
Comment 7•12 years ago
|
||
Regression window(m-c) Good: http://hg.mozilla.org/mozilla-central/rev/2dfbe4b9403b Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120808025427 Bad: http://hg.mozilla.org/mozilla-central/rev/b99a81e70b06 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120808092345 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2dfbe4b9403b&tochange=b99a81e70b06 Regression window(m-i) Good: http://hg.mozilla.org/integration/mozilla-inbound/rev/e001a7b3b817 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120807221927 Bad: http://hg.mozilla.org/integration/mozilla-inbound/rev/bd8ac25f41c4 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120808052727 Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e001a7b3b817&tochange=bd8ac25f41c4
Blocks: svgtext
tracking-firefox17:
--- → ?
tracking-firefox18:
--- → ?
Keywords: regressionwindow-wanted
Version: Trunk → 17 Branch
Assignee: nobody → cam
Updated•12 years ago
|
Assignee | ||
Comment 8•12 years ago
|
||
It seems like when there is no blur radius, we need to paint the text with the desired colour (shadowColor), but if there is blurring, we want to paint with solid black (to the alpha surface) and then mask that on to the target context using shadowColor. I'm not sure how my changes avoided this, or why the nsContextBoxBlur behaves like that.
Attachment #662444 -
Flags: review?(roc)
Comment on attachment 662444 [details] [diff] [review] patch Review of attachment 662444 [details] [diff] [review]: ----------------------------------------------------------------- The way this is supposed to work for clients of nsContextBoxBlur is that we draw into shadowContext using its current color --- which will be the text color if we're not blurring, or automatically solid black if we are blurring. But with the new code we're always setting shadowContext's current color to the text color. Can we not do that? Ideally the text drawing methods wouldn't even take a text color as a parameter, but would get the current color from the context if they need it (which they mostly shouldn't).
Assignee | ||
Comment 10•12 years ago
|
||
I would need to be able to get the current colour of the context to pass in to the callbacks for SVG text rendering when we are in callback mode. Can I actually do that? There's no GetColor() on gfxContext.
Comment 11•12 years ago
|
||
There's GetDeviceColor(), which you could retreive and then pass back to SetDeviceColor() later - would that serve?
Assignee | ||
Comment 12•12 years ago
|
||
Maybe, but the nscolors that I pass around for the text color also use the special color values that mean "foreground color" or "40% of foreground color". They might not survive being passed through here.
Then aren't we destroying those special values with your patch here?
Assignee | ||
Comment 14•12 years ago
|
||
No, because SVG text doesn't do text shadow (yet).
Assignee | ||
Comment 15•12 years ago
|
||
(And if/when it does, it would be done by invoking the callbacks rather than doing the shadow painting directly.)
Assignee | ||
Comment 16•12 years ago
|
||
Just updated the patch to fix a failing test.
Attachment #662527 -
Flags: review?(roc)
Assignee | ||
Comment 17•12 years ago
|
||
Comment on attachment 662444 [details] [diff] [review] patch >From 2bd49bd8da4bc6abe2bf77a79d40bb5a6209411a Mon Sep 17 00:00:00 2001 >From: Cameron McCormack <cam@mcc.id.au> >Date: Tue, 18 Sep 2012 08:24:02 +1000 >Subject: shadow fix > > >diff --git a/layout/generic/nsTextFrameThebes.cpp b/layout/generic/nsTextFrameThebes.cpp >index 087fee0..6ddd97f 100644 >--- a/layout/generic/nsTextFrameThebes.cpp >+++ b/layout/generic/nsTextFrameThebes.cpp >@@ -5248,29 +5248,31 @@ nsTextFrame::PaintOneShadow(PRUint32 aOffset, PRUint32 aLength, > shadowColor = aShadowDetails->mColor; > decorationOverrideColor = &shadowColor; > } else { > shadowColor = aForegroundColor; > decorationOverrideColor = nullptr; > } > > aCtx->Save(); >- aCtx->NewPath(); >+ aCtx->NewPath(); > aCtx->SetColor(gfxRGBA(shadowColor)); > > // Draw the text onto our alpha-only surface to capture the alpha values. > // Remember that the box blur context has a device offset on it, so we don't need to > // translate any coordinates to fit on the surface. > gfxFloat advanceWidth; > gfxRect dirtyRect(aDirtyRect.x, aDirtyRect.y, > aDirtyRect.width, aDirtyRect.height); > DrawText(shadowContext, dirtyRect, aFramePt + shadowOffset, > aTextBaselinePt + shadowOffset, aOffset, aLength, *aProvider, >- nsTextPaintStyle(this), shadowColor, aClipEdges, advanceWidth, >- (GetStateBits() & TEXT_HYPHEN_BREAK) != 0, decorationOverrideColor); >+ nsTextPaintStyle(this), >+ blurRadius == 0 ? shadowColor : NS_RGB(0, 0, 0), aClipEdges, >+ advanceWidth, (GetStateBits() & TEXT_HYPHEN_BREAK) != 0, >+ decorationOverrideColor); > > contextBoxBlur.DoPaint(); > aCtx->Restore(); > } > > // Paints selection backgrounds and text in the correct colors. Also computes > // aAllTypes, the union of all selection types that are applying to this text. > bool >diff --git a/layout/reftests/text-shadow/basic-opacity-ref.html b/layout/reftests/text-shadow/basic-opacity-ref.html >new file mode 100644 >index 0000000..11aa6d5 >--- /dev/null >+++ b/layout/reftests/text-shadow/basic-opacity-ref.html >@@ -0,0 +1,2 @@ >+<div style="position: absolute; top: 33px; left: 33px; color: rgba(0, 0, 255, 0.2); z-index: 0;">Hello</div> >+<div style="position: absolute; top: 30px; left: 30px; color: green; z-index: 1;">Hello</div> >diff --git a/layout/reftests/text-shadow/basic-opacity.html b/layout/reftests/text-shadow/basic-opacity.html >new file mode 100644 >index 0000000..1e2b12d >--- /dev/null >+++ b/layout/reftests/text-shadow/basic-opacity.html >@@ -0,0 +1 @@ >+<div style="position: absolute; top: 30px; left: 30px; color: green; text-shadow: rgba(0, 0, 255, 0.2) 3px 3px;">Hello</div> >diff --git a/layout/reftests/text-shadow/blur-opacity-ref.html b/layout/reftests/text-shadow/blur-opacity-ref.html >new file mode 100644 >index 0000000..0039865 >--- /dev/null >+++ b/layout/reftests/text-shadow/blur-opacity-ref.html >@@ -0,0 +1 @@ >+<div style="color: transparent; text-shadow: blue 4px 4px 2px; opacity: 0.5">The shadow should be blurred</div> >diff --git a/layout/reftests/text-shadow/blur-opacity.html b/layout/reftests/text-shadow/blur-opacity.html >new file mode 100644 >index 0000000..6713bc9 >--- /dev/null >+++ b/layout/reftests/text-shadow/blur-opacity.html >@@ -0,0 +1 @@ >+<div style="color: transparent; text-shadow: rgba(0, 0, 255, 0.5) 4px 4px 2px">The shadow should be blurred</div> >diff --git a/layout/reftests/text-shadow/reftest.list b/layout/reftests/text-shadow/reftest.list >index a329513..8984946 100644 >--- a/layout/reftests/text-shadow/reftest.list >+++ b/layout/reftests/text-shadow/reftest.list >@@ -1,16 +1,18 @@ > == basic.xul basic-ref.xul > random-if(Android) == basic-negcoord.xul basic-negcoord-ref.xul > != blur.xul blur-notref.xul > == color-inherit.xul color-inherit-ref.xul > == multiple-noblur.xul multiple-noblur-ref.xul >+== blur-opacity.html blur-opacity-ref.html > > == basic.html basic-ref.html > == basic-negcoord.html basic-negcoord-ref.html >+== basic-opacity.html basic-opacity-ref.html > != blur.html blur-notref.html > == color-inherit.html color-inherit-ref.html > == color-parserorder.html color-parserorder-ref.html > == decorations-multiple-zorder.html decorations-multiple-zorder-ref.html > == multiple-noblur.html multiple-noblur-ref.html > == quirks-decor-noblur.html quirks-decor-noblur-ref.html > == standards-decor-noblur.html standards-decor-noblur-ref.html > == padding-decoration.html padding-decoration-ref.html >-- >1.7.4.2 >
Attachment #662444 -
Attachment is obsolete: true
Attachment #662444 -
Flags: review?(roc)
OK, let's go with something like the current patch, but instead of testing blurRadius == 0 let's more directly test whether we're doing pass-through by checking aCtx == shadowContext.
Assignee | ||
Comment 19•12 years ago
|
||
OK.
Attachment #662527 -
Attachment is obsolete: true
Attachment #662527 -
Flags: review?(roc)
Attachment #662568 -
Flags: review?(roc)
Attachment #662568 -
Flags: review?(roc) → review+
Assignee | ||
Comment 20•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d20023a4fda6
Assignee | ||
Updated•12 years ago
|
Status: NEW → ASSIGNED
OS: Windows 7 → All
Hardware: x86_64 → All
Whiteboard: [leave open]
Comment 21•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf1cd486d60e for https://tbpl.mozilla.org/php/getParsedLog.php?id=15372150&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=15373736&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=15374756&tree=Mozilla-Inbound
Assignee | ||
Comment 22•12 years ago
|
||
Tweaked the test to pass on Android: https://hg.mozilla.org/integration/mozilla-inbound/rev/7eabbf24ef96
Comment 23•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7eabbf24ef96
Flags: in-testsuite+
Assignee | ||
Comment 24•12 years ago
|
||
[Approval Request Comment] Bug caused by (feature/regressing bug #): 655877 Part 26 User impact if declined: Text shadows render incorrectly. Testing completed (on m-c, etc.): Only been on m-c for a few hours, but regression tests have been added. Risk to taking this patch (and alternatives if risky): Low; test file in bug report verified as working now, and the new regression tests also check this. String or UUID changes made by this patch: none
Attachment #663967 -
Flags: approval-mozilla-aurora?
Updated•12 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
status-firefox16:
--- → unaffected
status-firefox17:
--- → affected
status-firefox18:
--- → fixed
Resolution: --- → FIXED
Updated•12 years ago
|
Attachment #663967 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 25•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/a853c1b3957b
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Comment 26•12 years ago
|
||
Backed out of Aurora for Windows Debug reftest failure: https://hg.mozilla.org/releases/mozilla-aurora/rev/3440e0b0cdb3
Comment 27•12 years ago
|
||
Pushed again in Aurora: http://hg.mozilla.org/releases/mozilla-aurora/rev/b39c9b481091
Assignee | ||
Comment 28•12 years ago
|
||
And to bring the tweaked tests on trunk in line with what re-landed on Aurora: https://hg.mozilla.org/integration/mozilla-inbound/rev/dcdda4778cef
Whiteboard: [leave open]
Comment 29•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/dcdda4778cef
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Comment 30•11 years ago
|
||
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0 The testcase works as expected. Verified on Windows 7, Ubuntu and Mac OS with Firefox beta 6.
Status: RESOLVED → VERIFIED
Updated•11 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•