Closed Bug 666689 Opened 9 years ago Closed 9 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: mats, Assigned: ventnor.bugzilla)

References

Details

(Keywords: css3)

Attachments

(2 files, 1 obsolete file)

Follow-up from bug 312156.

Implement text-shadow for the text-overflow marker text (ellipsis).
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...)
Attached patch Patch (obsolete) — Splinter Review
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...
Assignee: matspal → ventnor.bugzilla
Attachment #543076 - Flags: review?(roc)
Can we factor out shadow-painting code so we don't have to write all-new code here?
(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?
I don't mind at all, this looks great!  Thanks.
(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.
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*).
Attached patch Patch 2Splinter Review
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.
Attachment #543076 - Attachment is obsolete: true
Attachment #543359 - Flags: review?(roc)
Attachment #543076 - Flags: review?(roc)
Comment on attachment 543359 [details] [diff] [review]
Patch 2

Review of attachment 543359 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #543359 - Flags: review?(roc) → review+
http://hg.mozilla.org/mozilla-central/rev/2fba916e056c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 669015
Depends on: 669038
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...
Attached patch New reftestSplinter Review
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?)
Attachment #543714 - Flags: review?(roc)
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.
Attachment #543714 - Flags: review?(roc) → review+
You need to log in before you can comment on or make changes to this bug.