Last Comment Bug 601512 - background of text inside box-shadow becomes black after highlight
: background of text inside box-shadow becomes black after highlight
Status: RESOLVED FIXED
[fixed by Cairo 1.10 update in bug 56...
: regression, testcase
Product: Core
Classification: Components
Component: Layout: View Rendering (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla7
Assigned To: Mats Palmgren (:mats)
:
Mentors:
Depends on: 562746 628745
Blocks:
  Show dependency treegraph
 
Reported: 2010-10-03 14:47 PDT by Brandon Cheng
Modified: 2011-07-20 14:13 PDT (History)
11 users (show)
mats: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
4 testcase demos. (4.37 KB, text/html)
2010-10-03 15:55 PDT, Brandon Cheng
no flags Details
Testcase with both -moz- and no prefix in box-shadow (4.57 KB, text/html)
2010-10-03 17:20 PDT, Pedro Arvela
no flags Details
workaround (1.41 KB, patch)
2010-12-08 16:00 PST, Mats Palmgren (:mats)
no flags Details | Diff | Review
partial workaround (1.29 KB, patch)
2011-01-14 18:45 PST, Mats Palmgren (:mats)
no flags Details | Diff | Review
reset the current clip before painting an outer box shadow (1004 bytes, patch)
2011-01-25 16:24 PST, Mats Palmgren (:mats)
no flags Details | Diff | Review
Testcase #3, the first case here is fixed by attachment 504053 (2.56 KB, text/html)
2011-01-26 09:23 PST, Mats Palmgren (:mats)
no flags Details

Description Brandon Cheng 2010-10-03 14:47:03 PDT
User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b7pre) Gecko/20101002 Firefox/4.0b7pre
Build Identifier: 

Text inside a block shadow will have a black background after it is selected, and then unselected.

Reproducible: Always

Steps to Reproduce:
1. Open up Minefield/Firefox Beta 7 Pre
2. Navigate to a page with the error
3. Highlight some text
4. Un-highlight by clicking elsewhere
Actual Results:  
background of highlighted text becomes black

Expected Results:  
Return text background to original color
Comment 1 Mike Shaver (:shaver -- probably not reading bugmail closely) 2010-10-03 14:50:04 PDT
Confirmed.  Brandon: could you create a small HTML page that shows the problem, and attach it?
Comment 2 Brandon Cheng 2010-10-03 15:55:12 PDT
Created attachment 480535 [details]
4 testcase demos.
Comment 3 Brandon Cheng 2010-10-03 15:59:29 PDT
Comment on attachment 480535 [details]
4 testcase demos.

My friend Joseph Peter created this. I created my own, but his beat mine by a mile in quality. The bug seems to be also related to rounded box corners.

It also occurs when the box is out of sight after scrolling down the page. Even more strange, the black background returns normal after highlighting the sidebar and the content.
Comment 4 j.j. 2010-10-03 16:53:52 PDT
BTW, use also "box-shadow" in testcases, the -moz-prefix is dropped.
Comment 5 Pedro Arvela 2010-10-03 17:20:12 PDT
Created attachment 480545 [details]
Testcase with both -moz- and no prefix in box-shadow

Just a quick update from the previous testcase, this one features -moz-box-shadow followed by box-shadow so it can be compared in both Firefox 3.6.x and 4.0x

When I did the original testcase for Brandon, I forgot to include the no prefix property, although I intended to.

The behavior I see in my computer is the testcase to work as expected on Firefox 3.6.10 from the Ubuntu repositories. In Minefield 4.0b7pre the first "block" appears as expected until text is selected or is scrolled off sight and on again, in which the background turns black; the second "block" is always showing a black background and the shadow does not correspond to what is expected from the CSS; while the last 2 "blocks" are shown as expected.
Comment 6 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-10-03 22:07:24 PDT
Might the regression range match the regression range in bug 536061?  (See bug 536061 comment 13.)
Comment 7 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-10-05 08:15:58 PDT
Regression range doesn't match (this is a more recent regression), but it still could be the same underlying problem.
Comment 8 Brandon Cheng 2010-10-05 12:37:07 PDT
David: It seems similar (from reading comment 10), perhaps we should wait to see if fixing bug 536061 fixes this one?
Comment 9 Mats Palmgren (:mats) 2010-11-15 07:26:31 PST
"Patch rev. 1" in bug 536061 does not fix this bug.
Comment 10 Timothy Nikkel (:tnikkel) 2010-11-15 15:20:23 PST
Regression range on Windows
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8e0fce7d5b49&tochange=79d0beec27b5
Comment 11 Timothy Nikkel (:tnikkel) 2010-11-15 15:21:57 PST
I'm guessing David's border radius changes.
Comment 12 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-11-15 15:27:06 PST
Those are the changes that caused border-radius + overflow:hidden to set a rounded clip region, so it's probably a preëxisting problem with complex clip regions.
Comment 13 Mats Palmgren (:mats) 2010-12-08 16:00:07 PST
Created attachment 496337 [details] [diff] [review]
workaround

dbaron is right - this is a bug in Cairo.
When the box shadow is completely clipped AND the clip has a RoundedRectangle
then the Fill appears to be confused as to which areas to fill.
Fwiw, after debugging I found this workaround...

But then I saw that we will update to Cairo 1.10 in bug 562746 so
I tried that (patch v5) and it fixes this bug.
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-12-08 18:15:07 PST
We should probably figure out which cairo patch fixed the bug, so we can backport it now in case bug 562746 doesn't land.
Comment 15 Mats Palmgren (:mats) 2010-12-08 21:21:46 PST
The clip related changes looks quite substantial though.
https://bugzilla.mozilla.org/attachment.cgi?id=490975&action=diff#a/gfx/cairo/cairo/src/cairo-clip.c_sec2

It looks like we're missing most of the changes in 2010:
http://cgit.freedesktop.org/cairo/log/src/cairo-clip.c?id=108b1c7825116ed3f93aa57384bbd3290cdc9181
(except the fixes Karl contributed)
Comment 16 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-01-11 12:27:46 PST
The patch in comment #13 would be a potentially useful optimization so I think we should just take it. However, we should make it handle cases where the frame has border-radius. I think we just need to check whether 'clip' is inside skipGfxRect instead of frameGfxRect.
Comment 17 Mats Palmgren (:mats) 2011-01-14 18:44:11 PST
Fwiw, I did apply a few of the smaller patches from the Cairo tree
to see if it would fix this bug, but no luck...

I tried a few of the larger patches involving cairo-clip.c also,
but every time I ended up pulling in more and more unrelated
stuff just to make it compile...  and I doubt any patch of that
size would be accepted at this point...

So, I have given up on trying to fix this at the Cairo level.
Comment 18 Mats Palmgren (:mats) 2011-01-14 18:45:00 PST
Created attachment 504053 [details] [diff] [review]
partial workaround

Unfortunately this workaround seems to be less effective now -
it really only fixes a minority of cases I think.
It doesn't fix either of border-radius cases in the attached testcase.

I haven't measured if this pays off as an optimization.
Comment 19 Mats Palmgren (:mats) 2011-01-25 16:24:25 PST
Created attachment 506955 [details] [diff] [review]
reset the current clip before painting an outer box shadow

Bug 628745 probably helps in some cases, but not for the testcases in
this bug unfortunately.

Resetting the current clip before painting the outer box shadow fixes
this bug - it's the wrong thing to do of course, but perhaps the lesser
evil?  Perhaps we can limit this to the cases where the current clip
is rounded -- is there a way to find that out?

The two workarounds in this bug + the fix in bug 628745 passed reftests
locally on Linux64.  I've submitted that to TryServer.
Comment 20 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-01-25 16:49:34 PST
I don't understand why 628745 wouldn't fix attachment 480545 [details]. The text there doesn't intersect the corners, so the patch in bug 628745 should never be rendering it with a rounded clip rect. Can you explain this?
Comment 21 Timothy Nikkel (:tnikkel) 2011-01-25 16:57:05 PST
The box shadow does though, it seems.
Comment 22 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-01-25 17:08:50 PST
Hmm, yeah. But this is a somewhat unusual testcase in that respect, I think. I think maybe with bug 628745 fixed, we could take this off the blocker list.

Alternatively, it seems to me that none of the *visible* part of the shadow intersects the rounded corners, so we could probably do more analysis and discover that.
Comment 23 Mats Palmgren (:mats) 2011-01-26 09:23:48 PST
Created attachment 507154 [details]
Testcase #3, the first case here is fixed by attachment 504053 [details] [diff] [review]
Comment 24 Mats Palmgren (:mats) 2011-01-26 09:49:49 PST
This looks more and more like an edge case to me.  It appears the
problem only occurs when a box shadow is clipped by an ancestor
with overflow != visible and border-radius != 0.

I haven't seen any reports of this bug occurring on any major sites
and the problem should be relatively easy to workaround with a few
style changes.  I don't think this should be blocking 2.0.
Requesting re-triage of blocking status.

FWIW, the TryServer job with the two wallpapers in this bug +
the fix in bug 628745 passed unit tests.
Comment 25 Mats Palmgren (:mats) 2011-07-20 14:13:18 PDT
[fixed by Cairo 1.10 update in bug 562746]

Note You need to log in before you can comment on or make changes to this bug.