666689 - Implement text-shadow for the text-overflow marker text (ellipsis)

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect)

Description

[Mats Palmgren (inactive)]

Follow-up from bug 312156.

Implement text-shadow for the text-overflow marker text (ellipsis).

Comment 1

[Michael Ventnor]

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]

Attachment: 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) (email my personal email if necessary)]

Can we factor out shadow-painting code so we don't have to write all-new code here?

Comment 4

[Michael Ventnor]

(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 (inactive)]

I don't mind at all, this looks great!  Thanks.

Comment 6

[Michael Ventnor]

(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) (email my personal email if necessary)]

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]

Attachment: 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) (email my personal email if necessary)]

Comment on attachment 543359 [details] [diff] [review]
Patch 2

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

Comment 10

[Michael Ventnor]

http://hg.mozilla.org/mozilla-central/rev/2fba916e056c

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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]

Attachment: 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) (email my personal email if necessary)]

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 14

[Michael Ventnor]

http://hg.mozilla.org/integration/mozilla-inbound/rev/dfcbb9671e02

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/dfcbb9671e02