Closed
Bug 840584
Opened 12 years ago
Closed 12 years ago
The completeRotation class is not always removed at end of rotation gesture
Categories
(Firefox :: General, defect)
Tracking
()
RESOLVED
FIXED
Firefox 21
People
(Reporter: waterlo1, Assigned: waterlo1)
References
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
The completeRotation class is not always removed from image document elements at the end of the rotation gesture.
If the snap rotation (0, 90, 180, 270) is equal to the current rotation (where you're holding it), the "transition" property does nothing, so the "transitionend" event is never fired, and the class is not removed.
To fix this, if the target rotation and current rotation are equal, don't add the class since nothing needs to be done anyway.
Additionally, since this same CSS class will be used by the MSU capstone team for zoom on image documents, change the class to completeGesture, and let zoom and rotate share that function.
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → waterlo1
Assignee | ||
Comment 1•12 years ago
|
||
This patch changes the completeRotation CSS class to the completeGesture class, which will be shared with zoom. Also, it will only add the class if it is necessary, ensuring that it never gets added but fails to be removed.
Attachment #712974 -
Flags: feedback?(jwein)
Assignee | ||
Updated•12 years ago
|
Status: NEW → ASSIGNED
Comment 2•12 years ago
|
||
Comment on attachment 712974 [details] [diff] [review]
Fix for class error (plus fix for 839625)
Review of attachment 712974 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/base/content/browser.js
@@ +995,5 @@
> if (!contentElement)
> return;
> + // Ensure we're not in process of snapping currently
> + if (contentElement.classList.contains("completeGesture"))
> + return;
This means that the user would have to wait 300ms before seeing the UI respond to their touch events. Touch events need to be instantaneous, so we should cancel the transition if we receive any events while the rotation is completing. To cancel the transition you can call _clearCompleteRotation.
@@ +1104,3 @@
> */
> + _clearCompleteGesture: function() {
> + this.classList.remove("completeGesture");
Keep the name "completeRotation" and "_clearCompleteRotation" for now since we are still a ways off from landing the zoom gestures.
Attachment #712974 -
Flags: feedback?(jwein) → feedback+
Assignee | ||
Comment 3•12 years ago
|
||
This version of the patch incorporates jaws' feedback on previous version and attempts to get the correct patch metadata in. If it doesn't work, please contact me.
Attachment #712974 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #713081 -
Flags: review?(jwein)
Comment 4•12 years ago
|
||
Comment on attachment 713081 [details] [diff] [review]
Incorporates jaws' feedback and adds patch metadata
Review of attachment 713081 [details] [diff] [review]:
-----------------------------------------------------------------
I was able to apply your patch and it kept the author and subject. Are you sure you want to use MSU Capstone as your name? You can have multiple names as the author.
::: browser/base/content/browser.js
@@ +1103,5 @@
> * Removes the transition rule by removing the completeRotation class
> */
> _clearCompleteRotation: function() {
> + let contentElement = content.document.body.firstElementChild;
> + if (!contentElement)
Change this to:
> let contentElement = content.document &&
> content.document instanceof ImageDocument &&
> content.document.body &&
> content.document.body.firstElementChild;
> if (!contentElement)
> return;
Attachment #713081 -
Flags: review?(jwein) → feedback+
Assignee | ||
Comment 5•12 years ago
|
||
This patch fixes the author in the header and also includes the changes jaws' suggested.
Attachment #713081 -
Attachment is obsolete: true
Attachment #713641 -
Flags: feedback?(jAwS)
Comment 6•12 years ago
|
||
Comment on attachment 713641 [details] [diff] [review]
Patch including jaws' feedback
Review of attachment 713641 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/base/content/browser.js
@@ +1014,5 @@
> if (!contentElement)
> return;
> + // If we're currently snapping, cancel that snap
> + if (contentElement.classList.contains("completeRotation"))
> + this._clearCompleteRotation();
Is it possible to reach rotateEnd while we are currently snapping and have this class too? In other words, does the fix for rotate() make these three lines unnecessary?
Attachment #713641 -
Flags: feedback?(jAwS) → feedback+
Assignee | ||
Comment 7•12 years ago
|
||
This version includes jaws' feedback, removing the lines responsible for cancelling a rotation while in the rotateEnd() function, because the rotation will always have been cancelled already in the rotate() function.
Attachment #713641 -
Attachment is obsolete: true
Attachment #713937 -
Flags: feedback?(jAwS)
Comment 8•12 years ago
|
||
Comment on attachment 713937 [details] [diff] [review]
New patch including jaws' feedback
Review of attachment 713937 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good. The patches for bug 678392 are moving this code out to a browser-gestureSupport.js file. I've emailed Stephen to see when he plans on landing that code so we can coordinate the landing of these patches.
Attachment #713937 -
Flags: feedback?(jAwS) → review+
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 9•12 years ago
|
||
Keywords: checkin-needed
Comment 10•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
You need to log in
before you can comment on or make changes to this bug.
Description
•