Closed Bug 1426146 Opened 6 years ago Closed 6 years ago

support the paint-order property for non-SVG text as well as SVG

Categories

(Core :: Layout: Text and Fonts, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: jfkthame, Assigned: jfkthame)

References

Details

(Keywords: dev-doc-complete)

Attachments

(3 files, 1 obsolete file)

I think we should consider implementing paint-order (to control whether stroke is painted before or after fill) for glyph rendering in HTML text, similarly to how it can be used in SVG.

This is currently noted as an issue in the Fill and Stroke spec.[1]

Implementing this would provide authors with a simple solution to many of the kind of use cases seen in bug 1426073, where stroke-align:outset (probably much harder to spec and implement) is being requested.

[1] https://www.w3.org/TR/fill-stroke-3/#strokes
"stroke-align:outset" is easy to spec and pretty easy to implement for text.
I have a couple of patches that seem to work pretty well here.

fantasai, any current thoughts on whether it makes sense to move forward with this (i.e. answering "yes" to https://www.w3.org/TR/fill-stroke-3/#issue-57580890)? heycam, any opinion?

(I'll plan to write up an Intent to Implement for dev-platform, too.)
Flags: needinfo?(fantasai.bugs)
Flags: needinfo?(cam)
Seems reasonable to me. :) I think we were just asking that question because it wasn't something that anyone else had talked about before we drafted up that section; your answer is good enough to close the issue, I think.
Flags: needinfo?(fantasai.bugs)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
I hope that Firefox (etc) will support multiple outset strokes for text (for HTML and ideally also for SVG).

A use case example: Two strokes implemented using a workaround:
(Works eg in Chrome and in Nightly)
The h1 heading at https://tobireif.com/demos/grid/ .
(Or the stable example page https://tobireif.com/non_site_stuff/text-stroke_cut_off_example_for_bug-reports/grid.html .)

These W3C texts seem related:

From
https://www.w3.org/TR/SVG2/painting.html#SpecifyingPaint
"SVG 2 Addition:" "Allow multiple paints in fill and stroke."
"Resolution:" "We will allow multiple paints in the fill and stroke properties in SVG 2."
"This was dropped for SVG 2, but will be added later in sync with CSS Fill and Stroke Level 3"

https://www.w3.org/TR/fill-stroke-3/#stroke-layering
"The stroke of a box can have multiple layers. The number of layers is determined by the larger of the number of comma-separated values for the stroke-image property and the number of comma-separated values for the stroke-color property. A value of none still creates a layer."
Just FTR, the CSS WG resolved to accept paint-order: https://github.com/w3c/fxtf-drafts/issues/107#issuecomment-355171260.
Attachment #8939003 - Flags: review?(cam) → review?(jwatt)
Attachment #8939004 - Flags: review?(cam) → review?(jwatt)
Attachment #8939005 - Flags: review?(cam) → review?(jwatt)
Comment on attachment 8939003 [details] [diff] [review]
patch 1 - Support GLYPH_STROKE_UNDERNEATH in the gfxFont painting code

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

::: gfx/thebes/gfxFont.cpp
@@ +1677,5 @@
>          buf.mNumGlyphs = mNumGlyphs;
>  
>          const gfxContext::AzureState &state = mRunParams.context->CurrentState();
> +
> +        auto doStroke = [=,&buf]() {

Please don't use the default capture modes when using lambdas. They offer too many opportunities for footgunnery. Besides that it's much better to be clear about what's being captured.

In this case the by-value default capture mode is causing the AzureState object 'state' to be captured by value. That's a non-trivial object, so we don't really want to be copying that.

You're also relying on by-value capture of the address stored in 'this'. Since the lambda isn't stored that's less scary than it might be, but it's one of the footguns to watch out for so I'd personally rather we never do that.

In fact, rather than unnecessarily using capture, I think it would be cleaner to pass the three objects you need as arguments.
Attachment #8939003 - Flags: review?(jwatt) → review-
Comment on attachment 8939004 [details] [diff] [review]
patch 2 - Support the paint-order property for HTML text (in addition to SVG)

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

::: modules/libpref/init/all.js
@@ +2979,5 @@
>  #ifndef RELEASE_OR_BETA
>  pref("layout.css.emulate-moz-box-with-flex", false);
>  #endif
>  
> +// Is the paint-order property supported for HTML text (in addition to SVG)?

It might be unclear to readers whether the parenthetical means "this also applies to SVG". Maybe may it explicit. Something like:

// Enables the 'paint-order' property for HTML text (it's always enabled
// for SVG).
Attachment #8939004 - Flags: review?(jwatt) → review+
Updated, now with reduced footgunnery. :) Actually, I think the simplest approach here is to just have a separate (private) DrawStroke method, rather than writing it as a lambda. In particular, this means it automatically has access to /this/, without needing to capture it (implicitly or explicitly) or pass it as a GlyphBufferAzure argument in addition to the gfx::GlyphBuffer one (which gets confusing).
Attachment #8946596 - Flags: review?(jwatt)
Attachment #8939003 - Attachment is obsolete: true
Attachment #8946596 - Flags: review?(jwatt) → review+
Attachment #8939005 - Flags: review?(jwatt) → review+
Flags: needinfo?(cam)
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d64eb03f3da5
patch 1 - Support GLYPH_STROKE_UNDERNEATH in the gfxFont painting code. r=jwatt
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fbcfb275c82
patch 2 - Support the paint-order property for HTML text (in addition to SVG); currently preffed-off by default. r=jwatt
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1ad4de3ed91
patch 3 - Add a tentative web-platform reftest for paint-order applied to HTML text. r=jwatt
Really cool!

Related: Does or will this support multiple strokes?
Multiple strokes would require extending the CSS properties, as well as additional support at the painting level. So no, not at this time. If/when that gets implemented it will be as a separate bug. AFAICT, the CSS Fill & Stroke Module is quite some way from being implemented at this point. So for the time being, rendering with multiple strokes still requires alternative layering approaches.
Blocks: 1435684
I've documented this.

New property page added:
https://developer.mozilla.org/en-US/docs/Web/CSS/paint-order

Entry added in our experimental features page:
https://developer.mozilla.org/en-US/Firefox/Experimental_features#CSS

PRs made to relevant repos to add in compat data and formal syntax:
https://github.com/mdn/browser-compat-data/pull/1174
https://github.com/mdn/data/pull/181

Let me know if that looks OK. Thanks!
Flags: needinfo?(jfkthame)
Thanks, Chris!

One typo caught my eye: under Values, it lists "stoke" instead of "stroke".

The other thing I wonder is whether it's worth adding some kind of note that the "markers" part of paint-order is not relevant to HTML text (it's only applicable when drawing SVG shapes when the marker-* properties and <marker> element are used, AFAIK).

So in HTML text, the only relevant aspect of paint-order is the relative order of "stroke" and "fill".
Flags: needinfo?(jfkthame)
Oh, and as of bug 1435684, this is now enabled by default. (So the experimental features page is out-of-date.)
I added the below text to https://github.com/Fyrd/caniuse/issues/3394 :

"Firefox Nightly now supports paint-order for HTML text:
https://developer.mozilla.org/en-US/docs/Web/CSS/paint-order "
(In reply to Jonathan Kew (:jfkthame) from comment #17)
> Thanks, Chris!
> 
> One typo caught my eye: under Values, it lists "stoke" instead of "stroke".

Oops! Fixed.

> 
> The other thing I wonder is whether it's worth adding some kind of note that
> the "markers" part of paint-order is not relevant to HTML text (it's only
> applicable when drawing SVG shapes when the marker-* properties and <marker>
> element are used, AFAIK).
> 
> So in HTML text, the only relevant aspect of paint-order is the relative
> order of "stroke" and "fill".

This is a good point — I've added a note to cover this.

One more question — I've seen that this is supposed to be enabled by default (https://bugzilla.mozilla.org/show_bug.cgi?id=1435684), but when I tested it in the latest nightly the pref still seemed to be set to false, hence me documenting it as such. What is the deal here?

Thanks again.
Flags: needinfo?(jfkthame)
For me it works in Nightly without me having to set a pref:
https://twitter.com/TobiReif/status/966711161390497792
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #20)
> One more question — I've seen that this is supposed to be enabled by default
> (https://bugzilla.mozilla.org/show_bug.cgi?id=1435684), but when I tested it
> in the latest nightly the pref still seemed to be set to false, hence me
> documenting it as such. What is the deal here?

That seems odd... it's set to true by default in my Nightly here. What build ID are you on?
Flags: needinfo?(jfkthame)
Filed tickets for some other browsers:
https://twitter.com/TobiReif/status/966988875519414273
(In reply to Jonathan Kew (:jfkthame) from comment #22)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #20)
> > One more question — I've seen that this is supposed to be enabled by default
> > (https://bugzilla.mozilla.org/show_bug.cgi?id=1435684), but when I tested it
> > in the latest nightly the pref still seemed to be set to false, hence me
> > documenting it as such. What is the deal here?
> 
> That seems odd... it's set to true by default in my Nightly here. What build
> ID are you on?

Hrm, I am wondering if perhaps I had forgotten to update Nightly for a few days? Since you reporting it as on by default, and so is Tobi, I will update the docs to show it as enabled.
You need to log in before you can comment on or make changes to this bug.