Bug 61776 - text-shadow disappears if CSS uses ::selection { text-shadow: none } on a @font-face font
Summary: text-shadow disappears if CSS uses ::selection { text-shadow: none } on a @fo...
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kenichi Ishibashi
URL: http://crbug.com/84384
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-31 03:14 PDT by Kenichi Ishibashi
Modified: 2014-04-01 19:01 PDT (History)
3 users (show)

See Also:


Attachments
Patch V0 (4.80 KB, patch)
2011-05-31 03:24 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch V1 (14.00 KB, patch)
2011-06-06 22:18 PDT, Kenichi Ishibashi
simon.fraser: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenichi Ishibashi 2011-05-31 03:14:28 PDT
Reported at http://crbug.com/84384.

The cause is in FrameSelection::recomputeCaretRect().  This method invalidates the rect in which the caret is supposed to be rendered, even if the caret never appears.  We only need to invalidate the rect when the caret is certainly rendered.
Comment 1 Kenichi Ishibashi 2011-05-31 03:24:13 PDT
Created attachment 95412 [details]
Patch V0
Comment 2 Kenichi Ishibashi 2011-05-31 03:31:55 PDT
(In reply to comment #1)
> Created an attachment (id=95412) [details]
> Patch V0

The test of this patch might pass the run-webkit-tests even the patch are not applied.  I checked the test failed on Chromium (mac/linux) and Safari w/o the patch.  I'd like to ask some advice to correct the test.

Regards,
Comment 3 Hajime Morrita 2011-06-06 19:05:22 PDT
At this time, I recommend you not to use reftests. It's not ready on buildbots.
Comment 4 Hajime Morrita 2011-06-06 19:06:15 PDT
Comment on attachment 95412 [details]
Patch V0

r- for now to clear up the queue.
Comment 5 Kenichi Ishibashi 2011-06-06 20:48:25 PDT
Hi Morita-san,

Thank you for review.

(In reply to comment #3)
> At this time, I recommend you not to use reftests. It's not ready on buildbots.

I see.  Then, I think this bug should be checked by a pixel test.  How can I make -expected.png for each platform?  This bug is platform-independent.
Comment 6 Kenichi Ishibashi 2011-06-06 22:18:20 PDT
Created attachment 96201 [details]
Patch V1
Comment 7 Hajime Morrita 2011-06-07 03:25:19 PDT
Comment on attachment 96201 [details]
Patch V1

View in context: https://bugs.webkit.org/attachment.cgi?id=96201&action=review

Code looks simple enough.
I'd like to hear from experts.

> LayoutTests/fast/repaint/text-shadow-selection.html:30
> +        }, 100);

unless there is special reasons,  you shouldn't use setTimeout with non-zero wait time - it actually spend that time and make a test slow.
In many case it can be replaced with layoutTestController.leapForward().
Comment 8 Kenichi Ishibashi 2011-06-07 04:04:18 PDT
Comment on attachment 96201 [details]
Patch V1

View in context: https://bugs.webkit.org/attachment.cgi?id=96201&action=review

>> LayoutTests/fast/repaint/text-shadow-selection.html:30
>> +        }, 100);
> 
> unless there is special reasons,  you shouldn't use setTimeout with non-zero wait time - it actually spend that time and make a test slow.
> In many case it can be replaced with layoutTestController.leapForward().

I tried to use zero wait time out but I couldn't see the expected behavior in which the text shadow breaks up when I ran the test without the patch.  I agree with your concern so I'd be grad if someone could give suggestions to revise the test.
Comment 9 Simon Fraser (smfr) 2011-06-07 07:53:38 PDT
Comment on attachment 96201 [details]
Patch V1

View in context: https://bugs.webkit.org/attachment.cgi?id=96201&action=review

> LayoutTests/fast/repaint/text-shadow-selection.html:13
> +.textshadow {
> +  text-shadow: 10px 0px red;
> +  font-family: sans-serif;
> +  font-size: 36pt;
> +  font-weight: bold;
> +}
> +::selection {
> +  background: #ccc;
> +  text-shadow: none;
> +}

There is no @font-face in this testcase, so is the bug title no longer accurate?

> Source/WebCore/editing/FrameSelection.cpp:1218
> +            view->repaintRectangleInViewAndCompositedLayers(oldAbsoluteCaretRepaintBounds, false);
>              view->repaintRectangleInViewAndCompositedLayers(m_absoluteCaretRepaintBounds, false);

This change doesn't seem right. An extra invalidate via repaintRectangleInViewAndCompositedLayers() should not cause repaint failure, only extra work. By making this change, you're opening up the possibility that we'll leave stale images of the caret around.