846929 - Resetting zoom level on image documents also resets image rotation

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Reuben Morais [:reuben]]

STR:

1) Open https://s3.amazonaws.com/f.cl.ly/items/2V0L2P052v1V0J0D1R11/IMG_20130301_134335.jpg (or any image that is larger than the viewport).
2) Rotate using gesture (bug 833511)
3) Reset zoom level (View>Zoom>Reset or Cmd+0)

The image rotation is reset. It should be preserved.

Comment 1

[Reuben Morais [:reuben]]

I forgot step 2.5: Zoom in or out (Cmd+=/Cmd+-)

Comment 2

[Brandon Waterloo]

Found the source of the issue:
http://mxr.mozilla.org/mozilla-central/source/content/html/document/src/ImageDocument.cpp#476

Comment 3

[Brandon Waterloo]

(In reply to Reuben Morais [:reuben] from comment #0)
> STR:
> 
> 1) Open
> https://s3.amazonaws.com/f.cl.ly/items/2V0L2P052v1V0J0D1R11/
> IMG_20130301_134335.jpg (or any image that is larger than the viewport).
> 2) Rotate using gesture (bug 833511)
> 3) Reset zoom level (View>Zoom>Reset or Cmd+0)
> 
> The image rotation is reset. It should be preserved.

Additionally, image rotation will be reset when you toggle zoom in/out (click with the magnifying glass cursor), due to the same function linked in comment 2.

Comment 4

[Brandon Waterloo]

Attachment: Proposed patch v 0.1

Proposed patch.  Instead of removing the style attribute to clear the CSS style used to make the cursor magnify in/out, just remove that CSS property.  When the entire attribute was cleared, it broke rotation.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 722955 [details] [diff] [review]
Proposed patch v 0.1

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

::: content/html/document/src/ImageDocument.cpp
@@ +11,5 @@
>  #include "nsGenericHTMLElement.h"
>  #include "nsIDocumentInlines.h"
>  #include "nsIDOMHTMLImageElement.h"
> +#include "nsIDOMElementCSSInlineStyle.h"
> +#include "nsIDOMCSSStyleDeclaration.h"

These could be put in alphabetical order here (above nsIDOMHTMLImageElement.h).

@@ +419,5 @@
> +
> +  nsCOMPtr<nsIDOMElementCSSInlineStyle> styles = do_QueryInterface(mImageContent);
> +  nsCOMPtr<nsIDOMCSSStyleDeclaration> style;
> +  nsAutoString priority;
> +  styles->GetStyle(getter_AddRefs(style));

GetStyle returns an nsresult that should be checked for success.
> nsresult rv = styles->GetStyle(getter_AddRefs(style));
> NS_ENSURE_SUCCESS(rv, rv);

@@ +481,5 @@
>    nsCOMPtr<nsIContent> imageContent = mImageContent;
> +
> +  nsCOMPtr<nsIDOMElementCSSInlineStyle> styles = do_QueryInterface(mImageContent);
> +  nsCOMPtr<nsIDOMCSSStyleDeclaration> style;
> +  nsAutoString returnString;

What is the purpose of returnString here? Shouldn't that be the priority?

Comment 6

[Brandon Waterloo]

Attachment: Proposed Patch v0.2

This version of the patch incorporates jaws' feedback on the last patch, plus a few extra NS_ENSURE_SUCCESS() checks that were not explicitly mentioned.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 723630 [details] [diff] [review]
Proposed Patch v0.2

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

::: content/html/document/src/ImageDocument.cpp
@@ +434,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  rv = style->SetProperty(NS_LITERAL_STRING("cursor"),
> +                          NS_LITERAL_STRING("-moz-zoom-in"),
> +                          priority);
> +  NS_ENSURE_SUCCESS(rv, rv);

I think it would be a lot simpler to add a rule to TopLevelImageDocument.css like
.shrinkToFit { cursor:-moz-zoom-in; }
and the add/remove that class from the nsIContent to set the style.

Comment 8

[Brandon Waterloo]

Attachment: Proposed Patch v 0.3

This method uses Roc's suggestion to use classes instead of direct styles.

Note:  I had to change some other code in the file (around line 520) pertaining to the "decoded" class, because it was overwriting the "expand" class on the initial loading of the image, which resulted in no zoom-in cursor.

Comment 9

[Brandon Waterloo]

(In reply to Brandon Waterloo from comment #8)
> Created attachment 724046 [details] [diff] [review]
> Proposed Patch v 0.3
> 
> This method uses Roc's suggestion to use classes instead of direct styles.
> 
> Note:  I had to change some other code in the file (around line 520)
> pertaining to the "decoded" class, because it was overwriting the "expand"
> class on the initial loading of the image, which resulted in no zoom-in
> cursor.

Whoops, forgot to add the NS_ENSURE_SUCCESS calls in those other parts (relating to the "decoded" class).  Next version of the patch will have that.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 724046 [details] [diff] [review]
Proposed Patch v 0.3

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

::: content/html/document/src/ImageDocument.cpp
@@ -411,5 @@
>      return NS_OK;
>    }
>  
>    // Keep image content alive while changing the attributes.
> -  nsCOMPtr<nsIContent> imageContent = mImageContent;

It doesn't look like this comment above this line is relevant anymore. Does this mean that a reference will not be added and the image content could disappear while this function is running?

@@ +421,5 @@
>    // origin now that we're showing a shrunk-to-window version.
>    (void) ScrollImageTo(0, 0, false);
>  
> +  mozilla::ErrorResult rv;
> +  mImageContent->AsElement()->GetClassList()->Remove(NS_LITERAL_STRING("shrinkToFit"), rv);

Is it possible to create a local reference to the classList so you don't have to do the imageContent->AsElement->GetClassList twice? And afaik, when "get" is used as a prefix the function may return null.

@@ +476,5 @@
>    imageContent->UnsetAttr(kNameSpaceID_None, nsGkAtoms::width, true);
>    imageContent->UnsetAttr(kNameSpaceID_None, nsGkAtoms::height, true);
>    
>    if (mImageIsOverflowing) {
> +    mozilla::ErrorResult rv;

The ErrorResult declaration can be moved outside of the if/else block since it is used in both of them.

@@ +528,5 @@
>      if (mImageContent) {
>        // Update the background-color of the image only after the
>        // image has been decoded to prevent flashes of just the
>        // background-color.
> +      mozilla::ErrorResult rv;

ditto.

Comment 11

[Brandon Waterloo]

Attachment: Proposed Patch v 0.4

Version incorporating jaws' feedback on previous version.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 724089 [details] [diff] [review]
Proposed Patch v 0.4

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

::: content/html/document/src/ImageDocument.cpp
@@ +427,5 @@
> +  if (classList) {
> +    classList->Remove(NS_LITERAL_STRING("shrinkToFit"), rv);
> +    NS_ENSURE_SUCCESS(rv.ErrorCode(), rv.ErrorCode());
> +    classList->Add(NS_LITERAL_STRING("expand"), rv);
> +    NS_ENSURE_SUCCESS(rv.ErrorCode(), rv.ErrorCode());

This is confusing --- removing the "shrinkToFit" class when we're in shrink-to-fit mode isn't intuitive.

I think "shrinkToFit" should mean that we're in shrinkToFit mode, so here we'd be adding it, not removing it. The other class would then be, say, "overflowing".

Also, create a helper method SetModeClass that takes an enum with values SHRINK_TO_FIT, OVERFLOWING, NONE or something like that and call that from various places to take care of removing existing tokens and adding new ones.

@@ +531,5 @@
>    }
>  
> +  nsDOMTokenList* classList = mImageContent->AsElement()->GetClassList();
> +  mozilla::ErrorResult rv;
> +  if (classList) {

no need to check classList for null

Comment 13

[Brandon Waterloo]

Attachment: Proposed Patch v 0.5

Version incorporating roc's suggestions.  A new helper function and enum are made (local to the class definition in that file) to switch the cursor mode.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 724950 [details] [diff] [review]
Proposed Patch v 0.5

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

::: content/html/document/src/ImageDocument.cpp
@@ +121,5 @@
>    float GetZoomLevel();
>  
> +  enum eModeClasses {
> +    eNone,
> +    eShrink_To_Fit,

Don't mix camelCase with underscores. So just use eShrinkToFit.

@@ +124,5 @@
> +    eNone,
> +    eShrink_To_Fit,
> +    eOverflowing
> +  };
> +  nsresult SetModeClass(eModeClasses mode);

No need to return a result. We won't be able to do anything useful with it.

@@ +576,5 @@
> +      NS_ENSURE_SUCCESS(rv.ErrorCode(), rv.ErrorCode());
> +      classList->Add(NS_LITERAL_STRING("overflowing"), rv);
> +      NS_ENSURE_SUCCESS(rv.ErrorCode(), rv.ErrorCode());
> +      break;
> +  }

You can simplify this code a bit, like this:
if (mode == eShrinkToFit) {
  classList->Add("shrinkToFit")
} else {
  classList->Remove("shrinkToFit")
}
if (mode == eOverflowing) {
  classList->Add("overflowing")
} else {
  classList->Remove("overflowing")
}

etc

Comment 15

[Brandon Waterloo]

Attachment: Proposed Patch

Proposed patch including roc's previous comments.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 725490 [details] [diff] [review]
Proposed Patch

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

Almost there :-)

::: content/html/document/src/ImageDocument.cpp
@@ +560,5 @@
> +
> +  if (mode == eShrinkToFit) {
> +    classList->Add(NS_LITERAL_STRING("shrinkToFit"), rv);
> +    if (rv.Failed())
> +      return;

I wouldn't even bother checking these and returning early. That won't be useful.

@@ +562,5 @@
> +    classList->Add(NS_LITERAL_STRING("shrinkToFit"), rv);
> +    if (rv.Failed())
> +      return;
> +  }
> +  else {

} else {

Comment 17

[Brandon Waterloo]

Attachment: Proposed Patch

More changes from roc

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 725577 [details] [diff] [review]
Proposed Patch

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

Hooray!

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/de2a0438c81b

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/de2a0438c81b