Closed Bug 846929 Opened 7 years ago Closed 7 years ago

Resetting zoom level on image documents also resets image rotation

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 22

People

(Reporter: reuben, Assigned: waterlo1)

References

Details

Attachments

(1 file, 6 obsolete files)

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.
I forgot step 2.5: Zoom in or out (Cmd+=/Cmd+-)
Assignee: nobody → waterlo1
(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.
Attached patch Proposed patch v 0.1 (obsolete) — Splinter Review
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.
Attachment #722955 - Flags: feedback?(jAwS)
OS: Mac OS X → All
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?
Attachment #722955 - Flags: feedback?(jAwS) → feedback+
Attached patch Proposed Patch v0.2 (obsolete) — Splinter Review
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.
Attachment #722955 - Attachment is obsolete: true
Attachment #723630 - Flags: feedback?(jAwS)
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.
Attached patch Proposed Patch v 0.3 (obsolete) — Splinter Review
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.
Attachment #723630 - Attachment is obsolete: true
Attachment #723630 - Flags: review?(roc)
Attachment #724046 - Flags: feedback?(roc)
Attachment #724046 - Flags: feedback?(jAwS)
(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 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.
Attachment #724046 - Flags: feedback?(jAwS) → feedback+
Attached patch Proposed Patch v 0.4 (obsolete) — Splinter Review
Version incorporating jaws' feedback on previous version.
Attachment #724046 - Attachment is obsolete: true
Attachment #724046 - Flags: feedback?(roc)
Attachment #724089 - Flags: feedback?(jAwS)
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
Attached patch Proposed Patch v 0.5 (obsolete) — Splinter Review
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.
Attachment #724089 - Attachment is obsolete: true
Attachment #724089 - Flags: review?(roc)
Attachment #724950 - Flags: feedback?(jAwS)
Attachment #724950 - Flags: review?(roc)
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
Attached patch Proposed Patch (obsolete) — Splinter Review
Proposed patch including roc's previous comments.
Attachment #724950 - Attachment is obsolete: true
Attachment #724950 - Flags: review?(roc)
Attachment #725490 - Flags: review?(roc)
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 {
Attached patch Proposed PatchSplinter Review
More changes from roc
Attachment #725490 - Attachment is obsolete: true
Attachment #725490 - Flags: review?(roc)
Attachment #725577 - Flags: review?(roc)
Comment on attachment 725577 [details] [diff] [review]
Proposed Patch

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

Hooray!
Attachment #725577 - Flags: review?(roc) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/de2a0438c81b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Depends on: 862117
You need to log in before you can comment on or make changes to this bug.