1426092 - wide text-strokes may be cut off at the bottom and right edges

Status: VERIFIED FIXED

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

Description

[Tobi Reif]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171128222554

Steps to reproduce:

Open https://tobireif.com/non_site_stuff/text-stroke_cut_off_example_for_bug-reports/grid.html .


Actual results:

The text strokes are cut off.


Expected results:

The text strokes should not be cut off, as in Chrome, Edge, and Safari.

Comment 1

[Tobi Reif]

I hope that the rendering will match that of Chrome etc soon.

In the meantime / for now I'd be very thankful for a quick workaround for Firefox.
The real URL is https://tobireif.com/demos/grid/ (work in progress as of 2017-12-19).

Comment 2

[Tobi Reif]

Also, on https://tobireif.com/demos/grid/ , the distance between the h1 and the next element is much larger than in other browsers.
(that is the current state)

Comment 3

[Tobi Reif]

Also cut off in Canary.

I hope this can get fixed.

Comment 4

[Tobi Reif]

Attachment: screenshot of chrome vs firefox

Comment 5

[Tobi Reif]

Attached a screenshot of Chrome vs Firefox.

Looks fine in Chrome, is cut off in Firefox.

(Also, in Firefox the space below the h1 heading is much larger.)

Comment 6

[Tobi Reif]

No issues in Edge either (as in Chrome):

https://app.crossbrowsertesting.com/public/ib8393c20a124bac/livetests/10015909/snapshots/z6ddcfd3771cd91aeb4a

Comment 7

[Tobi Reif]

Basically the same issues in Firefox on Windows vs on Mac.

Comment 8

[Jonathan Kew [:jfkthame]]

This is simply a bug in the original implementation of text-stroke rendering in bug 1248708: when we enlarge the visual-overflow area to account for the thickness of the stroke, we need to add stroke-width * 1/2 on each side, but instead the code currently adds the whole stroke-width to the left and top edges. Fixing that will resolve the rendering problem here.

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: Reftest for incorrect clipping of thick text-stroke

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: Fix the inflation of the text bounding rect to account for stroke-width correctly

Comment 11

[Jonathan Kew [:jfkthame]]

Attachment: Fix the inflation of the text bounding rect to account for stroke-width correctly

Once more, now with the reftest manifest updated.

Comment 12

[Tobi Reif]

Thanks for working on this! I hope that the fix will be in stable Firefox (on the machines of the users) soon :)

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8938318 [details] [diff] [review]
Fix the inflation of the text bounding rect to account for stroke-width correctly

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

::: layout/generic/nsTextFrame.cpp
@@ +5789,5 @@
> +  // text-stroke overflows: add half of text-stroke-width on all sides
> +  nscoord textStrokeWidthInflation = StyleText()->mWebkitTextStrokeWidth / 2;
> +  if (textStrokeWidthInflation > 0) {
> +    // add an extra pixel to allow for antialiasing, rounding errors, etc
> +    textStrokeWidthInflation += appUnitsPerDevUnit;

Although the result probably isn't much different, wouldn't it make more sense to check the stroke width first, then calculate the inflation?

Maybe something like the following?
> nscoord textStrokeWidth = StyleText()->mWebkitTextStrokeWidth;
> if (textStrokeWidth > 0) {
>   nscoord inflation = textStrokeWidth / 2 + appUnitsPerDevUnit;
>   // ...
> }

Comment 14

[Tobi Reif]

Would this also fix the distance issue?

When I open https://tobireif.com/demos/grid/ in Chrome, it renders as intended (also OK in Safari), but in Firefox, (currently) the distance between "CSS Grid" and "Why use CSS Grid?" is (way) too large.

Comment 15

[Jonathan Kew [:jfkthame]]

(In reply to Tobi Reif from comment #14)
> Would this also fix the distance issue?
> 
> When I open https://tobireif.com/demos/grid/ in Chrome, it renders as
> intended (also OK in Safari), but in Firefox, (currently) the distance
> between "CSS Grid" and "Why use CSS Grid?" is (way) too large.

I don't expect this to have any effect on the spacing, which looks like it may have something to do with line-height computations with the font being used. It would be best to file a separate bug about that, ideally with a simplified testcase that shows a similar issue.

Comment 16

[Tobi Reif]

The thing is this: When I open https://tobireif.com/demos/grid/ in Firefox, and (using the dev tools) set the h1 :before and :after "content" property to "none", the strokes are gone (as expected) - and the oversized-spacing issue is gone as well (at least the space gets reduce by a lot).

That's why I thought that the two issues might be related - that the spacing issue might disappear when the stroke-clipping issue got fixed.

In your dev build, when you apply the fix for the clipped/cut strokes, does the spacing issue disappear as well? (eg compare the rendering to the rendering of Chrome, where both aspects are OK.)
If not (if the space issue remains despite the applied stroke fix), please let me know and I'll file an additional bug for the spacing issue.

Comment 17

[Jonathan Kew [:jfkthame]]

(In reply to Tobi Reif from comment #16)
> In your dev build, when you apply the fix for the clipped/cut strokes, does
> the spacing issue disappear as well?

No, it doesn't have any effect on the spacing.

Also note that just removing the strokes from the :before and :after pseudo-elements doesn't have any effect on the spacing.

It appears to be the use of position:absolute that leads to the issue, in combination with the unusually small line-height (for the size of font) that is used. I don't fully understand why, at the moment, but it looks like a bug, and is definitely unrelated to the clipping.

Comment 18

[Tobi Reif]

Created a separate bug report for the spacing issue:
https://bugzilla.mozilla.org/show_bug.cgi?id=1426760

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/433caabd1b47
Reftest for incorrect clipping of thick text-stroke. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d0e3c825e42
Fix the inflation of the text bounding rect to account for stroke-width correctly. r=xidorn

Comment 20

[Tobi Reif]

Great to see that there's progress!

Thanks!

Can I check the fix? eg in Nightly?

Comment 21

[Jonathan Kew [:jfkthame]]

The patch is on its way into the main code repository (currently on "inbound", not yet merged to mozilla-central). When it gets there, the fix will appear in the next version of Nightly that's built. Though given that we're hitting not only a weekend but also the holiday season, things may be slower than normal (e.g. if there aren't people around to do the inbound->central merge as often as usual).

So I expect it'll be in Nightly within a couple of days, but can't predict exactly when.

Comment 22

[Tobi Reif]

Thanks for the info! I'll try it in Nightly starting Dec 27.

Your fix (and also bug 1426760) is crucial for https://tobireif.com/demos/grid/ (currently status work in progress), so I sincerely (and patiently :) ) hope that both issues will be fixed in users' browsers / Firefox Stable soon :)

Thanks so much for all your efforts!

Comment 23

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/433caabd1b47
https://hg.mozilla.org/mozilla-central/rev/4d0e3c825e42

Comment 24

[Tobi Reif]

Just tested it in Nightly - it works perfectly!

Thank you!

Comment 25

[Oana Botisan, Desktop QA]

I managed to reproduce the bug on an older version of Nightly (2017-11-19) on Windows 10 using the steps from comment 0.
I retested everything on latest Nightly 60.0a1 and beta 59.0b6 on Windows 10 x64, Ubuntu 16.04 x64 and macOS 10.13 and the bug is not reproducing anymore.