Do you attach wrong patches? Looks like the patches not related to shape-outside.

Oops. Wrong bug number ...

Given that bug 1366049 was landed to enable stylo on fennec, it seems no longer a need to support gecko implementation here. TY, can we just make this bug invalid now?

I want to use this bug to implement layout part of shape-outside: <image>.

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #6) > I want to use this bug to implement layout part of shape-outside: <image>. I see. Thanks.

Do we have code & tests to enforce that these images aren't allowed cross-origin? (specifically: "use the potentially CORS-enabled fetch method" per https://drafts.csswg.org/css-shapes/#shape-outside-property <img src="..."> works with pretty much any URL (including cross-origin URLs) for legacy reasons -- but the embedding page can't figure anything out about the image beyond its dimensions, so it's pretty harmless. However, "shape-outside" needs to be stricter about cross-origin resources, because it lets the page figure out the actual image contents. (I did a quick search for "cors" and "origin" in the Part 1 patch as well as in testing/web-platform/tests/css/css-shapes/shape-outside/shape-image , and I didn't get any hits, so I'm guessing this might not be tested & might not be implemented right now.)

Comment on attachment 8942865 [details] Bug 1404222 Part 3 - Adjust the #image width in shape-image-001.html. https://reviewboard.mozilla.org/r/213072/#review219052 ::: commit-message-0a35c:3 (Diff revision 1) > +Per spec [1], the shape image's width is the #image's content box > +width (150px), so there's available space for for the two 'X'. I change > +the #image's width 100px so that the two "X" fit in the container. I don't think this patch is actually correct. The test might need fixing, but not in this way. In particular -- with this change, the test passes for me regardless of whether "shape-outside" support is enabled. So I think that means it's wrong / not-what-the-author-intended. Really, I think the test *does* intend for this element to have width:150px here -- and then the test intends for 50px of its width to get cancelled out via "shape-outside". It's trying to achieve the same effect as if we had the following: shape-outside:inset(0px 50px 0px 0px); That ^^ "inset()" tweak makes the test pass for me in Nightly (with the original width restored). So I think you need to investigate what needs to happen with the test and/or code to get that effect via the image here. (If you like: that might mean leaving this test marked as "fails" for the moment & filing a followup for further investigation.)

Comment on attachment 8942866 [details] Bug 1404222 Part 3 - Replace hash '#' with %23 in SVG data URIs, to fix XML parse errors. https://reviewboard.mozilla.org/r/213074/#review219060 ::: commit-message-8a2d6:1 (Diff revision 1) > +Bug 1404222 Part 4 - Replace hash '#' by %023 to fix XML parse errors. Three changes to this first line: s/by/with/ s/023/23/ s/to fix/in SVG data URIs, to fix/ ::: commit-message-8a2d6:3 (Diff revision 1) > +Bug 1404222 Part 4 - Replace hash '#' by %023 to fix XML parse errors. > + > +The hash symbol '#' is reserved by as fragment identifier per bug 1430526 s/by as/as/

Comment on attachment 8942867 [details] Bug 1404222 Part 5 - Adjust tolerance value in shape-outside-radial-gradient-004.html. https://reviewboard.mozilla.org/r/213076/#review219068 ::: commit-message-9d055:6 (Diff revision 1) > +In this test, the diff pixels of <span id='test0'> is 1.01666259765625, so I > +loose the tolerance to 1.5 pixels, which is the same as the tolerance value > +use in shape-outside-radial-gradient-001.html. Could we use 1.1 instead of 1.5 here? (so that we're increasing the tolerance by 10% rather than by 50%) It looks like you chose 1.5 so that this would be consistent with the other test. But I'll bet the other test *actually needs* a tolerance of (near) 1.5, whereas this test clearly does not. We might as well use as strict a tolerance as we can, so as not to inadvertently miss regressions / unexpected behavior-changes etc. (Particularly given that this is a shared 3rd-party test, and our changes will make a difference for other vendors.) So: 1.1 seems better here. Also: do you know who's correct, between us & Chrome, on the rendering of this tescase? The difference is subtle, but it (visually) looks like we have a 1px disagreement on the placement of this 'test0' div, and it seems like it should be possible to analyze the sizes of the shapes involved & determine who is correct. (It's probably a subtle rounding difference, but I imagine the behavior should still be well-defined.) That part is perhaps not a blocker for this bug, but material for a followup?

(In reply to Daniel Holbert [:dholbert] from comment #16) > Do we have code & tests to enforce that these images aren't allowed > cross-origin? Oh, I'm now seeing that the CORS stuff is tracked in bug 1418930, which doesn't have any patches yet. I'm glad that that's tracked -- but I'm a bit uneasy about landing the rest of these patches until we've *also* got the CORS support, though, because I think (?) that means we'll opening our enthusiast users (users who've preffed this on) to possible-attack in the interim. This shape-outside feature *is* preffed off by default, so users wouldn't be vulnerable by default -- but I'm worried that developers might pref it on for testing & leave it that way, without realizing that they're potentially opening themselves up to dangerous cross-origin information-stealing attacks. The pref sounds trivially safe right now ("layout.css.shape-outside.enabled") -- even to someone who knows the spec/feature, since the spec has these security guarantees built-in -- so that's why I'm worried that developers might try turning it on & leaving it on without realizing that it's dangerous. Ideally, we should hold these patches until bug 1418930 is ready. If that's impractical, I think I'd also probably be OK if we had some sort of blocking baked in at the C++ level, so that we'll fail towards safety even in builds where users have toggled the pref. I don't know what that blocking would look like off the top of my head, though -- simplest option would be adding "#ifdef NIGHTLY_BUILD" around the code that triggers the image request, but that'd still leave our Nightly enthusiasts potentially exposed if they've preffed this feature on, so that's still somewhat problematic. Or it could maybe be an "ac_add_options --enable-shape-outside" compile-time opt-in sort of thing, where that build option would control some #ifdef (and we could include that mozconfig line in mozilla-central's treeherder configs, but not on official Nightly builds & not on beta/release trees), perhaps. (In reply to Daniel Holbert [:dholbert] from comment #19) > Comment on attachment 8942867 [details] > Bug 1404222 Part 5 - Adjust tolerance value in > shape-outside-radial-gradient-004.html. > [...] > Also: do you know who's correct, between us & Chrome, on the rendering I did a bit more investigation & I think Chrome is correct on this testcase -- I filed bug 1430932 with more details.

Comment on attachment 8942869 [details] Bug 1404222 Part 5 - Add a crashtest. https://reviewboard.mozilla.org/r/213080/#review219102

Comment on attachment 8942870 [details] Bug 1404222 Part 8 - Update test expectations after implementing shape-outside: <image>. https://reviewboard.mozilla.org/r/213082/#review219104

Comment on attachment 8942865 [details] Bug 1404222 Part 3 - Adjust the #image width in shape-image-001.html. https://reviewboard.mozilla.org/r/213072/#review219052 > I don't think this patch is actually correct. The test might need fixing, but not in this way. In particular -- with this change, the test passes for me regardless of whether "shape-outside" support is enabled. So I think that means it's wrong / not-what-the-author-intended. > > Really, I think the test *does* intend for this element to have width:150px here -- and then the test intends for 50px of its width to get cancelled out via "shape-outside". It's trying to achieve the same effect as if we had the following: > shape-outside:inset(0px 50px 0px 0px); > > That ^^ "inset()" tweak makes the test pass for me in Nightly (with the original width restored). So I think you need to investigate what needs to happen with the test and/or code to get that effect via the image here. > > (If you like: that might mean leaving this test marked as "fails" for the moment & filing a followup for further investigation.) You're right. The test pass even if "shape-outside" support is disabled. I think the original author might want to achieve the effect like `shape-outside:inset(0px 50px 0px 0px)`. However, if I read the spec correctly, there's no way to adjust the shape image's width because the shape image's width is the element's width. I filed bug 1430983 for investigation with more details.

(In reply to Daniel Holbert [:dholbert] from comment #20) > (In reply to Daniel Holbert [:dholbert] from comment #16) > > Do we have code & tests to enforce that these images aren't allowed > > cross-origin? Yeh, I think the test mentioned in Bug 1373743 does the job. > > Oh, I'm now seeing that the CORS stuff is tracked in bug 1418930, which > doesn't have any patches yet. I'm glad that that's tracked -- but I'm a bit > uneasy about landing the rest of these patches until we've *also* got the > CORS support, though, because I think (?) that means we'll opening our > enthusiast users (users who've preffed this on) to possible-attack in the > interim. > > This shape-outside feature *is* preffed off by default, so users wouldn't be > vulnerable by default -- but I'm worried that developers might pref it on > for testing & leave it that way, without realizing that they're potentially > opening themselves up to dangerous cross-origin information-stealing > attacks. The pref sounds trivially safe right now > ("layout.css.shape-outside.enabled") -- even to someone who knows the > spec/feature, since the spec has these security guarantees built-in -- so > that's why I'm worried that developers might try turning it on & leaving it > on without realizing that it's dangerous. > > Ideally, we should hold these patches until bug 1418930 is ready. If that's > impractical, I think I'd also probably be OK if we had some sort of blocking > baked in at the C++ level, so that we'll fail towards safety even in builds > where users have toggled the pref. I don't know what that blocking would > look like off the top of my head, though -- simplest option would be adding > "#ifdef NIGHTLY_BUILD" around the code that triggers the image request, but > that'd still leave our Nightly enthusiasts potentially exposed if they've > preffed this feature on, so that's still somewhat problematic. Or it could > maybe be an "ac_add_options --enable-shape-outside" compile-time opt-in sort > of thing, where that build option would control some #ifdef (and we could > include that mozconfig line in mozilla-central's treeherder configs, but not > on official Nightly builds & not on beta/release trees), perhaps. True, if we land those patches without implementing CORS stuff, it might be some hazard in the interim. I'll start bug 1418930 to see if it's easier to get it implemented than adding some work around.

Comment on attachment 8942863 [details] Bug 1404222 Part 1 - Implement shape-outside: <image>. https://reviewboard.mozilla.org/r/213068/#review219412 Here's a partial review on "part 1", from a first-pass skim over parts of the patch. If possible, it'd probably be nice to have dbaron do the actual review this part instead of me, since he's more familiar with our float manager and its invariants etc. (It looks like he reviewed the most recent nontrivial changes to nsFloatManager.cpp, at least, over in bug 1326407.) Alternately: if he can't, I can do some digging to get up to speed on how all this stuff works, too. ::: layout/generic/nsFloatManager.cpp:1014 (Diff revision 1) > + float aShapeImageThreshold, > + const nsRect& aContentRect, > + WritingMode aWM, > + const nsSize& aContainerSize) > +{ > + const uint8_t threshold = aShapeImageThreshold * 255; Two things: (1) I think this wants to be: NSToIntRound(aShapeImageThreshold * 255)) (2) You should add an assertion here that "aShapeImageThreshold" is >=0.0 and <=1.0, to validate that this assignment makes sense. ::: layout/generic/nsFloatManager.cpp:1084 (Diff revision 1) > + } > + > + if (aWM.IsVerticalRL()) { > + // Because we scan the columns from left to right, we need to reverse > + // the array so that it's sorted (in ascending order) on the block > + // direction . nit: stray space before "." here ::: layout/generic/nsFloatManager.cpp:1173 (Diff revision 1) > + if (IsEmpty()) { > + // No need to create shape info for an empty float. > + return; > + } I think the comment might be misleading here. AFAICT, IsEmpty() doesn't mean "empty float" -- instead, it means "empty float container" (or something like that)... right? (Looks to me like this IsEmpty() is defined as mRect.IsEmpty(), which it looks like is basically equivalent to aContainerSize.IsEmpty(). So we're testing the passed-in aContainerSize here, basically.) ::: layout/generic/nsFloatManager.cpp:1499 (Diff revision 1) > +/* static */ UniquePtr<nsFloatManager::ShapeInfo> > +nsFloatManager::ShapeInfo::CreateImageShape( > + const UniquePtr<nsStyleImage>& aShapeImage, > + float aShapeImageThreshold, > + nsIFrame* const aFrame, > + WritingMode aWM, > + const nsSize& aContainerSize) > +{ > + nsImageRenderer imageRenderer(aFrame, aShapeImage.get(), As a sanity-check (and as documentation about the implicit relationships between the args here), could you add something like: MOZ_ASSERT(aShapeImage == aFrame->StyleDisplay()->mShapeOutside->GetShapeImage(), "aFrame should be the frame that we got aShapeImage from"); ::: layout/painting/nsImageRenderer.h:257 (Diff revision 1) > + /** > + * Draw the image to aRenderingContext which can be used to define the > + * float area. s/float area/float area in the presence of "shape-outside: <image>"/ (Since, out of context, it's not directly clear what this comment is talking about -- there are no other mentions of 'float' or 'shape-outside' in this file.) ::: layout/painting/nsImageRenderer.cpp:7 (Diff revision 1) > /* vim: set ts=8 sts=2 et sw=2 tw=80: */ > /* This Source Code Form is subject to the terms of the Mozilla Public > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > /* utility functions for drawing borders and backgrounds */ Let's update this comment, since this patch is adding code here that's unrelated to borders & backgrounds. Maybe to this: /* utility code for drawing images as CSS borders, backgrounds, & shapes */

Comment on attachment 8942864 [details] Bug 1404222 Part 2 - Trigger reflow after "shape-outside" image has finished decoding a frame. https://reviewboard.mozilla.org/r/213070/#review219426 ::: commit-message-3bba0:1 (Diff revision 1) > +Bug 1404222 Part 2 - Trigger reflow after the image has finished decoding a frame. s/after the image/after "shape-outside" image/ ::: commit-message-3bba0:3 (Diff revision 1) > +Bug 1404222 Part 2 - Trigger reflow after the image has finished decoding a frame. > + > +When we finish decode an image frame, we need to trigger reflow for the s/finish decode/finish decoding/ ::: commit-message-3bba0:4 (Diff revision 1) > +Bug 1404222 Part 2 - Trigger reflow after the image has finished decoding a frame. > + > +When we finish decode an image frame, we need to trigger reflow for the > +frame containing a float with shape image. Perhaps: s/shape image/shape-outside:<image>/ ? ::: layout/style/ImageLoader.cpp:388 (Diff revision 1) > +void > +ImageLoader::DoReflow(FrameSet* aFrameSet) > +{ Could we call this RequestReflow instead of DoReflow? The current name, "DoReflow", makes it sound like it synchronously reflows (inside of this function itself). We also already have a function called DoReflow(), on PresShell, which *does* synchronously reflow. So we shouldn't add other identically-named functions unless they've got approximately the same semantics. (I think you were going for naming consistency with the "DoRedraw" function - but really, *that* function probably wants a rename, too, since it doesn't synchronously redraw. :) Maybe include another patch or spin off a helper bug to rename DoRedraw, if you don't mind?) ::: layout/style/ImageLoader.cpp:394 (Diff revision 1) > + if (frame->StyleDisplay()->mShapeOutside.GetShapeImageData()) { > + // Tell the container of the float to reflow because the shape image > + // is ready, or changed, etc. > + nsIFrame* floatContainer = frame->GetInFlowParent(); > + floatContainer->PresShell()->FrameNeedsReflow( > + floatContainer, nsIPresShell::eStyleChange, NS_FRAME_IS_DIRTY); > + } Two things: (1) I think we only need to bother with this for frames that are floated, right? (Could we add a check on... frame->StyleDisplay()->mFloat != StyleFloat::None ...to the outer condition here?) (2) I think "or changed" in the code-comment is too vague & is possibly incorrect. (What "change" does this code actually handle? If the actual <image> value changes e.g. to a different URL, then that gets handled in nsStyleDisplay::CalcDifference, not here, IIUC.) I think the only "change" that can/should trigger this code are the image finishing its first frame -- so maybe reword to make that clearer? E.g.: "...because the shape image has finished decoding its first frame" ::: layout/style/ImageLoader.cpp:508 (Diff revision 1) > + // We also want to reflow if the image is used by shape-outside. > + DoReflow(frameSet); It looks like this will fire on *every* image-frame decode, for e.g. an animated-GIF "shape-outside:[image]" (since this is in OnFrameComplete which gets called on each frame). So that would trigger a lot of needless reflows. Can we avoid this, e.g. by unregistering/disassociating after this has fired once? (Or: if we already do that, please add a comment here indicating where/how, so that it's clearer that we don't *actually intend* to reflow on every frame-complete-notification, even though it looks like that's what'd happen here.) Also: can you make sure we have a test somewhere with an animated GIF (or PNG, or SVG) which ensures that we specifically only use the first frame for float calculations? ::: layout/style/nsStyleStruct.h:2478 (Diff revision 1) > { > MOZ_ASSERT(mType == StyleShapeSourceType::Image, "Wrong shape source type!"); > return mShapeImage; > } > > + imgRequestProxy* GetShapeImageData() const; Could you add a brief comment here, to make it clearer that this is just a convenience method & that callers don't have to bother e.g. checking the type before calling this? Maybe: // Iff we have "shape-outside:<image>" w/ an image URI (not a gradient), this // method returns the corresponding imgRequestProxy*. Else, returns null.

Comment on attachment 8942868 [details] Bug 1404222 Part 4 - Add web-platform-tests for linear-gradient with writing-modes. https://reviewboard.mozilla.org/r/213078/#review219474 ::: commit-message-12e5f:1 (Diff revision 1) > +Bug 1404222 Part 6 - Add web-platfrom-tests for linear-gradient with writing-modes. Typo: s/platfrom/platform/ (the "r" and "o" are swapped) ::: testing/web-platform/meta/MANIFEST.json:123186 (Diff revision 1) > + "css/css-shapes/shape-outside/shape-image/gradients/shape-outside-linear-gradient-005.html": [ > + [ > + "/css/css-shapes/shape-outside/shape-image/gradients/shape-outside-linear-gradient-005.html", Chrome on my machine doesn't pass any of the new tests in this patch, AFAICT (shape-outside-linear-gradient-005.html through 008). I see red & misshapen squares in all of them. Could you file a bug[1] for at least the first two (005 and 006) to be sure they're aware & to be sure we're in agreement on the correct spec-interpretation here? (The last two tests (007 and 008) trivially fail in Chrome because they don't support sideways-rl/sideways-lr[2], so no need to file a new bug on those ones.) [1] https://bugs.chromium.org/p/chromium/issues/ [2] https://bugs.chromium.org/p/chromium/issues/detail?id=680331

Comment on attachment 8942868 [details] Bug 1404222 Part 4 - Add web-platform-tests for linear-gradient with writing-modes. https://reviewboard.mozilla.org/r/213078/#review219508 ::: testing/web-platform/tests/css/css-shapes/shape-outside/shape-image/gradients/shape-outside-linear-gradient-005.html:24 (Diff revision 1) > + #float-left { > + float: left; Note: It might be helpful to people reading/grokking this test if you added a comment inside this rule, like the following: /* Note: In .container's writing-mode, "float:left" actually floats us towards the top. */ And similar in the #float-right{...} rule (which floats us towards the bottom, IIUC. Consider adding these comments to all 4 new testcases here. (They'd be identical in the first 3 tests, and the comments would have "top"/"bottom" swapped in the final test).

Comment on attachment 8942863 [details] Bug 1404222 Part 1 - Implement shape-outside: <image>. https://reviewboard.mozilla.org/r/213068/#review219412 > I think the comment might be misleading here. AFAICT, IsEmpty() doesn't mean "empty float" -- instead, it means "empty float container" (or something like that)... right? > > (Looks to me like this IsEmpty() is defined as mRect.IsEmpty(), which it looks like is basically equivalent to aContainerSize.IsEmpty(). So we're testing the passed-in aContainerSize here, basically.) I think the mRect is initialized as the aMarginRect (of the float), not aContainerSize. I'll make the comment clearer.

Comment on attachment 8942864 [details] Bug 1404222 Part 2 - Trigger reflow after "shape-outside" image has finished decoding a frame. https://reviewboard.mozilla.org/r/213070/#review219426 > Could we call this RequestReflow instead of DoReflow? > > The current name, "DoReflow", makes it sound like it synchronously reflows (inside of this function itself). We also already have a function called DoReflow(), on PresShell, which *does* synchronously reflow. So we shouldn't add other identically-named functions unless they've got approximately the same semantics. > > (I think you were going for naming consistency with the "DoRedraw" function - but really, *that* function probably wants a rename, too, since it doesn't synchronously redraw. :) Maybe include another patch or spin off a helper bug to rename DoRedraw, if you don't mind?) Yeh. `RequestReflow()` is a better name. Filed bug 1433039 for renaming `DoRedraw()`. > It looks like this will fire on *every* image-frame decode, for e.g. an animated-GIF "shape-outside:[image]" (since this is in OnFrameComplete which gets called on each frame). So that would trigger a lot of needless reflows. > > Can we avoid this, e.g. by unregistering/disassociating after this has fired once? (Or: if we already do that, please add a comment here indicating where/how, so that it's clearer that we don't *actually intend* to reflow on every frame-complete-notification, even though it looks like that's what'd happen here.) > > Also: can you make sure we have a test somewhere with an animated GIF (or PNG, or SVG) which ensures that we specifically only use the first frame for float calculations? I haven't look into how to avoid firefox multiple reflow requests on every frame-complete-notification. Perhaps it suffices to fire requeust reflow once on the decode-complete phase. testing/web-platform/tests/css/css-shapes/shape-outside/shape-image/shape-image-025.html tests animated GIF. `nsImageRenderer::DrawShapeImage` in Part 1 pass `imgIContainer::FRAME_FIRST` flag to image container's `Draw()` to ensure the first frame of the GIF is used.

Comment on attachment 8942868 [details] Bug 1404222 Part 4 - Add web-platform-tests for linear-gradient with writing-modes. https://reviewboard.mozilla.org/r/213078/#review219474 > Chrome on my machine doesn't pass any of the new tests in this patch, AFAICT (shape-outside-linear-gradient-005.html through 008). I see red & misshapen squares in all of them. > > Could you file a bug[1] for at least the first two (005 and 006) to be sure they're aware & to be sure we're in agreement on the correct spec-interpretation here? (The last two tests (007 and 008) trivially fail in Chrome because they don't support sideways-rl/sideways-lr[2], so no need to file a new bug on those ones.) > > [1] https://bugs.chromium.org/p/chromium/issues/ > [2] https://bugs.chromium.org/p/chromium/issues/detail?id=680331 Sure. Filed https://bugs.chromium.org/p/chromium/issues/detail?id=805809

The latest patch set should have addressed all the review comments except the multiple reflow requests issue mentioned in comment 30. This bug required dedicating time to work on. I'm sorry that I might not be able to contribute to this feature for the moment. So unassign myself.

I'll try to complete this work.

(In reply to Daniel Holbert [:dholbert] from comment #26) > ::: layout/style/ImageLoader.cpp:508 > (Diff revision 1) > > + // We also want to reflow if the image is used by shape-outside. > > + DoReflow(frameSet); > > It looks like this will fire on *every* image-frame decode, for e.g. an > animated-GIF "shape-outside:[image]" (since this is in OnFrameComplete which > gets called on each frame). So that would trigger a lot of needless reflows. I have confirmed it does NOT fire onFrameComplete on every image-frame decode, though I haven't figured out why yet. When I figure it out, I'll add an explanatory comment. There is a separate problem with the implementation of DoReflow/RequestReflow: it triggers reflow in an onFrameComplete from a background-image associated with the frame, which is incorrect. I'll figure out a fix for this as well.

(In reply to Brad Werth [:bradwerth] from comment #40) > I have confirmed it does NOT fire onFrameComplete on every image-frame > decode, though I haven't figured out why yet. When I figure it out, I'll add > an explanatory comment. I think this is just event notification collapsing -- that all 3 frames in my example are decoded before the first event is fired, so my callback is only getting called once. An animated GIF with more frames will likely fire more events, so this still needs a fix. My plan is to extend ImageLoader::AssociateRequestToFrame with a new bool parameter indicating that animation refresh request is needed or not.

Pulling and reposting the patches seriously confused MozReview -- I did something wrong. I've tried to setup review flags that preserve the existing r+ on Ting Yu's patches.

Yeah, I think MozReview can't handle multiple people posting patches on bus. :( It looks you've cleaned up Bugzilla to make this coherent, but even so, once you click over to MozReview, it's got two separate concepts of patch-stacks for this bug (one with TYLin's stack, and one with your version). We might want to just abandon MozReview for this bug and use attachments for Parts 1 and 2 at least (the parts that need r?dbaron) -- IIRC, dbaron prefers normal attachments rather than MozReview anyway, particularly when MozReview goes nuts as it has here. :)

Comment on attachment 8949221 [details] Bug 1404222 Part 7: Turn off a 'todo' in a mochitest. https://reviewboard.mozilla.org/r/218598/#review224422 (anyway, r=me on the CORS mochitest todo-removal)

I guess I should abandon my patch stack to avoid confusing MozReview, and let Brad re-upload his patches and set review flags properly.

Many of the web platform tests are intermittent. Clearing review flags until the patch consistently passes all of them (that don't also involve shape-margin).

I've confirmed that the intermittent wpt failures are due to the reftest receiving a "load" event before the shape-outside image has been applied by the nsFloatManager. I need advice on how to block the load event until the image has been loaded and floated. bz, can you give me some advice on how to proceed? I can sort of see how image loading blocks the load event, and I imagine I can find the place where the initial reflow blocks the loading event. But I don't see how that first reflow float calculation is chained to the image loading, which would give me an indication of how to add images loaded from shape-outside into that dependency chain. All insights appreciated.

So I'd have thought the stuff in patch 2 -- that modifies the ImageLoader and its calling code -- should have dealt with that. It's analogous to what that code does background images and border images. But in this case an image load completion requires reflow, which is not true of background-image or border-image... but it *used* to be true for border-image, so there might be old code in the file's history that could be resurrected, that might be slightly preferable to the code there. (I'd consider the code in ImageLoader::RequestReflowIfNeeded to be a little roundabout -- the old code used to handle whether reflow was needed in the registration.) I wonder if the call to nsLayoutUtils::DeregisterImageRequest is messing things up.

(Though there might be the process of waiting for an image decode somewhere in the middle.)

In general, firing the load event will wait for the image load, but will _not_ do an extra reflow unless there are user fonts involved. That said, if layout has been requested by then by something, it would presumably happen before we snapshot things or any time we ask for layout information. You probably do want to associate the new request before disassociating the old one.

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #88) > So I'd have thought the stuff in patch 2 -- that modifies the ImageLoader > and its calling code -- should have dealt with that. > > It's analogous to what that code does background images and border images. > But in this case an image load completion requires reflow, which is not true > of background-image or border-image... but it *used* to be true for > border-image, so there might be old code in the file's history that could be > resurrected, that might be slightly preferable to the code there. (I'd > consider the code in ImageLoader::RequestReflowIfNeeded to be a little > roundabout -- the old code used to handle whether reflow was needed in the > registration.) > > I wonder if the call to nsLayoutUtils::DeregisterImageRequest is messing > things up. (In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #90) > In general, firing the load event will wait for the image load, but will > _not_ do an extra reflow unless there are user fonts involved. That said, > if layout has been requested by then by something, it would presumably > happen before we snapshot things or any time we ask for layout information. > > You probably do want to associate the new request before disassociating the > old one. Thank you both for your help. I've confirmed that the order of registering and registering image requests is not the culprit (though the next posted patch will do as bz proposes above), and that the deregistering the shape-outside image after getting the first frame is not the culprit either. The image fetch for the shape-outside is just not getting added to the document loadgroup or doing anything else to block the load event firing. Interestingly, in my testing, it appears that background-image loads don't block the load event, either (nsDocument::UnblockDOMContentLoaded fires before imgLoader::LoadImage). That surprises me -- I assume that would also cause intermittent reftest failures. So I'm not finding examples that show a way to block the load event from anything other than HTMLImageElements. I'll test border-image next and see if that points to a solution.

> background-image loads don't block the load event, either > (nsDocument::UnblockDOMContentLoaded fires before imgLoader::LoadImage) The "DOMContentLoaded" event and the "load" event are two different events. The former fires when parsing is done and is not affected by pending subresource loads. The latter _is_ affected by pending subresource loads. > The image fetch for the shape-outside is just not getting added to the document loadgroup That's odd. If you're going through nsContentUtils::LoadImage then NewImageChannel should add things to the right loadgroups...

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #92) > > background-image loads don't block the load event, either > > (nsDocument::UnblockDOMContentLoaded fires before imgLoader::LoadImage) > > The "DOMContentLoaded" event and the "load" event are two different events. > The former fires when parsing is done and is not affected by pending > subresource loads. The latter _is_ affected by pending subresource loads. Ah, that explains that result, then. What's the right place to breakpoint the "load" event firing? > > The image fetch for the shape-outside is just not getting added to the document loadgroup > > That's odd. If you're going through nsContentUtils::LoadImage then > NewImageChannel should add things to the right loadgroups... I was incorrect. The image does get added to the loadgroup. The call chain is going from nsStyleDisplay::FinishStyle through to nsContentUtils::LoadImage. But if that call happens after the load event has fired, we could still see the observed behavior.

(In reply to Brad Werth [:bradwerth] from comment #93) > Ah, that explains that result, then. What's the right place to breakpoint > the "load" event firing? I'm using nsDocumentViewer::LoadComplete now. Looks like that is the right signifier.

> What's the right place to breakpoint the "load" event firing? nsDocumentViewer::LoadComplete, as you found. > But if that call happens after the load event has fired, we could still see the observed behavior. Right. We should be making sure somehow that it happens before we've fired the load event... What's the callstack to that FinishStyle call?

Here's the relevant call stacks, in order. 1) RestyleManager::ProcessRestyledFrames -> ... -> nsStyleDisplay::FinishStyle (loads the image) 2) PresShell::DoReflow -> ... -> nsFloatManager::AddFloat (first reflow, but image isn't ready yet -- only size is available) 3) nsDocumentViewer::LoadComplete (fires onload) 4) ImageLoader::onFrameComplete (image available, requests restyle) 5) PresShell::DoReflow -> ... -> nsFloatManager::AddFloat (second reflow, image ready, successful) So the load event IS being delayed past the loading the image, contrary to my earlier repeated assertions. The actual problem is that the float can't happen on the first reflow because the first frame isn't loaded yet (nsStyleImage::IsComplete is returning false). So solutions I can imagine are... a) In the first nsFloatManager::AddFloat, when it's detected that the shape-outside image isn't complete, block the document onLoad, then release that on the second nsFloatManager::AddFloat. Have to handle timeouts and other nastiness here. b) Make the image load synchronous during the first nsFlaotManager::AddFloat, or otherwise block until image is complete. I'll start working on option a. Thank you again for your help.

Hold on. Image _loads_ block onload, but image _decodes_ do not, last I checked. But image decoding (apart from knowing the image size) can't affect layout ... at least until this patch. Looking at the spec for shape-outside, it sounds like we need to block onload on decoding of shape-outside images, right? In that case, option (a), or some way of indicating to imagelib that we want to block load on decode, seems to be the right way to go. Presumably we are already indicating in some way that we want to do a eager decode on these images?