Closed Bug 997010 Opened 7 years ago Closed 7 years ago

dynamic image-orientation changes are ignored

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: bzbarsky, Assigned: seth)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

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.
Attached patch Reftest (obsolete) — Splinter Review
Attached patch wip (obsolete) — Splinter Review
Is this the right approach?  (the reftest pass locally on Linux)
Attachment #8409861 - Flags: review?(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().
Attachment #8409861 - Flags: review?(bzbarsky) → review-
OS: Mac OS X → All
Hardware: x86 → All
Mats, if it's alright with you I'm going to jump in here and take this one.
Assignee: nobody → seth
Depends on: 997604
Attachment #8409861 - Attachment is obsolete: true
Attachment #8407308 - Attachment is obsolete: true
This patch updates the appropriate size information if the value of image-orientation changes, using the DidSetStyleContext hook.
Attachment #8442633 - Flags: review?(dbaron)
This is Boris's reftest from comment 1. I noticed it had never been reviewed, so let's do that now.
Attachment #8442634 - Flags: review?(dbaron)
Attachment #8442633 - Flags: superreview?(bzbarsky)
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 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.
Attachment #8442633 - Flags: superreview?(bzbarsky) → superreview+
(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 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 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
Attachment #8442633 - Flags: review?(dbaron) → review+
Attachment #8442634 - Flags: review?(dbaron) → review+
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.)
I've addressed the review comments in this version of the patch. If try is green then this is ready to go.
Attachment #8442633 - Attachment is obsolete: true
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.
Blocks: 1029285
I've rearranged the dependencies between this bugs so these patches can go ahead and land.
https://hg.mozilla.org/mozilla-central/rev/6c4efae1bab3
https://hg.mozilla.org/mozilla-central/rev/13ac876b0439
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Product: Core → Core Graveyard
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.