Last Comment Bug 666689 - Implement text-shadow for the text-overflow marker text (ellipsis)
: Implement text-shadow for the text-overflow marker text (ellipsis)
Status: RESOLVED FIXED
: css3
Product: Core
Classification: Components
Component: Layout: Block and Inline (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: ---
Assigned To: Michael Ventnor
:
Mentors:
Depends on: 669015 669038
Blocks: 312156
  Show dependency treegraph
 
Reported: 2011-06-23 11:37 PDT by Mats Palmgren (vacation)
Modified: 2011-07-04 05:09 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (7.91 KB, patch)
2011-06-30 00:07 PDT, Michael Ventnor
no flags Details | Diff | Splinter Review
Patch 2 (18.26 KB, patch)
2011-06-30 23:11 PDT, Michael Ventnor
roc: review+
Details | Diff | Splinter Review
New reftest (3.86 KB, patch)
2011-07-03 20:51 PDT, Michael Ventnor
roc: review+
Details | Diff | Splinter Review

Description Mats Palmgren (vacation) 2011-06-23 11:37:04 PDT
Follow-up from bug 312156.

Implement text-shadow for the text-overflow marker text (ellipsis).
Comment 1 Michael Ventnor 2011-06-29 02:28:30 PDT
Mats, will you be able to fix this before the Fx7 merge?

If not, I might be able to do it (no promises though, I have my own big thing I'm working on...)
Comment 2 Michael Ventnor 2011-06-30 00:07:06 PDT
Created attachment 543076 [details] [diff] [review]
Patch

I hope you don't mind, Mats, but I really want this fixed for Fx7 not only for when we use this in the UI (we use text-shadow almost everywhere now, and I'm sure our current overflow hax will be replaced with text-overflow soon), but also because text-shadow has become very popular on the web, and I firmly believe many devs still won't use text-overflow unless they can get the text looking precisely the way they want.

Shadows and gradients everywhere. Everyone wants to be like Apple...
Comment 3 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-30 00:23:54 PDT
Can we factor out shadow-painting code so we don't have to write all-new code here?
Comment 4 Michael Ventnor 2011-06-30 00:39:43 PDT
(In reply to comment #3)
> Can we factor out shadow-painting code so we don't have to write all-new
> code here?

It's already factored out into nsContextBoxBlur.
If you meant all the stuff before that, I can probably do that but it looks like it'll need a few out parameters, is that OK?
Comment 5 Mats Palmgren (vacation) 2011-06-30 00:47:21 PDT
I don't mind at all, this looks great!  Thanks.
Comment 6 Michael Ventnor 2011-06-30 01:24:27 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > Can we factor out shadow-painting code so we don't have to write all-new
> > code here?
> 
> It's already factored out into nsContextBoxBlur.
> If you meant all the stuff before that, I can probably do that but it looks
> like it'll need a few out parameters, is that OK?

Actually, I don't think this is a good idea.
The problem is every part of the code has different ways of drawing text. Also, every variable needs to be used (notice how we must add the shadow offset to the text baseline), so in fact factoring this sort of code may be more trouble than its worth.
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-30 03:09:07 PDT
I think you can share code with nsTextBoxFrame's shadow drawing. The actual text drawing (call to DrawText in nsTextBoxFrame, call to nsLayoutUtils::DrawString here) should be wrapped up into a callback function that you pass in. This callback function would take as parameters an nsRenderingContext*, an nsPoint offset, an nscolor for the shadow color, and a void* specific to the callback function (either the nsDisplayTextOverflowMarker*, or the nsTextBoxFrame*).
Comment 8 Michael Ventnor 2011-06-30 23:11:32 PDT
Created attachment 543359 [details] [diff] [review]
Patch 2

I did originally think of doing something like that, but convinced myself it wouldn't work because of all the different ways that text is drawn. Apparently you've proved me wrong yet again roc.
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-30 23:26:35 PDT
Comment on attachment 543359 [details] [diff] [review]
Patch 2

Review of attachment 543359 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-07-03 14:11:08 PDT
Actually, couldn't we have a proper reftest here? I mean, you can use a monospace font like the other text-overflow tests do, then check that with a text-shadow, HelloKitty with text-overflow:"..." in a div of width 8.5em renders the same as Hello...
Comment 12 Michael Ventnor 2011-07-03 20:51:27 PDT
Created attachment 543714 [details] [diff] [review]
New reftest

Yeah, I thought about this some more and managed to come up with a working == reftest.

(Just an aside, why do we need overflow:hidden to get text-overflow? Can't we get it just by setting a width on the box?)
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-07-03 21:17:26 PDT
Comment on attachment 543714 [details] [diff] [review]
New reftest

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

text-overflow should really be named text-clip ... it only takes effect when text is clipped.
Comment 15 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-07-04 05:09:15 PDT
http://hg.mozilla.org/mozilla-central/rev/dfcbb9671e02

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