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

RESOLVED FIXED

Status

()

Core
Layout: Block and Inline
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mats, Assigned: Michael Ventnor)

Tracking

({css3})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Follow-up from bug 312156.

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

Comment 1

6 years ago
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...)
(Assignee)

Comment 2

6 years ago
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...
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?
(Assignee)

Comment 4

6 years ago
(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?
(Reporter)

Comment 5

6 years ago
I don't mind at all, this looks great!  Thanks.
(Assignee)

Comment 6

6 years ago
(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*).
(Assignee)

Comment 8

6 years ago
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.
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+
(Assignee)

Comment 10

6 years ago
http://hg.mozilla.org/mozilla-central/rev/2fba916e056c
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Depends on: 669015

Updated

6 years ago
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...
(Assignee)

Comment 12

6 years ago
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?)
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+
(Assignee)

Comment 14

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/dfcbb9671e02
http://hg.mozilla.org/mozilla-central/rev/dfcbb9671e02
You need to log in before you can comment on or make changes to this bug.