Text shadow with transparency is rendered too light

VERIFIED FIXED in Firefox 17

Status

()

Core
Layout: Text
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Andy Boze, Assigned: heycam)

Tracking

({regression, testcase})

17 Branch
mozilla18
regression, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox16 unaffected, firefox17+ fixed, firefox18+ verified)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 661470 [details]
textshadow.html

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

5 years ago
Attachment #661470 - Attachment mime type: text/plain → text/html

Comment 1

5 years ago
I see this too.

Andy, can you narrow down the regression range? Which build did you first see this?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression

Updated

5 years ago
Component: General → Layout: Text
Keywords: regressionwindow-wanted
Product: SeaMonkey → Core
(Reporter)

Comment 2

5 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

5 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
(Reporter)

Comment 4

5 years ago
Here you go.

Built from http://hg.mozilla.org/mozilla-central/rev/9fff2012b66c
(Reporter)

Comment 5

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

Comment 6

5 years ago
Here's the right info.

Built from http://hg.mozilla.org/mozilla-central/rev/3199bc043da4

Comment 7

5 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: 655877
tracking-firefox17: --- → ?
tracking-firefox18: --- → ?
Keywords: regressionwindow-wanted
Version: Trunk → 17 Branch
Keywords: testcase
Assignee: nobody → cam

Updated

5 years ago
tracking-firefox17: ? → +
tracking-firefox18: ? → +
(Assignee)

Comment 8

5 years ago
Created attachment 662444 [details] [diff] [review]
patch

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

5 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.
There's GetDeviceColor(), which you could retreive and then pass back to SetDeviceColor() later - would that serve?
(Assignee)

Comment 12

5 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

5 years ago
No, because SVG text doesn't do text shadow (yet).
(Assignee)

Comment 15

5 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

5 years ago
Created attachment 662527 [details] [diff] [review]
patch (v1.1)

Just updated the patch to fix a failing test.
Attachment #662527 - Flags: review?(roc)
(Assignee)

Comment 17

5 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

5 years ago
Created attachment 662568 [details] [diff] [review]
patch (v1.2)

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

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d20023a4fda6
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
OS: Windows 7 → All
Hardware: x86_64 → All
Whiteboard: [leave open]
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

5 years ago
Tweaked the test to pass on Android:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7eabbf24ef96
https://hg.mozilla.org/mozilla-central/rev/7eabbf24ef96
Flags: in-testsuite+
(Assignee)

Comment 24

5 years ago
Created attachment 663967 [details] [diff] [review]
checked-in patch

[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

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox16: --- → unaffected
status-firefox17: --- → affected
status-firefox18: --- → fixed
Resolution: --- → FIXED

Updated

5 years ago
Attachment #663967 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 25

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/a853c1b3957b
(Assignee)

Updated

5 years ago
status-firefox17: affected → fixed
(Assignee)

Comment 26

5 years ago
Backed out of Aurora for Windows Debug reftest failure:

https://hg.mozilla.org/releases/mozilla-aurora/rev/3440e0b0cdb3
Status: RESOLVED → REOPENED
status-firefox17: fixed → affected
Resolution: FIXED → ---

Comment 27

5 years ago
Pushed again in Aurora:
http://hg.mozilla.org/releases/mozilla-aurora/rev/b39c9b481091
status-firefox17: affected → fixed
(Assignee)

Comment 28

5 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]
https://hg.mozilla.org/mozilla-central/rev/dcdda4778cef
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18

Comment 30

5 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

5 years ago
status-firefox18: fixed → verified
You need to log in before you can comment on or make changes to this bug.