Closed Bug 1488300 Opened 3 years ago Closed 3 years ago
Loading of external resources referenced from CSS like elements from SVG document should block onload
46 bytes, text/x-phabricator-request
|Details | Review|
Currently, reftest snapshot isn't block on external SVG resources like those referenced from filter or mask-image properties. This can cause difficulty of writing tests for these features (for example see bug 1486252 and the backouts), and may have to use hacks. The hacks are considered fine for internal tests, but may cause more impediment for wpt. I had a look at the code, and it seems that the snapshot in web-platform reftest is currently only blocked on onload event, but as far as I know, there are many other stuff that doesn't block onload, like CSS image loading. It's not clear how do we currently workaround that since that should be something fairly common in tests.
Joel, do you have idea how wpt reftest currently handle renderings that don't block onload? Is it possible to put extra blocker somewhere to make external resource dependency more reliably?
So based on discussion on IRC, what we know now is that: 1. Loading of CSS images do block onload 2. Decoding of CSS images doesn't block onload, but it blocks readback / snapshot via sync decoding It is a bit tricky for external resources. Maybe we should make it block onload as well.
Component: web-platform-tests → DOM
Product: Testing → Core
Summary: Have reftest snapshot blocked on loading external resources like remote SVG document → Loading of external resources like elements from SVG document should block onload
Component: DOM → CSS Parsing and Computation
Summary: Loading of external resources like elements from SVG document should block onload → Loading of external resources referenced from CSS like elements from SVG document should block onload
So the problem here is that, external resources loading does block onload, but it doesn't for those from CSS because those may not happen before onload. Currently, loading of external resources for stuff like mask and filter is triggered by painting. For mask image, it is done this path: * nsDisplayMask::BuildLayer invokes SVGObserverUtils::GetEffectProperties, which * invokes GetOrCreateMaskProperty which creates nsSVGMaskProperty, which * holds a list of nsSVGPaintingProperty, which inherits nsSVGIDRenderingObserver, which * holds an instance of IDTracker, and the constructor invokes IDTracker::Reset, which * invokes nsIDocument::RequestExternalResource to kick off loading of the external resource Start loading something until painting sounds like way too late. We should kick off it from style resolving, like, in FinishStyle. However, the problem is that, nsIDocument::RequestExternalResource wants to know what element triggers that request. This is rather complicated because in FinishStyle we don't have information about the element. Actually, our current handling seems to be completely wrong. Tracking down the resource loading code, it seems to be using the provided element for various security-related stuff (getting the principal, etc.), however, via the path mentioned above, we are assuming that the originator of the loading is the element which has the style applied, which feels wrong. The originator should be the element where the style is from, shouldn't it? I don't know how much that matters. Anyway, even follow our current behavior, we should probably trigger the resource loading from post traversal, or find a way to pass element into FinishStyle? It's not totally clear to me what's the right solution here.
There's a bunch of image-related code in nsIFrame::DidSetStyleContext. It'd be nice to use that instead of putting more image code somewhere else I'd think.
Oh, DidSetComputedStyle. That sounds like the right place to put this code in! Thanks.
Comment on attachment 9006147 [details] Bug 1488300 - Kick off external SVG resource loading from DidSetComputedStyle. Daniel Holbert [:dholbert] has approved the revision.
Attachment #9006147 - Flags: review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/68592cefee57 Kick off external SVG resource loading from DidSetComputedStyle. r=dholbert
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12902 for changes under testing/web-platform/tests
Assignee: nobody → xidorn+moz
Status: NEW → ASSIGNED
Can't merge web-platform-tests PR due to failing upstream checks: Github PR https://github.com/web-platform-tests/wpt/pull/12902 * continuous-integration/travis-ci/pr (https://travis-ci.org/web-platform-tests/wpt/builds/425946106?utm_source=github_status&utm_medium=notification)
Err... this is funny. The wpt PR fails because the updated test has unstable result on Firefox Nightly, but this patch itself means to fix such instability. Maybe we can have the CI rerun once this change gets into the nightly.
Upstream PR merged
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #11) > Err... this is funny. The wpt PR fails because the updated test has unstable > result on Firefox Nightly, but this patch itself means to fix such > instability. Did things stabilize? I don't see any annotations suggesting the test fails intermittently.
(In reply to Jonathan Watt [:jwatt] from comment #14) > (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #11) > > Err... this is funny. The wpt PR fails because the updated test has unstable > > result on Firefox Nightly, but this patch itself means to fix such > > instability. > > Did things stabilize? I don't see any annotations suggesting the test fails > intermittently. Yes, it got stablized and the upstream PR has been merged as indicated in comment 13. I trigger the CI task for wpt to run again after this bug got into Nightly.
You need to log in before you can comment on or make changes to this bug.