Closed Bug 1404222 Opened 2 years ago Closed 2 years ago

Support shape-outside: <image>

Categories

(Core :: Layout: Floats, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox57 --- wontfix
firefox61 --- fixed

People

(Reporter: TYLin, Assigned: bradwerth)

References

(Blocks 3 open bugs, )

Details

(Keywords: dev-doc-complete, Whiteboard: [designer-tools][wptsync upstream])

Attachments

(10 files, 42 obsolete files)

59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
4.97 KB, patch
Details | Diff | Splinter Review
25.93 KB, patch
Details | Diff | Splinter Review
24.95 KB, patch
Details | Diff | Splinter Review
No description provided.
Depends on: 1404243
Do you attach wrong patches? Looks like the patches not related to shape-outside.
Flags: needinfo?(tlin)
Oops. Wrong bug number ...
Flags: needinfo?(tlin)
Attachment #8918815 - Attachment is obsolete: true
Attachment #8918815 - Flags: review?(mtseng)
Attachment #8918816 - Attachment is obsolete: true
Attachment #8918816 - Flags: review?(mtseng)
Depends on: 1418224
Depends on: 1418930
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?
Flags: needinfo?(tlin)
I want to use this bug to implement layout part of shape-outside: <image>.
Flags: needinfo?(tlin)
(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.)
Flags: needinfo?(tlin)
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.)
Attachment #8942865 - Flags: review?(dholbert) → review-
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/
Attachment #8942866 - Flags: review?(dholbert) → review+
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?
Attachment #8942867 - Flags: review?(dholbert) → review+
(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.
Blocks: 1430932
Comment on attachment 8942869 [details]
Bug 1404222 Part 5 - Add a crashtest.

https://reviewboard.mozilla.org/r/213080/#review219102
Attachment #8942869 - Flags: review?(dholbert) → review+
Comment on attachment 8942870 [details]
Bug 1404222 Part 8 - Update test expectations after implementing shape-outside: <image>.

https://reviewboard.mozilla.org/r/213082/#review219104
Attachment #8942870 - Flags: review?(dholbert) → review+
Blocks: 1430983
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.
Flags: needinfo?(tlin)
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 */
Attachment #8942863 - Flags: review?(dholbert)
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.
Attachment #8942864 - Flags: review?(dholbert) → review-
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).
Attachment #8942868 - Flags: review?(dholbert) → review+
Whiteboard: [designer-tools]
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
Attachment #8942865 - Attachment is obsolete: true
Attachment #8942867 - Attachment is obsolete: true
Attachment #8942870 - Attachment is obsolete: true
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.
Assignee: aethanyc → nobody
Status: ASSIGNED → NEW
See Also: → 1433039
I'll try to complete this work.
Assignee: nobody → bwerth
(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.
Attachment #8942864 - Attachment is obsolete: true
Attachment #8949216 - Attachment is obsolete: true
Attachment #8949218 - Attachment is obsolete: true
Attachment #8949219 - Attachment is obsolete: true
Attachment #8949220 - Attachment is obsolete: true
Attachment #8949221 - Flags: review?(dholbert)
Attachment #8942863 - Flags: review?(dbaron)
Attachment #8949217 - Flags: review?(dbaron)
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)
Attachment #8949221 - Flags: review?(dholbert) → review+
Attachment #8942863 - Attachment is obsolete: true
Attachment #8942863 - Flags: review?(dbaron)
Attachment #8942866 - Attachment is obsolete: true
Attachment #8942868 - Attachment is obsolete: true
Attachment #8942869 - Attachment is obsolete: true
I guess I should abandon my patch stack to avoid confusing MozReview, and let Brad re-upload his patches and set review flags properly.
Attachment #8949216 - Flags: review?(dbaron)
Attachment #8949217 - Flags: review?(dbaron)
Attachment #8949218 - Flags: review?(dholbert)
Attachment #8949219 - Flags: review?(dholbert)
Attachment #8949220 - Flags: review?(dholbert)
Attachment #8949216 - Flags: review?(dbaron)
Attachment #8949217 - Flags: review?(dbaron)
Attachment #8949218 - Flags: review?(dholbert)
Attachment #8949219 - Flags: review?(dholbert)
Attachment #8949220 - Flags: review?(dholbert)
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.
Flags: needinfo?(bzbarsky)
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.
Flags: needinfo?(bzbarsky)
(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.
Flags: needinfo?(bzbarsky)
(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.
Flags: needinfo?(bzbarsky)
> 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?
Blocks: 1439709
Attachment #8952483 - Attachment is obsolete: true
Attachment #8949216 - Flags: review?(dbaron)
Attachment #8949217 - Flags: review?(dbaron)
Attachment #8951808 - Flags: review?(dbaron)
Attachment #8951809 - Flags: review?(dbaron)
Attachment #8951810 - Flags: review?(dbaron)
Attachment #8951083 - Flags: review?(dbaron)
Attachment #8949218 - Flags: review?(dholbert)
Attachment #8949219 - Flags: review?(dholbert)
Attachment #8949220 - Flags: review?(dholbert)
Attachment #8949922 - Flags: review?(dholbert)
Attachment #8949547 - Flags: review?(dholbert)
Comment on attachment 8949218 [details]
Bug 1404222 Part 4: Replace hash '#' with %23 in SVG data URIs, to fix XML parse errors.

https://reviewboard.mozilla.org/r/218592/#review228374
Attachment #8949218 - Flags: review?(dholbert) → review+
Comment on attachment 8949219 [details]
Bug 1404222 Part 5: Add web-platform-tests for linear-gradient with writing-modes.

https://reviewboard.mozilla.org/r/218594/#review228380
Attachment #8949219 - Flags: review?(dholbert) → review+
Comment on attachment 8949220 [details]
Bug 1404222 Part 6: Add a crashtest.

https://reviewboard.mozilla.org/r/218596/#review228384
Attachment #8949220 - Flags: review?(dholbert) → review+
Comment on attachment 8949922 [details]
Bug 1404222 Part 8: Fix wpt reftest shape-image-001.html to correct a too-wide container.

https://reviewboard.mozilla.org/r/219222/#review228388

::: commit-message-22502:1
(Diff revision 5)
> +Bug 1404222 Part 11: Fix wpt reftest shape-image-001.html to correct a too-wide container.
> +
> +MozReview-Commit-ID: 3fwtUNCqWLX

In cases where we're editing another vendor's test, we should try to be clear about what was wrong & that the change is correct and not arbitrary.

So, in light of that -- could you provide a bit more detail in the commit message about what was wrong here and why the fix is correct?  That'll help justify the change when this is merged to upstream WPT, if anyone there gets curious about why we messed with this CSS.

(And: if Chrome/Blink "passes" this test regardless of your change, and that's incorrect, perhaps file a bug on them?)
Attachment #8949922 - Flags: review?(dholbert) → review-
Comment on attachment 8949547 [details]
Bug 1404222 Part 10: Mark as PASS all shape-outside image web-platform-tests that don't rely on shape-margin.

https://reviewboard.mozilla.org/r/218898/#review228390

::: commit-message-8229d:1
(Diff revision 8)
> +Bug 1404222 Part 12: Mark as PASS all shape-outside image web-platform-tests that don't rely on shape-margin or SVG data URIs.
> +
> +MozReview-Commit-ID: FNgeeBpFtMs

(1) Could you add a brief mention in a second line of this commit message, about why these two specific categories of tests aren't passing yet?

i.e.:
 - shape-margin: not yet supported
 - SVG Data URIs: I'm not sure

(2) ...and actually, it looks like you *are* marking some tests as PASS that *do* rely on SVG data URIs: shape-image-002.html and shape-image-005.html in particular.  So this needs a bit of clarification.
Attachment #8949547 - Flags: review?(dholbert) → review-
Comment on attachment 8949922 [details]
Bug 1404222 Part 8: Fix wpt reftest shape-image-001.html to correct a too-wide container.

https://reviewboard.mozilla.org/r/219222/#review228388

> In cases where we're editing another vendor's test, we should try to be clear about what was wrong & that the change is correct and not arbitrary.
> 
> So, in light of that -- could you provide a bit more detail in the commit message about what was wrong here and why the fix is correct?  That'll help justify the change when this is merged to upstream WPT, if anyone there gets curious about why we messed with this CSS.
> 
> (And: if Chrome/Blink "passes" this test regardless of your change, and that's incorrect, perhaps file a bug on them?)

Sure, makes sense. I'll explain the rationale in an expanded commit message. Chrome does not pass the test as written.
Blocks: 1418470
Comment on attachment 8949216 [details]
Bug 1404222 Part 1: Implement shape-outside: <image>.

https://reviewboard.mozilla.org/r/218588/#review229352


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: layout/generic/nsFloatManager.cpp:1042
(Diff revision 10)
> +    // If we're in the area between w and aStride, skip this pixel,
> +    // or if we're vertical, skip the remainder of the loop.
> +    if (col >= w) {
> +      if (aWM.IsVertical()) {
> +        break;
> +      } else {

Warning: Do not use 'else' after 'break' [clang-tidy: readability-else-after-return]
Comment on attachment 8949547 [details]
Bug 1404222 Part 10: Mark as PASS all shape-outside image web-platform-tests that don't rely on shape-margin.

https://reviewboard.mozilla.org/r/218898/#review230124
Attachment #8949547 - Flags: review?(dholbert) → review+
Comment on attachment 8949922 [details]
Bug 1404222 Part 8: Fix wpt reftest shape-image-001.html to correct a too-wide container.

https://reviewboard.mozilla.org/r/219222/#review230126

::: commit-message-22502:5
(Diff revision 7)
> +Bug 1404222 Part 11: Fix wpt reftest shape-image-001.html to correct a too-wide container.
> +
> +MozReview-Commit-ID: 3fwtUNCqWLX
> +
> +The test currently stretches a 100 x 100 image to 150 pixels wide, which makes the shaded region of the png stretch to 75 pixels. None of the math in the rest of the test accounts for this stretching, and the test fails on all browsers. It seems clear that the intention was to use an unstretched, 100 pixel wide image, which makes the test pass.

(not a big deal in the grand scheme of this patch stack -- hence not ticking "open an issue" -- but you might want to rewrap this long comment to be 80 characters per line.

(Typically, we put the main commit message on a single line regardless of its length (since the first line of a commit message has special semantic meaning), but then subsequent extra-info lines get wrapped at 80 characters.)
Attachment #8949922 - Flags: review?(dholbert) → review+
Blocks: 1442702
Attachment #8955588 - Flags: review?(dholbert)
Attachment #8955588 - Flags: review?(dholbert)
Attachment #8951808 - Attachment is obsolete: true
Attachment #8951808 - Flags: review?(dbaron)
Attachment #8951809 - Attachment is obsolete: true
Attachment #8951809 - Flags: review?(dbaron)
Attachment #8951810 - Attachment is obsolete: true
Attachment #8951810 - Flags: review?(dbaron)
Attachment #8951083 - Attachment is obsolete: true
Attachment #8951083 - Flags: review?(dbaron)
Attachment #8955588 - Attachment is obsolete: true
Due to MozReview's inability to inspect old patch state in a usable way
(some combination of bugs like bug 1234279, bug 1249468, bug 1296135),
and the fact that the MozReview history is no longer visible in bugzilla
because the comments are hidden, I'm no longer accepting review requests
in MozReview.  If you'd like me to review patches, please submit those
review requests as attached patch files.
Flags: needinfo?(bwerth)
Attachment #8949216 - Flags: review?(dbaron)
Attachment #8949216 - Attachment is obsolete: true
Attachment #8957249 - Flags: review?(dbaron)
Attachment #8949217 - Attachment is obsolete: true
Attachment #8949217 - Flags: review?(dbaron)
Flags: needinfo?(bwerth)
Attachment #8957250 - Flags: review?(dbaron)
Comment on attachment 8957249 [details] [diff] [review]
Bug 1404222 Part 1: Implement shape-outside: <image>.

># User Brad Werth <bwerth@mozilla.com>

Didn't Ting-Yu write part of this?  If so, his name and email should be
on here also.  (Just list both, comma-separated.)


>+  // An Interval stores line-left and line-right at a given block coordinate
>+  // in the float manager's coordinate space. The Y() of mLineLeft and
>+  // mLineRight should be the same.

This should probably use ".y" rather than "Y()" since that's far more common.
Further, I think it should be clearer that the y is actually a logical coordinate
in the block direction, and the .x is in inline direction.

nsFloatManager::ImageShapeInfo::ImageShapeInfo should
MOZ_ASSERT(aStride >= aImageSize.width).

>+  const uint8_t threshold = NSToIntRound(aShapeImageThreshold * 255);

This seems wrong.  Since the threshold is defined in the spec so that
pixels that are *more* opaque than the threshold should be used, I think
this should use NSToIntFloor, so that, if the unrounded value is 49.7,
the uint8_t will be 49 so that a value of 50 will count and a value of
49 won't.

There should also be a test that tests this... preferably in
layout/reftests/w3c-css/submitted/shapes1/.

>+  int32_t min = -1;
>+  int32_t max = -1;
...
>+      // Reset min and max for the next row or column.
>+      min = -1;
>+      max = -1;

Just declare these variables inside the outer loop rather than doing
this resetting.

>+      if (max < curr) {
>+        max = curr;
>+      }

Seems better to just:
   MOZ_ASSERT(max < curr);
   max = curr;


That said, I guess I don't see how this loop actually works for vertical
writing-modes, since it always scans the image in horizontal rows, but the
logic depends on the image being scanned an inline-direction row at a time.
It seems like in a horizontal writing mode, we'll scan one row and then
enter this condition:
  >+    // At the end of a row (or column if vertical), and found something.
  >+    if (curr == (aWM.IsVertical() ? h - 1 : w - 1) && (min != -1)) {
but in a vertical writing-mode, we'll scan almost the entire image and
then enter that condition for every pixel as we scan the bottom row,
thus putting almost all the data from the image in the interval for the
leftmost physical column of the image.  Or am I missing something here?
(I'm marking the patch review- for this.)

>+      if (aWM.IsVerticalLR() && aWM.IsSideways()) {
>+        // sideways-lr: its physical directions of line-left and line-right
>+        // are bottom and top, which are the opposite of other vertical
>+        // writing modes.
>+        lineLeft.MoveBy(colAppUnits, maxAppUnits);
>+        lineRight.MoveBy(colAppUnits, minAppUnits);
>+      } else if (aWM.IsVertical()) {
>+        lineLeft.MoveBy(colAppUnits, minAppUnits);
>+        lineRight.MoveBy(colAppUnits, maxAppUnits);
>+      } else {
>+        // horizontal-tb
>+        lineLeft.MoveBy(minAppUnits, rowAppUnits);
>+        lineRight.MoveBy(maxAppUnits, rowAppUnits);
>+      }

Given that there was once discussion of having sideways-rl, I think it's
more future proof to make the first test just for Vertical rather than
VerticalLR, and then restructure so the code is:

  if (aWM.IsVertical()) {
    if (aWM.IsSideways()) {
      // ...
    } else {
      // ...
    }
  } else {
    // ...
  }

That said, I think once the other issues are fixed, a simpler way to
structure this code may fall out.


I think the LineLeft and LineRight methods should do binary search
rather than linear search.  The data structure is even already set up for it.
(They could share the code.)

>+  if (IsEmpty()) {
>+    // Per spec, a float area defined by a shape is clipped to the float’s
>+    // margin box. Therefore, no need to create a shape info if the float's
>+    // margin rect is empty.
>+    return;
>+  }

Hmmm... could you update this comment elsewhere in nsFloatManager.cpp to cite
css-shapes saying it's actually correct:
>      // For compatibility, ignore floats with empty rects, even though it
>      // disagrees with the spec.  (We might want to fix this in the
>      // future, though.)
(Might still want to mention that CSS2 says it's incorrect... if it
still does.)


I'd like dholbert (or someone else with imagelib knowledge) to review
nsFloatManager::ShapeInfo::CreateImageShape and
nsImageRenderer::DrawShapeImage; he knows the image code better than I
do.

I'm a little worried about the interaction with decoding and discarding,
and what happens when the frame's content rect changes.



Are there tests in layout/reftests/w3c-css/submitted/shapes1/ (or
imported web-platform-tests from other browsers) that test:
 * different writing modes.  (I suspect not, given above.)
 * zoom (this actually can't be in the W3C tests since reftest-zoom
   isn't part of the format)
(and probably these should be tested with png, svg, and gradients).
Attachment #8957249 - Flags: review?(dbaron) → review-
Comment on attachment 8957250 [details] [diff] [review]
Bug 1404222 Part 2: Block onload when shape-outside images are first decoded, and keep it blocked until a second reflow is complete.

Do you know what the implications are of using std::unordered_set for codesize?  I'd be inclined to suggest use of our own hashtable classes instead, if you're going to use hashtables.
Flags: needinfo?(bwerth)
Also, I don't understand why it makes sense to call ForgetAllBlockingResources at the callsite.  Could you explain?  And preferably also add comments to explain.
Comment on attachment 8957250 [details] [diff] [review]
Bug 1404222 Part 2: Block onload when shape-outside images are first decoded, and keep it blocked until a second reflow is complete.

I don't see why the new methods on nsIDocument should be virtual; they
could just be like DefaultStyleAttrURLData, I'd think, and be nsIDocument
methods implemented in nsDocument.cpp.

(more later...)
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #224)
> Comment on attachment 8957250 [details] [diff] [review]
> Bug 1404222 Part 2: Block onload when shape-outside images are first
> decoded, and keep it blocked until a second reflow is complete.
> 
> I don't see why the new methods on nsIDocument should be virtual; they
> could just be like DefaultStyleAttrURLData, I'd think, and be nsIDocument
> methods implemented in nsDocument.cpp.

Yeah, agreed, that would be extremely nice, specially since I'm devirtualizing them lately (bug 1443553, bug 1443756, bug 1444580). We have a lot of stuff that that declares a function as pure virtual, then only implements it once, it'd be nice to stop using this pattern :)
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #223)
> Also, I don't understand why it makes sense to call
> ForgetAllBlockingResources at the callsite.  Could you explain?  And
> preferably also add comments to explain.

This was a misfire. In an earlier formulation of the patch, I used this callsite to prevent a load timeout. With the current state of the patch, I think the only way we can get a timeout is if the shape-outside image reports its size but never decodes its first frame (if OnSizeAvailable fires but OnFrameComplete doesn't). I *think* that is impossible and therefore the code is safe from a timeout.

I've removed the callsite to ForgetAllBlockingResources, and the function definition itself. If future callers start using BlockOnloadOnResource and need a way to clear out all resources at once, that function can be re-created (and tested) then.
Comment on attachment 8957250 [details] [diff] [review]
Bug 1404222 Part 2: Block onload when shape-outside images are first decoded, and keep it blocked until a second reflow is complete.

... continued from comment 222, comment 223, comment 224...


nsDocument::UnblockOnloadOnResource should probably assert that
aResource is present in mOnloadBlockingResources (maybe using the return
value of erase?), unless there's a good reason not to.

nsDocument::BlockOnloadOnResource should assert that resource isn't
already in the set.  (Except it seems like that assertion will fail if
multiple elements use the same imgIRequest, which can certainly
happen...  but is the code all going to work correctly in that case?)

The code in ImageLoader.cpp that figures out if the current imgRequest
is the shape-outside of the frame (in both BlockOnloadIfNeeded and
RequestReflowIfNeeded) is quite awkward -- could you instead pass a
value (probably an enum?) in so that you don't have to do this?

Why does BlockOnloadIfNeeded happen in OnSizeAvailable?  Isn't that once
the beginning of the image has arrived from the network?  Couldn't
onload have fired already by that point?  (You should be able to add a
test for this with a loading mechanism that introduces a small delay,
though maybe it's not worth it.)

What happens when a frame is destroyed while its shape-outside image is
loading (say, because the page made an ancestor display:none?)?
nsFrame::DestroyFrom calls PresShell::NotifyDestroyingFrame which calls
ImageLoader::DropRequestsForFrame... which doesn't remove the resource
from the map... which seems like it will then block onload forever.

If you fix that in the obvious way, then what happens if multiple
elements use the same imgIRequest?  We'd call BlockOnloadOnResource
multiple times.  What if we then (with the new codepath added to fix the
previous issue) remove the imgIRequest from the map?  Doesn't that then
mean that we then won't wait for the image to fire onload, even though
other elements are still waiting for it?

I'm also not crazy about using the nsIReflowCallback mechanism.  It's an
old clunky thing that I'd wish would go away; I'd rather not add another
user.

I'd also suggest that GetShapeImageData return imgIRequest* rather than
imgRequestProxy*, since that's all the caller needs.

I think it might be worth looking at the old code that we had for
border-image back when border-image affected layout.  It may have
avoided some or all of these problems.  Though maybe not...
Attachment #8957250 - Flags: review?(dbaron) → review-
Attachment #8957249 - Attachment is obsolete: true
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #221)
> Comment on attachment 8957249 [details] [diff] [review]
> Bug 1404222 Part 1: Implement shape-outside: <image>.
> That said, I guess I don't see how this loop actually works for vertical
> writing-modes, since it always scans the image in horizontal rows, but the
> logic depends on the image being scanned an inline-direction row at a time.

The loop does work as written. I added a better comment in the updated Part 1 patch:

>// Scan the pixels row by row, from top to bottom (if vertical, column by
>// column, from left to right). We iterate generically over the total
>// number of pixels, and then calculate col and row to create the desired
>// traversal order (by rows for horizontal writing modes, by columns
>// for vertical writing modes).
Flags: needinfo?(bwerth)
(In reply to Brad Werth [:bradwerth] from comment #231)
> The loop does work as written. I added a better comment in the updated Part
> 1 patch:
> 
> >// Scan the pixels row by row, from top to bottom (if vertical, column by
> >// column, from left to right). We iterate generically over the total
> >// number of pixels, and then calculate col and row to create the desired
> >// traversal order (by rows for horizontal writing modes, by columns
> >// for vertical writing modes).

Oh, I guess I'd convinced myself that it was looping over the pixels in the order they were in aAlphaPixels.

Given that it's not -- could you just make it a pair of nested loops over rows and cols?  I think that would be much clearer, wouldn't require manually skipping the padding for the stride, and would allow the per-row logic to be inside the outer loop but outside the inner loop, avoiding the need for special conditions?


(Also, please don't miss comment 227.)
Flags: needinfo?(bwerth)
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #232)
> Given that it's not -- could you just make it a pair of nested loops over
> rows and cols?  I think that would be much clearer, wouldn't require
> manually skipping the padding for the stride, and would allow the per-row
> logic to be inside the outer loop but outside the inner loop, avoiding the
> need for special conditions?

I'm not sure this is going to be clearer. Since we want to iterate col within row for horizontal, and row within col for vertical, the loop logic is going to look pretty weird. I'll do my best and you can decide if you think it is an improvement.
Attachment #8958227 - Attachment is obsolete: true
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #227)
> Comment on attachment 8957250 [details] [diff] [review]
> nsDocument::UnblockOnloadOnResource should probably assert that
> aResource is present in mOnloadBlockingResources (maybe using the return
> value of erase?), unless there's a good reason not to.
> 
> nsDocument::BlockOnloadOnResource should assert that resource isn't
> already in the set.  (Except it seems like that assertion will fail if
> multiple elements use the same imgIRequest, which can certainly
> happen...  but is the code all going to work correctly in that case?)

I may be thinking of this wrong, but I see the current behavior as a feature. I wanted to make a mechanism that would be useful for a different calling pattern, and earlier versions of the patch required support for that calling pattern, with mismatched calls. Assuming the intent of blocking onload on a resource is to unblock as soon as the resource is "ready", then duplicate block requests SHOULD be coalesced, and the first unblock request should be the only one that matters. There is some value in asserting on the unblock of a resource that was never blocked, but since that doesn't cause any problems (there was never a block!) it feels like at most it should be a warning, not an assert. A reference count like system would also just be identical to calling BlockOnload and UnblockOnload on the nsDocument directly -- since the calls have to be paired. If you feel strongly that it should be more like a reference count, I'll strip out these new methods and revert to direct calls of BlockOnload and UnblockOnload.

> The code in ImageLoader.cpp that figures out if the current imgRequest
> is the shape-outside of the frame (in both BlockOnloadIfNeeded and
> RequestReflowIfNeeded) is quite awkward -- could you instead pass a
> value (probably an enum?) in so that you don't have to do this?

I agree it's clunky. I'll try to find a more elegant method.

> Why does BlockOnloadIfNeeded happen in OnSizeAvailable?  Isn't that once
> the beginning of the image has arrived from the network?  Couldn't
> onload have fired already by that point?  (You should be able to add a
> test for this with a loading mechanism that introduces a small delay,
> though maybe it's not worth it.)

I think this is a safe location to start blocking. All the images in the stylesheet are treated as "dependent resources" so the first layout can't occur until sizes are read. The onload can't fire until that layout is complete. So if we block onload when the size is read, we're adding a block within the already existing block/unblock pair, and there is no coverage gap.

> What happens when a frame is destroyed while its shape-outside image is
> loading (say, because the page made an ancestor display:none?)?
> nsFrame::DestroyFrom calls PresShell::NotifyDestroyingFrame which calls
> ImageLoader::DropRequestsForFrame... which doesn't remove the resource
> from the map... which seems like it will then block onload forever.

This would block onload, yes. I'll have to find a solution for this.

> If you fix that in the obvious way, then what happens if multiple
> elements use the same imgIRequest?  We'd call BlockOnloadOnResource
> multiple times.  What if we then (with the new codepath added to fix the
> previous issue) remove the imgIRequest from the map?  Doesn't that then
> mean that we then won't wait for the image to fire onload, even though
> other elements are still waiting for it?

This is one reason to keep the semantics I propose at the beginning of this response. I think those semantics are useful for these cases.

> I'm also not crazy about using the nsIReflowCallback mechanism.  It's an
> old clunky thing that I'd wish would go away; I'd rather not add another
> user.

I agree it feels bad to allocate a callback object and then have it delete itself. I would love to find another solution to trigger an event at the end of layout. Please let me know of other examples I could use as a template.

> I'd also suggest that GetShapeImageData return imgIRequest* rather than
> imgRequestProxy*, since that's all the caller needs.

I'll do that.

> I think it might be worth looking at the old code that we had for
> border-image back when border-image affected layout.  It may have
> avoided some or all of these problems.  Though maybe not...

Yes, you mentioned this before. I struggled to find the original border-image bug. I'll be more diligent and find it.
Flags: needinfo?(bwerth)
(In reply to Brad Werth [:bradwerth] from comment #233)
> I'm not sure this is going to be clearer. Since we want to iterate col
> within row for horizontal, and row within col for vertical, the loop logic
> is going to look pretty weird. I'll do my best and you can decide if you
> think it is an improvement.

Sorry -- I meant that the loops would be over the logical rows/cols, not the physical ones.  So the pixel index calculation would have to vary on writing mode.

(In reply to Brad Werth [:bradwerth] from comment #236)
> I may be thinking of this wrong, but I see the current behavior as a
> feature. I wanted to make a mechanism that would be useful for a different
> calling pattern, and earlier versions of the patch required support for that
> calling pattern, with mismatched calls. Assuming the intent of blocking
> onload on a resource is to unblock as soon as the resource is "ready", then
> duplicate block requests SHOULD be coalesced, and the first unblock request
> should be the only one that matters. There is some value in asserting on the
> unblock of a resource that was never blocked, but since that doesn't cause
> any problems (there was never a block!) it feels like at most it should be a
> warning, not an assert. A reference count like system would also just be
> identical to calling BlockOnload and UnblockOnload on the nsDocument
> directly -- since the calls have to be paired. If you feel strongly that it
> should be more like a reference count, I'll strip out these new methods and
> revert to direct calls of BlockOnload and UnblockOnload.

I guess it could be viable, but you need to figure out how to handle what happens when one particular thing no longer needs the resource, but something else might.

> I think this is a safe location to start blocking. All the images in the
> stylesheet are treated as "dependent resources" so the first layout can't
> occur until sizes are read. The onload can't fire until that layout is
> complete. So if we block onload when the size is read, we're adding a block
> within the already existing block/unblock pair, and there is no coverage gap.

OK, that should be explained in a comment.
Attachment #8958315 - Attachment is obsolete: true
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #227)
> Comment on attachment 8957250 [details] [diff] [review]

> What happens when a frame is destroyed while its shape-outside image is
> loading (say, because the page made an ancestor display:none?)?
> nsFrame::DestroyFrom calls PresShell::NotifyDestroyingFrame which calls
> ImageLoader::DropRequestsForFrame... which doesn't remove the resource
> from the map... which seems like it will then block onload forever.

I've found a solution for this. ImageLoader::DropRequestsForFrame is also used for frame recreation, so it wasn't suitable to use as the point to unblock onload on the document. Instead I focused on cases where the imgIRequest was going away, being untracked, etc. Those places are:

* ImageLoader::OnFrameComplete: Will unblock the document even if no frames are associated with the request anymore.
* ImageLoader::ClearFrames: If all the frames are going away, requests get cleared, too.
* ImageLoader::RemoveImage: This gets called by the css::ImageValue destructor. The request itself is being destroyed.
Attachment #8958593 - Attachment is obsolete: true
Attachment #8958925 - Flags: review?(dbaron)
Attachment #8958925 - Flags: review?(dholbert)
Comment on attachment 8958925 [details] [diff] [review]
Bug 1404222 Part 1: Implement shape-outside: <image>.

I'd suggest the following renames in the ImageShapeInfo constructor, to
be closer to our conventions for logical sizes and positions ("inline"
and "block" style naming):

  outerLimit -> bSize
  innerLimit -> iSize
  outer -> b (same with AppUnits suffix)
  inner -> i
  min -> iMin (same with AppUnits suffix)
  max -> iMax (same with AppUnits suffix)

I'd also note I'm a little disturbed by the computations here being done
in CSS pixels; that seems to imply that the image was sized with image
pixels matching CSS pixels.  I think it would probably be better to size
the image in device pixels; this will be more accurate on high-DPI
displays.

In MinIntervalIndexWithYLargerThan, I think you should rename iStart,
iEnd, and iMid to startIdx, endIdx, midIdx, since iStart and iEnd in
layout code tend to mean "inline" rather than "index".  (See previous
comment on naming.)

>+    if (midY < aTargetY) {

If you change this to (midY <= aTargetY) it will both:
* match the function's description more clearly
* allow you to remove this bit below:
  >+      if (midY == aTargetY) {
  >+        break;
  >+      }
  (which also violates the "greater than" condition)

>+      iStart = iMid;

This should be iStart = iMid + 1.

This is since the code is structured so that iStart and iEnd both might
be the answer (given how they're initialized), but iEnd might not be a
valid index.  Since the test (with the correction above) means that iMid
doesn't satisfy the condition of having a y() value (midY) greater than
aTargetY, so iStart can be iMid + 1.

This, in turn, means that you can remove:
  >+      if (iStart == iMid) {
  >+        break;
  >+      }
since you don't need it to get the loop to terminate.

So, overall, that simplifies the loop (without the renames above) to be:

+  while (iStart < iEnd) {
+    size_t iMid = iStart + (iEnd - iStart) / 2;
+    nscoord midY = mIntervals[iMid].mLineLeft.Y();
+    if (midY <= aTargetY) {
+      iStart = iMid + 1;
+    } else {
+      iEnd = iMid;
+    }
+  }

It also means that if you instead wanted the description of the function
to be "greater than or equal" (and the name changed to match) you could
just change the "<=" to "<".

(It might also be clearer if you flip the then/else branches and invert
the test, though.)

>+  for (size_t i = MinIntervalIndexWithYLargerThan(aBStart);
>+          i < intervalCount;
>+          ++i) {

This should just be two lines, with the i on the second line lined up
with the size_t on the first.

But this also seems like the wrong interval to start considering; you
should be considering an interval that starts before aBStart as long as
it *ends* after aBStart.

>+      auto& interval = mIntervals[i];

strange indentation on this line

>-/* utility functions for drawing borders and backgrounds */
>+/* This file contains utility code for drawing images as CSS borders,
>+ * backgrounds, and shapes. */

Skip the "This file contains ".  These comments are designed to show up
in https://dxr.mozilla.org/mozilla-central/source/layout/painting so
they should be short.
Attachment #8958925 - Flags: review?(dbaron) → review-
Comment on attachment 8958927 [details] [diff] [review]
Bug 1404222 Part 2: Block onload when shape-outside images are first decoded, and keep it blocked until a second reflow is complete.

In nsIDocument.h, use nsPtrHashKey<void> (no "*"), and ditch the (void**)
casts in nsDocument.cpp.

I'm still not happy about IsRequestForShapeOutsideOnFrame (which is now
refactored, but still doing fundamentally the same thing I was unhappy
about in comment 227).  I would much prefer this information be passed
in from the caller.  It's the more normal way of writing this sort of
thing, and it better extends to adding new features that work in similar
ways.  (For example, could the |FrameSet| typedef be an array of structs
that have both a frame and a word of flags, where the first flag is
whether the frame requires reflow?)

>+  // We may need reflow (for example if the image is from shape-outside).
>+  bool reflowRequested = RequestReflowIfNeeded(frameSet, aRequest);
>+  if (reflowRequested) {
>+    // Currently, the only reason we would request reflow is because the
>+    // image is used by shape-outside, which means that we don't want
>+    // to get any more of these events. Deregister with the refresh driver.
>+    nsPresContext* presContext = GetPresContext();
>+    if (presContext) {
>+      nsLayoutUtils::DeregisterImageRequest(presContext,
>+                                            aRequest,
>+                                            nullptr);
>+    }
>+  }

I didn't catch this last time, but...

What happens here if the image is used both as a background-image and
(on a different frame) as a shape-outside?  (This isn't a particularly
farfetched scenario.)  Will this do the wrong thing for the one with the
background-image?  (Consider also the case of somebody *later* using
the same request as a background-image.)  Or does something guarantee
that we never get the same imgIRequest pointer?  If so, you should add
assertions to verify that and explain what would go wrong if the
condition didn't hold.

Speaking of which... what happens when 'shape-outside' refers to an
animated image?  Should the layout change as the image animates?  Does
this approach work for that?

(There should be a test for 'shape-outside' with an animated image, at
the very least to make sure it doesn't crash or assert, but preferably
more.)



Also, I guess I don't see a reason for the BlockOnloadOnResource /
UnblockOnloadOnResource API to be on the document at all.  Why aren't
they just methods on the image loader with the hashtable on the image
loader?  It seems like the more widely they're used, the more likely
things are to get confused.

I'd also prefer it to be based on the image loader's own state rather
than a duplicate set of state in a separate hashtable, actually.  If
whether we're blocked/unblocked could be determined from the contents of
the mRequestToFrameMap (or mFrameToRequestMap, if that makes more sense)
and perhaps a counter, that would seem cleaner and also less likely to
get out of sync.  (See my previous comment about FrameSet being an array
of structs.)

(I guess part of what I'm saying is that I don't know how to verify that
this patch makes the UnblockOnloadOnResource calls in the right places,
and I'm suggesting a way to structure it that I think would be easier to
verify and maintain.)

I guess I should have been clearer about suggesting this particular
approach in the previous review, although I hadn't really come up with
all of it at the time.
Attachment #8958927 - Flags: review?(dbaron) → review-
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #246)
> Comment on attachment 8958927 [details] [diff] [review]
> I'm still not happy about IsRequestForShapeOutsideOnFrame (which is now
> refactored, but still doing fundamentally the same thing I was unhappy
> about in comment 227).  I would much prefer this information be passed
> in from the caller.  It's the more normal way of writing this sort of
> thing, and it better extends to adding new features that work in similar
> ways.  (For example, could the |FrameSet| typedef be an array of structs
> that have both a frame and a word of flags, where the first flag is
> whether the frame requires reflow?)
I see. I understand your proposed solution better, now. I'll implement it as you suggest.

> What happens here if the image is used both as a background-image and
> (on a different frame) as a shape-outside?  (This isn't a particularly
> farfetched scenario.)  Will this do the wrong thing for the one with the
> background-image?  (Consider also the case of somebody *later* using
> the same request as a background-image.)  Or does something guarantee
> that we never get the same imgIRequest pointer?  If so, you should add
> assertions to verify that and explain what would go wrong if the
> condition didn't hold.
> 
> Speaking of which... what happens when 'shape-outside' refers to an
> animated image?  Should the layout change as the image animates?  Does
> this approach work for that?
Ah, that is a problem since this code would prevent the background-image from animating. I'll address that. The de-registering that happens here was intended as an optimization since shape-outside only needs the first frame, per spec. I'll instead change it to only generate the float area on the first frame complete notification, but not actually deregister for future events.

> (There should be a test for 'shape-outside' with an animated image, at
> the very least to make sure it doesn't crash or assert, but preferably
> more.)
This is tested by the wpt test css/css-shapes/shape-outside/shape-image/shape-image-025.html.

> Also, I guess I don't see a reason for the BlockOnloadOnResource /
> UnblockOnloadOnResource API to be on the document at all.  Why aren't
> they just methods on the image loader with the hashtable on the image
> loader?  It seems like the more widely they're used, the more likely
> things are to get confused.
> 
> I'd also prefer it to be based on the image loader's own state rather
> than a duplicate set of state in a separate hashtable, actually.  If
> whether we're blocked/unblocked could be determined from the contents of
> the mRequestToFrameMap (or mFrameToRequestMap, if that makes more sense)
> and perhaps a counter, that would seem cleaner and also less likely to
> get out of sync.  (See my previous comment about FrameSet being an array
> of structs.)
Yes, I agree that the logic can and should live within ImageLoader. I'll move it there. I'll try to find a way to tie it to the internal state instead of using a potentially redundant set of blocking imgIRequest pointers.
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #245)
> Comment on attachment 8958925 [details] [diff] [review] 
> I'd also note I'm a little disturbed by the computations here being done
> in CSS pixels; that seems to imply that the image was sized with image
> pixels matching CSS pixels.  I think it would probably be better to size
> the image in device pixels; this will be more accurate on high-DPI
> displays.
I still stumble on the different pixel definitions and I was unsure how to choose the correct one here. I confess I'm still fuzzy on it. If device pixels have a higher resolution than css pixels, that means the rendering in nsFloatManager::ShapeInfo::CreateImageShape would generate a denser bitmap. Since this algorithm has to process each pixel in that bitmap, I assume we will have to make a judgment call about how dense is too dense. Does that consideration make CSS pixels the right choice here, even if less accurate?
Flags: needinfo?(dbaron)
I tend to think the right thing to do is the full accuracy of what we're rendering -- if that turns out to be a performance problem I think we could consider optimizing it later to a different resolution.
Flags: needinfo?(dbaron)
I guess we might want to choose a higher resolution to generate a denser bitmap even if the CSS pixel and the device pixel has 1:1 mapping to fix bug 1430932.