Closed Bug 1269971 Opened 4 years ago Closed 4 years ago

Implement background-clip:text using a mask

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox48 + fixed
firefox49 --- fixed

People

(Reporter: u459114, Assigned: u459114)

References

Details

Attachments

(16 files, 1 obsolete file)

58 bytes, text/x-review-board-request
jfkthame
: review+
Details
58 bytes, text/x-review-board-request
jfkthame
: review+
mtseng
: review+
Details
58 bytes, text/x-review-board-request
jfkthame
: review+
Details
58 bytes, text/x-review-board-request
jfkthame
: review+
Details
241.44 KB, image/png
Details
178.76 KB, image/png
Details
58 bytes, text/x-review-board-request
jfkthame
: review+
Details
58 bytes, text/x-review-board-request
jfkthame
: review+
Details
58 bytes, text/x-review-board-request
jfkthame
: review+
Details
6.25 KB, patch
Details | Diff | Splinter Review
8.35 KB, patch
Details | Diff | Splinter Review
16.60 KB, patch
Details | Diff | Splinter Review
1.23 KB, patch
Details | Diff | Splinter Review
1.49 KB, patch
Details | Diff | Splinter Review
8.21 KB, patch
Details | Diff | Splinter Review
4.35 KB, patch
Details | Diff | Splinter Review
We can not generate path of stroke if the drawing backend is Cairo. Alternatively, using mask can fix this problem.
Still need to handle text selection.
Attachment #8748642 - Attachment is obsolete: true
Attachment #8748575 - Flags: review?(jfkthame)
Attachment #8748784 - Flags: review?(jfkthame)
Attachment #8748785 - Flags: review?(jfkthame)
Attachment #8748819 - Flags: review?(jfkthame)
Hi Jonathan,
The patches here change everything from using glyph path to mask.
The reason of doing this is to integrate text stroke with bg-clip:text.
From bug 1267128, we know that we can't get stoke path on cario-backend, so I decide to switch to mask solution:
1. Using mask on all platforms, event if we can use glyph path solution on mac, to have the same behavior on different platforms.
2. Before we switch to skia on all platforms, we can keep using mask.

That's what I thought.
Blocks: 1264905
Summary: Implement background-clip:text by mask → Implement background-clip:text using a mask
I tried a local build with these patches, and it looks like something may not be quite right here.... the rendering of the testcase from https://bug1267697.bmoattachments.org/attachment.cgi?id=8745399 shows that the stroke masks seem to be inconsistent or offset or something. See the top image in the attached screenshot.

In contrast, the stroke-path clip rendering from the patch in bug 1267128 looks much more consistent; see bottom image in the screenshot.
(In reply to Jonathan Kew (:jfkthame) from comment #17)
> Created attachment 8749169 [details]
> rendering of testcase with the patches here (top) vs bug 1267128 (bottom)
> 
> I tried a local build with these patches, and it looks like something may
> not be quite right here.... the rendering of the testcase from
> https://bug1267697.bmoattachments.org/attachment.cgi?id=8745399 shows that
> the stroke masks seem to be inconsistent or offset or something. See the top
> image in the attached screenshot.
> 
> In contrast, the stroke-path clip rendering from the patch in bug 1267128
> looks much more consistent; see bottom image in the screenshot.
Yes, it looks weird. By removing rotation effect
100% {
    opacity: 1;
    transform: rotate(0deg) scale(1);
}
Rendering result looks good. Both rotate and skew make border width not consistent. Checking
More update.

<p> itself looks good. The stroke of p:after and p:before pseudo element shift. 
So if you remove .pullquote:before and .pullquote:after, rendering result is correct.
I bet it's relative to maskTransform in GenerateAndPushTextMask. It's too late to calculate any matrix, will check it tomorrow
After discuss with Morris, rewrite matrix evaluation:

maskCtx->SetMatrix(gfxMatrix::Translation(bounds.TopLeft()) *     << bounds should be pre-translation.
                   currentMatrix *                                << source matrix.
                   gfxMatrix::Translation(-drawRect.TopLeft()));  << drawRect should be post-translation.
Status: NEW → ASSIGNED
This seems to be working well; the result with transforms etc looks much better now.

The one thing that is still a bit problematic is antialiasing. Looking at the test failures in https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f5e02b2f95b (is this the latest try run?), there seems to be a major discrepancy between the antialiasing of the reference file and the testcase using mask. In particular, on both Linux and Windows (Ru), the testcase seems to be lacking proper AA, so that the diagonals and curves appear very jagged -- compare the "c" and "p" glyphs, and the dot of the "i", between testcase and reference on any of the Linux jobs.

On OS X, the testcase seems to render consistently "fatter" glyphs than the reference. This looks to me like the difference between OS X's grayscale (lighter) and subpixel (heavier) antialiasing, even though the background rendering using the mask does not actually yield true subpixel results. As an experiment, I tried adding "-moz-osx-font-smoothing:grayscale" to a testcase, and in my local testing, this does produce a much closer match (not quite pixel-perfect, but within the range of reasonable "fuzz" -- visually, they look the same weight) between test and reference.

And on Windows GDI (Ru tests) it looks like not only antialiasing is different, but even the glyph shapes themselves, suggesting that a different hinting mode may be in effect.

I think it's OK if we don't get a pixel-perfect match between test and reference cases here, given the complexity of the antialiasing and compositing that's involved. But I think we should try to ensure that the hinting and antialiasing is consistent at least in theory, even if there's some fuzz in the final outcome.

The fact that adding -moz-osx-font-smoothing:grayscale affects the testcase on OS X, and makes it visually match the reference, seems to confirm that improvement is possible here. (FWIW, I expect grayscale AA is indeed a more correct mode than subpixel for the glyph rendering used in generating the mask -- and indeed, given that we may eventually be painting a multi-colored image through the mask, it's really not possible to achieve a subpixel result.) However, as an experiment, I tried simply calling maskDT->SetPermitSubpixelAA(false) after the maskDT is created, and this did _NOT_ seem to have any effect on the result.

Unfortunately, I don't really have a good handle on how all this is controlled in the new gfx world, so it might be helpful to discuss with others such as Bas or Jeff to understand more of how the options are managed and propagated through the code.
At my local side, no AA problem on both mac(flatter like you said) and linux. I think it should be relative AA option while mask blending. Will check it tomorrow
Attachment #8748784 - Flags: review?(mtseng)
Comment on attachment 8748784 [details]
MozReview Request: Bug 1269971 - Part 2. From ClipBackgroundByText to GenerateAndPushTextMask;

https://reviewboard.mozilla.org/r/50531/#review48309

Looks good for transform part.
Attachment #8748784 - Flags: review?(mtseng) → review+
Hi Jonathan,
I applied these patch on linux/mac/windows. Rendering results are all good on these platforms. I didn't see any AA problem. So I think the solution here should be correct.

To figure out what happen on try:linux and try:windows, I submitted a  test try [1]
Please have a look on reftest-analyzer [2]

You can see two "Text Clip" strings in "background-clip-text-1a.html"
1. The first sting is *simple* a text element with green color.
2. The second string is bg-clip:text.
Both of them have AA problem. It reveal a fact that it's a common text rendering problem on try:linux and is not caused by the implementation of bg-clip:text.

Conclusion:
My solution is replace SVG text in background-clip-text-1-ref.html by normal HTML text. And here is the test result [3]

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ced90c18023&selectedJob=20598298
[2] http://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/CCgw41xfQAu_ggYLKf_d9A/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1
[3] https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ba9b7c9a6f3
Attachment #8750631 - Flags: review?(jfkthame)
(In reply to C.J. Ku[:cjku] from comment #39)
> You can see two "Text Clip" strings in "background-clip-text-1a.html"
> 1. The first sting is *simple* a text element with green color.
> 2. The second string is bg-clip:text.
> Both of them have AA problem. It reveal a fact that it's a common text
> rendering problem on try:linux and is not caused by the implementation of
> bg-clip:text.

OK, thanks for looking into it. Let's not worry about that here, then.

> Conclusion:
> My solution is replace SVG text in background-clip-text-1-ref.html by normal
> HTML text. And here is the test result [3]

> [3] https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ba9b7c9a6f3

Looking at the Windows and OS X results there, what I think we're seeing is that the background-clip-text mask is being generated from subpixel-antialiased text, even though the mask is only a single alpha channel, not actually per-subpixel alpha.

This is why, in the OS X results, the background-clip text looks significantly fatter than the plain HTML text in the reference. The latter is using grayscale-only antialiasing, which is based on the true glyph outlines; but when OS X does subpixel AA, it applies a slight "fattening" to the outlines prior to rasterization. That "fattening" is why the background-clip text looks different from the HTML version.

And in the Windows (Ru) results, the HTML text shows subpixel AA (seen as the yellow and cyan "fringes" of the glyph left and right edges, if you zoom in and look closely), while the background-clip rendering looks like it is exactly the same glyph shapes, but with the subpixel AA "collapsed" to a single alpha channel that just results in different levels of green instead of subpixel-specific coloration.

For the OS X case, I suspect we can solve (or at least greatly reduce) the mismatch in the reftest by adding -moz-osx-font-smoothing:grayscale, so that neither the testcase nor the reference tries to use subpixel-antialiased glyphs and therefore gets the OS X "fattening". I've pushed a try job[1] with this added (and with fuzzy annotations removed) so we can see what results that gives.

As for Windows, we don't have an equivalent of -moz-osx-font-smoothing:grayscale, so we may need to just live with the fuzzy annotation for now. I don't think it's ideal that we're using subpixel-AA glyph rasterization to generate the mask for background-clip:text -- in theory, grayscale would be the more correct one to use -- but I don't want to hold up this bug for that detail. It may not be important enough to ever fix, and if it is, we can do it as a followup.

I'll try to finish reading through the patches shortly, so we can get all this landed. Thanks for the work!

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=22f7d155d308
Comment on attachment 8748575 [details]
MozReview Request: Bug 1269971 - Part 1. Rename IsForGenerateGlyphPath to IsForGenerateGlyphMask;

https://reviewboard.mozilla.org/r/50401/#review48411

r=me (please fix the repetition of "IsForGen" in the commit message!)
Attachment #8748575 - Flags: review?(jfkthame) → review+
Comment on attachment 8748784 [details]
MozReview Request: Bug 1269971 - Part 2. From ClipBackgroundByText to GenerateAndPushTextMask;

https://reviewboard.mozilla.org/r/50531/#review48413
Attachment #8748784 - Flags: review?(jfkthame) → review+
Comment on attachment 8748785 [details]
MozReview Request: Bug 1269971 - Part 3. Handle selection text color and selection backgrond painting;

https://reviewboard.mozilla.org/r/50533/#review48419

::: layout/base/nsDisplayList.cpp:516
(Diff revision 7)
> +    // Paint text selection background into sourceCtx.
> +    gfxContextMatrixAutoSaveRestore save(sourceCtx);
> +    sourceCtx->SetMatrix(sourceCtx->CurrentMatrix().Translate(bounds.TopLeft()));
> +
> +    nsLayoutUtils::PaintFrame(aContext, aFrame,
> +                              nsRect(nsPoint(0, 0), aFrame->GetSize()),
> +                              NS_RGB(255, 255, 255),
> +                              nsDisplayListBuilderMode::GENERATE_GLYPH);

This seems a bit mysterious... the comment says "paint text selection background", but then we call PaintFrame with no indication that we're asking it to use a special selection-background-only mode.

I realize that with the nsTextFrame changes below, that's what GENERATE_GLYPH will end up doing, but that's far from obvious when reading the code here.

If we're no longer generating glyph paths for clipping, does GENERATE_GLYPH still mean what it says, or could we replace it with a mode like PAINT_SELECTION_BACKGROUND that would make it clear at this callsite that we're doing exactly what the comment claims?

::: layout/generic/nsTextFrame.h:448
(Diff revision 7)
>    // Return false if the text was not painted and we should continue with
>    // the fast path.
>    bool PaintTextWithSelection(const PaintTextSelectionParams& aParams,
> -                              const nsCharClipDisplayItem::ClipEdges& aClipEdges);
> +                              const nsCharClipDisplayItem::ClipEdges& aClipEdges,
> +                              bool aGenerateTextMask,
> +                              bool aPaintSelecitonBGOnly);

nit: there's a persistent typo here and throughout much of the patch (copy&paste!)...

    s/Seleciton/Selection/g;

please.
Attachment #8748785 - Flags: review?(jfkthame)
Comment on attachment 8748819 [details]
MozReview Request: Bug 1269971 - Part 4. Add text stroke into text mask;

https://reviewboard.mozilla.org/r/50543/#review48429

s/stoke/stroke/ in commit message
Attachment #8748819 - Flags: review?(jfkthame) → review+
Attachment #8749584 - Flags: review?(jfkthame) → review+
Comment on attachment 8749584 [details]
MozReview Request: Bug 1269971 - Part 5. Correct draw region in nsDisplayBackgroundColor::Paint;

https://reviewboard.mozilla.org/r/51047/#review48431
https://reviewboard.mozilla.org/r/50533/#review48419

> This seems a bit mysterious... the comment says "paint text selection background", but then we call PaintFrame with no indication that we're asking it to use a special selection-background-only mode.
> 
> I realize that with the nsTextFrame changes below, that's what GENERATE_GLYPH will end up doing, but that's far from obvious when reading the code here.
> 
> If we're no longer generating glyph paths for clipping, does GENERATE_GLYPH still mean what it says, or could we replace it with a mode like PAINT_SELECTION_BACKGROUND that would make it clear at this callsite that we're doing exactly what the comment claims?

agree. doing right now
Attachment #8748785 - Flags: review?(jfkthame) → review+
Comment on attachment 8748785 [details]
MozReview Request: Bug 1269971 - Part 3. Handle selection text color and selection backgrond painting;

https://reviewboard.mozilla.org/r/50533/#review48923

::: layout/generic/nsTextFrame.cpp:5997
(Diff revision 8)
>    gfxFloat iOffset, hyphenWidth;
>    Range range; // in transformed string
>    SelectionType type;
>    TextRangeStyle rangeStyle;
>    // Draw background colors
> -  if (anyBackgrounds) {
> +  if (anyBackgrounds && (!aParams.generateTextMask||

nit: need a space before ||
Comment on attachment 8749585 [details]
MozReview Request: Bug 1269971 - Part 6. Modify reftest according to the way we generate bg-clip:text;

https://reviewboard.mozilla.org/r/51049/#review48931

According to https://treeherder.mozilla.org/#/jobs?repo=try&revision=22f7d155d308, adding -moz-osx-font-smoothing:grayscale will eliminate the fuzz on OS X caused by the subpixel-AA fringes, so let's do that. Then we'll have just Windows in the fuzz annotations, with the numbers at least slightly reduced.
Attachment #8749585 - Flags: review?(jfkthame)
Comment on attachment 8750631 [details]
MozReview Request: Bug 1269971 - Part 7. Clean out unused things created in bug 759568;

https://reviewboard.mozilla.org/r/51561/#review48933
Attachment #8750631 - Flags: review?(jfkthame) → review+
(In reply to C.J. Ku[:cjku] from comment #72)
> Try result
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b7204ec262e
background-clip-text-1e.html  failure is because -moz-osx-font-smoothing:grayscale missed in that file. So, wait for testing result of windows before patches re-update
(In reply to Jonathan Kew (:jfkthame) from comment #36)
> The fact that adding -moz-osx-font-smoothing:grayscale affects the testcase
> on OS X, and makes it visually match the reference, seems to confirm that
> improvement is possible here. (FWIW, I expect grayscale AA is indeed a more
> correct mode than subpixel for the glyph rendering used in generating the
> mask -- and indeed, given that we may eventually be painting a multi-colored
> image through the mask, it's really not possible to achieve a subpixel
> result.) However, as an experiment, I tried simply calling
> maskDT->SetPermitSubpixelAA(false) after the maskDT is created, and this did
> _NOT_ seem to have any effect on the result.
Bug 1272318 filed
Comment on attachment 8748575 [details]
MozReview Request: Bug 1269971 - Part 1. Rename IsForGenerateGlyphPath to IsForGenerateGlyphMask;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50401/diff/9-10/
Attachment #8749585 - Flags: review?(jfkthame)
Comment on attachment 8749585 [details]
MozReview Request: Bug 1269971 - Part 6. Modify reftest according to the way we generate bg-clip:text;

https://reviewboard.mozilla.org/r/51049/#review49151

Looks good; r=me, with the fix to background-clip-text-1e.html.
Attachment #8749585 - Flags: review?(jfkthame) → review+
[Tracking Requested - why for this release]:
We need these patches to enable background-clip:text in FF 48 release version, so that we can turn on webkit prefix by default.
Duplicate of this bug: 1267128
CJ & jfkthame: do you have any leanings on how feasible it would be to uplift this to Aurora? (so that we could uplift bug 1264905 & bug 1259345, so those features can ship with Firefox 48)

(My default assumption is "best not to uplift this", since this is a multi-part reimplementation of a feature, and that seems like the sort of change that has potential for regressions and complexity, and that makes for a bad uplift-candidate.  But, since I wasn't involved with the code here, I want to sanity-check this assumption with you guys.)
In fact, flow control does not change much. Using glyph-path, nsTextFrame::Paint generates path and puts path into gfxContext; using mask, nsTextFrame::Paint paints text, with opaque color, into gfxContext. From implementation level, I think the later one is simpler. It's ok to put mask-version in 48, and pref-off while any serious bug found. I would like to hear jfkthame's opinion as well.
I'm inclined to think we could uplift this (and then also bug 1264905 & bug 1259345) at this point, with half of the Aurora cycle left. As CJ says, if we do find serious problems, we can still revert the pref changes and hold it all back, but the reimplementation here does not seem too scary to me.

Although the code here looks like a lot of patches, they are for the most part very simple and safe. The really significant code changes are largely confined to patches 2 and 3; and even these changes seem unlikely to carry much risk for content that doesn't try to use the new features.

So given that (AIUI) we're keen to try and ship the -webkit-prefix stuff soon, I think uplift here would be reasonable. That reduces the time this code spends in testing on nightly & aurora, but in practice I doubt that will make much difference; if we're going to find issues with it, they're more likely to show up once it hits beta and gets substantially wider exposure.

CJ, would you like to request aurora approval for these patches?
Yes, I can. But I found another shipping block - Bug 1273068
That bug is not relative to patches here. We already have that since bug 759568. I think I should fix that one before uplift.
CJ, could you request uplift for this to Aurora 48 (with the understanding that it'll be disabled in the final release of Firefox 48, during the beta period).  I just talked to Jet, and we'd like to ship with webkit prefixing enabled for 2-3 week trial during the Beta 48 cycle, to get some broader early feedback, since we think the feature is pretty much ready at this point.  The sooner we find out what (if anything) we're missing, the better.

(And in order for that experiment to be effectively testing the code that will actually be shipping, we'll need to uplift the backgorund-clip:text reimplementation in this bug.)

I suspect the remaining background-clip:text bugs mentioned in comment 90 are unlikely to affect any actual content on the web (or make any content change from working-great to broken), so I think we're OK to backport *just* the fixes on this bug here, and not worry about comment 90's bugs for the beta trial.
Flags: needinfo?(cku)
no problem. I will do it. Rebasing and testing. keep needinfo
[Tracking Requested - why for this release]:
dholbert explained why we want to uplift these patches in comment 93. In 48, although this feature will be disable in release versions, we still want to ship with webkit prefixing(with background-clip:text) enabled for 2-3 week trial during the Beta 48.
ok, let wait and see try result. Time to sleep.
Comment on attachment 8754956 [details] [diff] [review]
(aurora) 0001-Bug-1269971-Part-1.-Rename-IsForGenerateGlyphPath-to.patch

Approval Request Comment
[Feature/regressing bug #]: Bug 759568
[User impact if declined]: Keep the implementation in 48 align with 49 so that we can get valid feedback from our users.
[Describe test coverage new/current, TreeHerder]: manual test + try
[Risks and why]: low risk. We have a preference key to turn off this feature totally.
[String/UUID change made/needed]: none

Please uplift patches from (aurora)-0001 to (aurora)-0007
Attachment #8754956 - Flags: approval-mozilla-aurora?
Comment on attachment 8754956 [details] [diff] [review]
(aurora) 0001-Bug-1269971-Part-1.-Rename-IsForGenerateGlyphPath-to.patch

OK, seems that there is a consensus to uplift that into 48, taking it to help you guys.
However, if any regressions come up, I would prefer a quick backout.
Taking it in aurora.
Attachment #8754956 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1287705
Depends on: 1465305
You need to log in before you can comment on or make changes to this bug.