Last Comment Bug 840584 - The completeRotation class is not always removed at end of rotation gesture
: The completeRotation class is not always removed at end of rotation gesture
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: 21 Branch
: All All
: -- minor (vote)
: Firefox 21
Assigned To: Brandon Waterloo
:
Mentors:
Depends on:
Blocks: 833511
  Show dependency treegraph
 
Reported: 2013-02-12 09:42 PST by Brandon Waterloo
Modified: 2013-02-24 06:18 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix for class error (plus fix for 839625) (4.77 KB, patch)
2013-02-12 10:07 PST, Brandon Waterloo
jaws: feedback+
Details | Diff | Review
Incorporates jaws' feedback and adds patch metadata (4.02 KB, patch)
2013-02-12 13:24 PST, Brandon Waterloo
jaws: feedback+
Details | Diff | Review
Patch including jaws' feedback (4.19 KB, patch)
2013-02-13 14:11 PST, Brandon Waterloo
jaws: feedback+
Details | Diff | Review
New patch including jaws' feedback (3.54 KB, patch)
2013-02-14 09:12 PST, Brandon Waterloo
jaws: review+
Details | Diff | Review

Description Brandon Waterloo 2013-02-12 09:42:21 PST
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.
Comment 1 Brandon Waterloo 2013-02-12 10:07:47 PST
Created attachment 712974 [details] [diff] [review]
Fix for class error (plus fix for 839625)

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.
Comment 2 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2013-02-12 11:25:40 PST
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.
Comment 3 Brandon Waterloo 2013-02-12 13:24:31 PST
Created attachment 713081 [details] [diff] [review]
Incorporates jaws' feedback and adds patch metadata

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.
Comment 4 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2013-02-12 13:53:10 PST
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;
Comment 5 Brandon Waterloo 2013-02-13 14:11:53 PST
Created attachment 713641 [details] [diff] [review]
Patch including jaws' feedback

This patch fixes the author in the header and also includes the changes jaws' suggested.
Comment 6 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2013-02-14 08:44:59 PST
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?
Comment 7 Brandon Waterloo 2013-02-14 09:12:43 PST
Created attachment 713937 [details] [diff] [review]
New patch including jaws' feedback

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.
Comment 8 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2013-02-14 09:25:15 PST
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.
Comment 9 Ryan VanderMeulen [:RyanVM] 2013-02-14 10:59:41 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/bad7ced53bef
Comment 10 Ryan VanderMeulen [:RyanVM] 2013-02-15 06:54:40 PST
https://hg.mozilla.org/mozilla-central/rev/bad7ced53bef

Note You need to log in before you can comment on or make changes to this bug.