997010 - dynamic image-orientation changes are ignored

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[Boris Zbarsky [:bzbarsky]]

nsImageFrame only considers mImageOrientation in NotifyNewCurrentRequest and OnStartContainer.  That means that dynamic changes in the property value are ignored until a new load happens.

That seems pretty wrong.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Reftest

Comment 2

[Mats Palmgren (inactive)]

Attachment: wip

Is this the right approach?  (the reftest pass locally on Linux)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8409861 [details] [diff] [review]
wip

You shouldn't need the invalidate in Reflow; just change the hint for that property in nsStyleStruct to include repainting.

Apart from that, the main problem here is that this is updating the intrinsic size when we get to Reflow(), which may be too late.  We might already have computed an intrinsic size for our parent using our incorrect intrinsic size.  So we should be doing similar things in GetPrefWidth()/GetMinWidth(), no?  And perhaps in GetIntrinsicRatio() and ComputeSize().

Comment 4

[Seth Fowler [:seth] [:s2h]]

Mats, if it's alright with you I'm going to jump in here and take this one.

Comment 5

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Support dynamic changes to the image-orientation property

This patch updates the appropriate size information if the value of image-orientation changes, using the DidSetStyleContext hook.

Comment 6

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Reftest for dynamic changes to the image-orientation property

This is Boris's reftest from comment 1. I noticed it had never been reviewed, so let's do that now.

Comment 7

[Seth Fowler [:seth] [:s2h]]

The new imgIContainer::Unwrap function added in Part 1 is needed to remove any existing OrientedImage wrapper. Otherwise we'd end up layer the new orientation on top of the previous one, so if we had an original rotation of 90 degrees and we set a new value of 180 degrees, we'd end up with a 270 degree rotation if we didn't use imgIContainer::Unwrap.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8442633 [details] [diff] [review]
(Part 1) - Support dynamic changes to the image-orientation property

Looks fine, though if you add nostdcall to the idl you won't need the NS_IMETHODIMP_ bits either.

Comment 9

[Seth Fowler [:seth] [:s2h]]

(In reply to Boris Zbarsky [:bz] from comment #8)
> Looks fine, though if you add nostdcall to the idl you won't need the
> NS_IMETHODIMP_ bits either.

Thanks for the quick review!

That sounds like a good change; I'll make it in the final version of the patch.

Comment 10

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8442633 [details] [diff] [review]
(Part 1) - Support dynamic changes to the image-orientation property

Review of attachment 8442633 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsImageFrame.cpp
@@ +230,5 @@
> +
> +  bool shouldUpdateOrientation =
> +    !aContext ||
> +    aContext->StyleVisibility()->mImageOrientation != newOrientation;
> +                            

Argh, just noticed this whitespace (and the other chunk below). This will be removed in the final version of the patch.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8442633 [details] [diff] [review]
(Part 1) - Support dynamic changes to the image-orientation property

> void
>+nsImageFrame::DidSetStyleContext(nsStyleContext* aContext)

Please call this aOldStyleContext rather than aContext.  This is consistent with other implementations of this method, and makes the code make a lot more sense.

>+{
>+  ImageFrameSuper::DidSetStyleContext(aContext);
>+
>+  if (!mImage) {
>+    // We'll pick this change up whenever we do get an image.
>+    return;
>+  }
>+
>+  nsStyleImageOrientation newOrientation = StyleVisibility()->mImageOrientation;
>+
>+  bool shouldUpdateOrientation =
>+    !aContext ||
>+    aContext->StyleVisibility()->mImageOrientation != newOrientation;

No need no null-check aContext.

>+                            
>+  if (shouldUpdateOrientation) {
>+    nsCOMPtr<imgIContainer> image(mImage->Unwrap());
>+    mImage = nsLayoutUtils::OrientImage(image, StyleVisibility()->mImageOrientation);

Reuse newOrientation here, please.  (Or, if you don't, don't bother with the newOrientation variable at all.  But it's probably better to reuse it.)

>+  virtual void DidSetStyleContext(nsStyleContext* aContext) MOZ_OVERRIDE;

aOldStyleContext here too.


r=dbaron with that

Comment 12

[Seth Fowler [:seth] [:s2h]]

Thanks for the review, David! I'll make those changes. (Though as we discussed in person, we do need to null-check aContext. I'll add a comment about why.)

Comment 13

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Support dynamic changes to the image-orientation property

I've addressed the review comments in this version of the patch. If try is green then this is ready to go.

Comment 14

[Seth Fowler [:seth] [:s2h]]

Try job here:

https://tbpl.mozilla.org/?tree=Try&rev=a936097466a4

Comment 15

[Seth Fowler [:seth] [:s2h]]

Try looks good, but unfortunately bug 997604 needs more work. The dependency between these two bugs is fairly artificial, so I could rebase these patches to avoid the issue, but I'll hold off a bit to see if bug 997604 gets ready soon.

Comment 16

[Seth Fowler [:seth] [:s2h]]

I've rearranged the dependencies between this bugs so these patches can go ahead and land.

Comment 17

[Seth Fowler [:seth] [:s2h]]

Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c4efae1bab3
https://hg.mozilla.org/integration/mozilla-inbound/rev/13ac876b0439

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6c4efae1bab3
https://hg.mozilla.org/mozilla-central/rev/13ac876b0439