Closed Bug 1426092 Opened 2 years ago Closed 2 years ago

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

Categories

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

57 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla59
Tracking Status
firefox59 --- verified
firefox60 --- verified

People

(Reporter: tobi, Assigned: jfkthame)

References

Details

Attachments

(3 files, 1 obsolete file)

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.
Component: Untriaged → Layout: Text
Product: Firefox → Core
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).
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)
Also cut off in Canary.

I hope this can get fixed.
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.)
Basically the same issues in Firefox on Windows vs on Mac.
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.
Assignee: nobody → jfkthame
Blocks: 1248708
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Summary: text-strokes cut off → wide text-strokes may be cut off at the bottom and right edges
Once more, now with the reftest manifest updated.
Attachment #8938318 - Flags: review?(xidorn+moz)
Attachment #8938312 - Attachment is obsolete: true
Attachment #8938312 - Flags: review?(xidorn+moz)
Thanks for working on this! I hope that the fix will be in stable Firefox (on the machines of the users) soon :)
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;
>   // ...
> }
Attachment #8938318 - Flags: review?(xidorn+moz) → review+
Attachment #8938311 - Flags: review?(xidorn+moz) → review+
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.
(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.
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.
(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.
Created a separate bug report for the spacing issue:
https://bugzilla.mozilla.org/show_bug.cgi?id=1426760
Pushed by jkew@mozilla.com:
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
Great to see that there's progress!

Thanks!

Can I check the fix? eg in Nightly?
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.
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!
https://hg.mozilla.org/mozilla-central/rev/433caabd1b47
https://hg.mozilla.org/mozilla-central/rev/4d0e3c825e42
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Just tested it in Nightly - it works perfectly!

Thank you!
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.