If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Resetting zoom level on image documents also resets image rotation

RESOLVED FIXED in Firefox 22

Status

()

Firefox
General
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: reuben, Assigned: Brandon Waterloo)

Tracking

Trunk
Firefox 22
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

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

Comment 2

5 years ago
Found the source of the issue:
http://mxr.mozilla.org/mozilla-central/source/content/html/document/src/ImageDocument.cpp#476
(Assignee)

Updated

5 years ago
Assignee: nobody → waterlo1
(Assignee)

Comment 3

5 years ago
(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.
(Assignee)

Comment 4

5 years ago
Created attachment 722955 [details] [diff] [review]
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.
Attachment #722955 - Flags: feedback?(jAwS)
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 6

5 years ago
Created attachment 723630 [details] [diff] [review]
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.
Attachment #722955 - Attachment is obsolete: true
Attachment #723630 - Flags: feedback?(jAwS)
Attachment #723630 - Flags: review?(roc)
Attachment #723630 - Flags: feedback?(jAwS)
Attachment #723630 - Flags: feedback+
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.
(Assignee)

Comment 8

5 years ago
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.
Attachment #723630 - Attachment is obsolete: true
Attachment #723630 - Flags: review?(roc)
Attachment #724046 - Flags: feedback?(roc)
Attachment #724046 - Flags: feedback?(jAwS)
(Assignee)

Comment 9

5 years ago
(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+
(Assignee)

Comment 11

5 years ago
Created attachment 724089 [details] [diff] [review]
Proposed Patch v 0.4

Version incorporating jaws' feedback on previous version.
Attachment #724046 - Attachment is obsolete: true
Attachment #724046 - Flags: feedback?(roc)
Attachment #724089 - Flags: feedback?(jAwS)
Attachment #724089 - Flags: review?(roc)
Attachment #724089 - Flags: feedback?(jAwS)
Attachment #724089 - Flags: feedback+
Status: NEW → ASSIGNED
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
(Assignee)

Comment 13

5 years ago
Created attachment 724950 [details] [diff] [review]
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.
Attachment #724089 - Attachment is obsolete: true
Attachment #724089 - Flags: review?(roc)
Attachment #724950 - Flags: feedback?(jAwS)
(Assignee)

Updated

5 years ago
Attachment #724950 - Flags: review?(roc)
Attachment #724950 - Flags: feedback?(jAwS)
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
(Assignee)

Comment 15

5 years ago
Created attachment 725490 [details] [diff] [review]
Proposed Patch

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 {
(Assignee)

Comment 17

5 years ago
Created attachment 725577 [details] [diff] [review]
Proposed Patch

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+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/de2a0438c81b
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/de2a0438c81b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22

Updated

5 years ago
Depends on: 862117
You need to log in before you can comment on or make changes to this bug.