840584 - The completeRotation class is not always removed at end of rotation gesture

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Brandon Waterloo]

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]

Attachment: 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

[Jared Wein [:jaws] (please needinfo? me)]

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]

Attachment: 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

[Jared Wein [:jaws] (please needinfo? me)]

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]

Attachment: Patch including jaws' feedback

This patch fixes the author in the header and also includes the changes jaws' suggested.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

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]

Attachment: 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

[Jared Wein [:jaws] (please needinfo? me)]

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]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bad7ced53bef

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bad7ced53bef