Last Comment Bug 692752 - text selection highlighting should paint behind text-shadow
: text selection highlighting should paint behind text-shadow
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Jonathan Kew (:jfkthame)
:
Mentors:
Depends on: 738239 810564 857924 721750 755994
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-07 03:56 PDT by Jonathan Kew (:jfkthame)
Modified: 2013-04-04 10:19 PDT (History)
3 users (show)
Ms2ger: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch, paint text-shadow after selection highlight background (4.74 KB, patch)
2011-10-07 10:43 PDT, Jonathan Kew (:jfkthame)
roc: review+
Details | Diff | Review
reftest for painting selected text with shadow (3.46 KB, patch)
2011-10-08 02:05 PDT, Jonathan Kew (:jfkthame)
roc: review+
Details | Diff | Review
reftest v2 - updated to use white text (3.80 KB, patch)
2011-10-08 09:24 PDT, Jonathan Kew (:jfkthame)
jfkthame: review+
Details | Diff | Review

Description Jonathan Kew (:jfkthame) 2011-10-07 03:56:02 PDT
Currently, it appears that the text-selection highlight is painted on top of any text-shadow in effect. IMO, this makes for a jarring user experience when selecting text that uses shadows. I note that Safari paints the shadow on top of the selection highlight, and I believe this is preferable behavior.

Simple demonstration:
data:text/html,<p style="font-size:40px; text-shadow:2px 2px 1px red;">Hello, shadowed world</p>

Drag back and forth across the text to select/unselect part of it, and note how the shadow is "lost" under the selection highlight. Compare behavior in Safari.
Comment 1 Jonathan Kew (:jfkthame) 2011-10-07 10:43:24 PDT
Created attachment 565577 [details] [diff] [review]
patch, paint text-shadow after selection highlight background

Something like this ought to improve the user experience - seems to work in simple testing, at least, and passes unit tests on try.
Comment 2 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-10-07 13:43:22 PDT
Comment on attachment 565577 [details] [diff] [review]
patch, paint text-shadow after selection highlight background

Review of attachment 565577 [details] [diff] [review]:
-----------------------------------------------------------------

Reftest? Should at least be able to use a != test
Comment 3 Jonathan Kew (:jfkthame) 2011-10-08 02:05:44 PDT
Created attachment 565721 [details] [diff] [review]
reftest for painting selected text with shadow

I think we can test in both != and == forms, by faking the reference rendering using two overlaid divs. Pushed this to tryserver to check that it passes on various platforms (worked ok locally).
Comment 4 Jonathan Kew (:jfkthame) 2011-10-08 06:09:11 PDT
(In reply to Jonathan Kew (:jfkthame) from comment #3)
> Created attachment 565721 [details] [diff] [review] [diff] [details] [review]
> reftest for painting selected text with shadow
> 
> I think we can test in both != and == forms, by faking the reference
> rendering using two overlaid divs. Pushed this to tryserver to check that it
> passes on various platforms (worked ok locally).

Hmm, the == test fails on platforms where the text color is automagically changed to white when selected. Making the text explicitly "color:white" solves this. Re-running tryserver job to verify there are no other surprises.
Comment 5 Jonathan Kew (:jfkthame) 2011-10-08 09:24:43 PDT
Created attachment 565743 [details] [diff] [review]
reftest v2 - updated to use white text

Updated to use color:white; now passes on all platforms. Carrying forward r=roc.

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