Closed
Bug 846929
Opened 12 years ago
Closed 12 years ago
Resetting zoom level on image documents also resets image rotation
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 22
People
(Reporter: reuben, Assigned: waterlo1)
References
Details
Attachments
(1 file, 6 obsolete files)
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
I forgot step 2.5: Zoom in or out (Cmd+=/Cmd+-)
Assignee | ||
Comment 2•12 years ago
|
||
Found the source of the issue:
http://mxr.mozilla.org/mozilla-central/source/content/html/document/src/ImageDocument.cpp#476
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → waterlo1
Assignee | ||
Comment 3•12 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•12 years ago
|
||
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•12 years ago
|
OS: Mac OS X → All
Comment 5•12 years ago
|
||
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•12 years ago
|
||
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)
Updated•12 years ago
|
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•12 years ago
|
||
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•12 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 10•12 years ago
|
||
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•12 years ago
|
||
Version incorporating jaws' feedback on previous version.
Attachment #724046 -
Attachment is obsolete: true
Attachment #724046 -
Flags: feedback?(roc)
Attachment #724089 -
Flags: feedback?(jAwS)
Updated•12 years ago
|
Attachment #724089 -
Flags: review?(roc)
Attachment #724089 -
Flags: feedback?(jAwS)
Attachment #724089 -
Flags: feedback+
Updated•12 years ago
|
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•12 years ago
|
||
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•12 years ago
|
Attachment #724950 -
Flags: review?(roc)
Updated•12 years ago
|
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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
Keywords: checkin-needed
Comment 19•12 years ago
|
||
Keywords: checkin-needed
Comment 20•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
You need to log in
before you can comment on or make changes to this bug.
Description
•