Closed Bug 1901414 Opened 4 months ago Closed 4 months ago

SVG text does somtimes not show up with an embedded font

Categories

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

Product:
Core
Component:
Layout: Text and Fonts
Version:
Firefox 128
Platform:
x86_64
Windows 10
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
129 Branch
Tracking Flags:
Tracking Status
firefox-esr115 --- unaffected
firefox127 --- wontfix
firefox128 --- verified
firefox129 --- verified

People

(Reporter: kontakt, Assigned: emilio)

References

Details

Attachments

(4 files, 1 obsolete file)

test.svg
286.11 KB, image/svg+xml
Details
reporter's testcase
111 bytes, text/html
Details
Putting this in `<img>` another HTML document causes actual behaviour
285.94 KB, image/svg+xml
Details
Bug 1901414 - Flush layout before firing SVG image doc load. r=dshin
48 bytes, text/x-phabricator-request
RyanVM
: approval-mozilla-beta+
Details | Review
Attached file repro.tar.gz (obsolete) —

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:128.0) Gecko/20100101 Firefox/128.0

Steps to reproduce:

I am trying to use a base64-embedded font in an SVG file, and embed that SVG file in an HTML file using an <img> tag.

The SVG file looks like this (excluding the huge base64 blob):

<svg width="800" height="80"
    xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
    <style>
        @font-face {
            font-family: CascadiaCode;
            src: url("data:font/woff2;base64,[base64 -w0 CascadiaCode.woff2]");
        }
        svg {
            font-family: CascadiaCode;
        }
        @media (prefers-color-scheme: dark) {
            text {
                fill: blue;
            }
        }
        @media (prefers-color-scheme: light) {
            text {
                fill: red;
            }
        }
    </style>
    <text x="50" y="50" font-size="50">Hello, world!</text>
</svg>

and the HTML file looks like this:

<!DOCTYPE html>
<img src="test.svg" alt="A test image">

Both files are also attached to this report.

Actual results:

When serving the HTML file with a HTTP server (python -m http.server) and opening the file in Firefox Nightly 128.0a1 (2024-06-08) (64-bit) on my Windows 10 machine, the "Hello, world!" text does not always show up.
When I force refresh the page (Ctrl+F5) a few times, sometimes the red text appears, and sometimes it doesn't.
In the cases where it doesn't, using the developer tools to switch to "prefers-color-scheme: dark" makes the blue text show up, but switching back yields a blank screen again.

I was unable to reproduce this behavior without an embedded font.

Expected results:

The red "Hello, world!" text should always show up.

OS: Unspecified → Windows 10
Hardware: Unspecified → x86_64

The Bugbug bot thinks this bug should belong to the 'Core::Layout: Text and Fonts' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Component: Untriaged → Layout: Text and Fonts
Product: Firefox → Core

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=7a7e10acc67d83c1b5d36bd0951bd37d1f7904a9&tochange=66d921932ca7cf275d17bf4942aa92a9b9ef9f9d

Good build: The text render as expected.
Bad build : The bug appears at initial load. And each reload (Ctrl+F5) alternates between this bug and normal drawing.

status-firefox129: --- → affected
Keywords: regression
Regressed by: 1310170
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached image test.svg —
Attachment #9406250 - Attachment is obsolete: true

FWIW the patch in bug 1901378 does not fix this.

:dshin, since you are the author of the regressor, bug 1310170, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Flags: needinfo?(dshin)

Set release status flags based on info from the regressing bug 1310170

status-firefox127: --- → affected
status-firefox128: --- → affected
status-firefox-esr115: --- → unaffected

Ok.. Interesting.
It seems like we're rasterizing the SVG for <img> before the font load completes - I see the text run being painted before nsFontFaceLoader::FontLoadComplete triggers. Forcing the size change by 1 px afterwards triggers the re-rendering and makes the text appear.
Removing the line-height portion of finish_restyle removes the issue, so there seems to be some kind of race involved.

Assignee: nobody → dshin
Flags: needinfo?(dshin)

Looking further: Calculating line-height is actually what triggers the request to load the user font. This causes us to take the deferred load path since we're in style thread.
Refreshing seems to hit the cache instead.

That should still block the document load event right? https://searchfox.org/mozilla-central/rev/aa9d148d5be3e7b606448f0b8da6e9f4fa43112f/layout/style/nsFontFaceLoader.cpp#65-71

Or is that somehow too late?

Ultimately nothing gets drawn because we set the flag to not draw anything as the font isn't loaded in yet.

This happens pre-lh/rlh work if the SVG's CSS is adjusted to ask for font metrics in styling, as attached (Confirmed with mozregression --launch 2023-09-01).

(barely tested, and would need a test written, but it seems easy to do
based on the test case on the bug)

(Removing regression keyword as per comment 11)

Keywords: regression
No longer regressed by: 1310170
See Also: → 1310170
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6ef49ef5159c Flush layout before firing SVG image doc load. r=dshin
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/46730 for changes under testing/web-platform/tests
Pushed by emilio@crisal.io: https://hg.mozilla.org/integration/autoland/rev/16c56fce7fe3 Appease WPT lint which somehow didn't fail locally.

Backed out for causing reftest failures related to CustomizableUI.sys.mjs

[task 2024-06-13T16:04:54.792Z] 16:04:54     INFO - REFTEST TEST-START | layout/printing/crashtests/1663722.html
[task 2024-06-13T16:04:54.796Z] 16:04:54     INFO - REFTEST INFO | SET PREFERENCE pref(dom.window.sizeToContent.enabled,true)
[task 2024-06-13T16:04:54.797Z] 16:04:54     INFO - REFTEST TEST-LOAD | file:///D:/task_171829326750694/build/tests/reftest/tests/layout/printing/crashtests/1663722.html | 3081 / 4037 (76%)
[task 2024-06-13T16:04:56.070Z] 16:04:56     INFO - 1718294696068	Marionette	TRACE	Received observer notification browser-delayed-startup-finished
[task 2024-06-13T16:04:56.415Z] 16:04:56     INFO - console.error: CustomizableUI:
[task 2024-06-13T16:04:56.417Z] 16:04:56     INFO -   TypeError: can't access property "localName", aNode is null -- resource:///modules/CustomizableUI.sys.mjs:1724
[task 2024-06-13T16:04:56.800Z] 16:04:56     INFO - REFTEST TEST-PASS | layout/printing/crashtests/1663722.html | (LOAD ONLY)
[task 2024-06-13T16:04:56.801Z] 16:04:56     INFO - REFTEST TEST-END | layout/printing/crashtests/1663722.html
[task 2024-06-13T16:04:56.859Z] 16:04:56     INFO - REFTEST TEST-START | layout/printing/crashtests/1671503.html
Flags: needinfo?(dshin)
Upstream PR was closed without merging
Flags: needinfo?(dshin) → needinfo?(emilio)
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8ce3de807170 Flush layout before firing SVG image doc load. r=dshin
Pushed by emilio@crisal.io: https://hg.mozilla.org/integration/autoland/rev/9f913cbd60ee Forgot to cherry-pick the WPT lint failure, of course.
Assignee: dshin → emilio
Flags: needinfo?(emilio)
Pushed by emilio@crisal.io: https://hg.mozilla.org/integration/autoland/rev/80fd881ad06d Skip 1747514.html in tsan too since it's too slow.
Duplicate of this bug: 1430755
Upstream PR merged by moz-wptsync-bot

The patch landed in nightly and beta is affected.
:emilio, is this bug important enough to require an uplift?

  • If yes, please nominate the patch for beta approval.
  • If no, please set status-firefox128 to wontfix.

For more information, please visit BugBot documentation.

Flags: needinfo?(emilio)

Comment on attachment 9407001 [details]
Bug 1901414 - Flush layout before firing SVG image doc load. r=dshin

Beta/Release Uplift Approval Request

  • User impact if declined: comment 0
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: comment 0
  • List of other uplifts needed: none
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): Small patch, but non-trivial.
  • String changes made/needed: none
  • Is Android affected?: Yes
Flags: needinfo?(emilio)
Attachment #9407001 - Flags: approval-mozilla-beta?
Flags: qe-verify+

Comment on attachment 9407001 [details]
Bug 1901414 - Flush layout before firing SVG image doc load. r=dshin

Approved for 128.0b5.

Attachment #9407001 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
status-firefox128: fix-optional → fixed
Flags: in-testsuite+
QA Whiteboard: [qa-triaged]
Regressions: 1903377

Verified fixed on Nightly 129.0a1 (20240618214224) and Beta 128.0b5 (20240618035431) using the attachment from Comment 4.

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
status-firefox128: fixed → verified
status-firefox129: fixed → verified
Flags: qe-verify+
Duplicate of this bug: 1877126

From the diagnonsis in comments 8 through 12 and the comment in the patch, it sounds like the problem is that the font face load happens after load and the rendering in the img of the svg isn't updated. And the patch here was trying to make sure that font face load happens before load. svg in img should handle invalidates that happen after load, in fact bug 1805599 was specifically to restore that property which was inadvertently regressed. So it seems that the root cause here is that the invalidate from the font face load isn't getting all the way through to the rendering in the top level doc?

Flags: needinfo?(emilio)

The way the font face invalidation and co works is that it marks stuff dirty and schedules a flush. As I understand it, right now SVG images only trigger rendering updates on-demand (as a result of animations and so on), so it's unclear to me how it's supposed to work? In any case font face loads and so on do block the load event, generally.

I was not familiar with that mechanism myself at least. Bug 1877126 is a very similar case where the "delayed" load is an image (because it is part of an <svg:use> subtree, which gets updated from FlushPendingNotifications). So either bug 1805599 regressed again, or it never really worked as intended?

Flags: needinfo?(emilio) → needinfo?(tnikkel)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #32)

The way the font face invalidation and co works is that it marks stuff dirty and schedules a flush. As I understand it, right now SVG images only trigger rendering updates on-demand (as a result of animations and so on), so it's unclear to me how it's supposed to work? In any case font face loads and so on do block the load event, generally.

The VectorImage has a SVGRenderingObserver, when something in the svg document happens that would change the rendering, OnRenderingChange should be called

https://searchfox.org/mozilla-central/rev/e729cdd1537458f9268215c2ff6dddb231c3a6d9/image/VectorImage.cpp#107

In nsIFrame::SchedulePaint we have a InvalidateRenderingObservers call

https://searchfox.org/mozilla-central/rev/e729cdd1537458f9268215c2ff6dddb231c3a6d9/layout/generic/nsIFrame.cpp#7751

that should cause an OnRenderingChange call. For non animated images that should get to VectorImage::SendInvalidationNotifications which should invalidate saved renderings (SurfaceCache::InvalidateImage) and then send out an image notification FLAG_FRAME_COMPLETE

https://searchfox.org/mozilla-central/rev/e729cdd1537458f9268215c2ff6dddb231c3a6d9/image/VectorImage.cpp#523

which should reach any use of the image in a document and cause it to repaint.

I was not familiar with that mechanism myself at least. Bug 1877126 is a very similar case where the "delayed" load is an image (because it is part of an <svg:use> subtree, which gets updated from FlushPendingNotifications). So either bug 1805599 regressed again, or it never really worked as intended?

I think that bug 1805599 did work as intended and hasn't regressed because I spent a good chunk of time writing a reliable test for it that is still in tree. I suspect there is another bug at play.

In the past I think we've had bugs where we didn't call InvalidateRenderingObservers or something else in that path from some code, that would be a good guess as to what might be happening.

Flags: needinfo?(tnikkel)

So what happens is that InvalidateRenderingObservers would be called only after reflow, and there's nothing triggering the reflow right?

How come nothing is triggering reflow? I'm not familiar with the font code.

So font loads up, and it tries to ensure that there will be a layout flush, but then doesn't do so - No reflow seems to happen afterwards. Perhaps because because mDocument->GetDisplayDocument() == nullptr?

Right, it seems the cue for updating the containing documents should (also?) be EnsureLayoutFlush

I don't think svg images use the mDisplayDocument field on Document's, that is for svg filters referencing uris and things like that.

I think the problem is this

https://searchfox.org/mozilla-central/rev/783fac5174123467e97fbb955a4ede7eb6e6f419/layout/base/nsRefreshDriver.cpp#1800

The above will ask for a refresh driver tick, but the timer won't get started. It's not an animated svg, so it doesn't get registered as an animated image, and hence VectorImage::RequestRefresh isn't called, and so the refresh driver for the svg document is not ticked, and the reflow doesn't happen.

I think the proper fix would be something like giving non-animated svg's being used as images one tick when requested. We tell if it's animated using this code https://searchfox.org/mozilla-central/rev/783fac5174123467e97fbb955a4ede7eb6e6f419/image/SVGDocumentWrapper.cpp#101

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: