833511 - Add rotation gesture support to image documents

Status: RESOLVED FIXED

(Firefox :: General, enhancement)

Description

[Brandon Waterloo]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_0) AppleWebKit/537.17 (KHTML, like Gecko) Chrome/24.0.1312.52 Safari/537.17

Steps to reproduce:

I did a two-finger rotation gesture in Mac OS X Firefox, when I had only an image or video open (synthetic documents).


Actual results:

Nothing


Expected results:

The image/video should rotate along with the gesture until the gesture is released, at which point it should smoothly snap to the nearest 90 degree rotation

Comment 1

[Brandon Waterloo]

Attachment: Simple patch using CSS from new file browser-gestures.js

This is a simple patch that uses JavaScript and CSS to apply rotation.  Rotation snaps to nearest 90 degrees in 0.3s at the end of gesture.

THIS PATCH IS INTENDED FOR DEMONSTRATION PURPOSES ONLY, IT IS NOT COMPLETE.

Must attach commands in about:config:
browser.gesture.twist.end = 'cmd_gestureRotateEnd' (new field)
browser.gesture.twist.left = 'cmd_gestureRotateLeft'
browser.gesture.twist.right = 'cmd_gestureRotateRight'
browser.gesture.twist.threshold = 0

In the future:
--Add momentum to gesture so that you can "throw" the image around
--Fix the "instanceof VideoDocument" test, which is broken

Comment 2

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

Comment on attachment 705977 [details] [diff] [review]
Simple patch using CSS from new file browser-gestures.js

Review of attachment 705977 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-gestures.js
@@ +3,5 @@
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +#endif
> + */

You don't need the #ifdef 0 and #endif here. The tiny diskspace win will be offset by the error lines being off by 5 everywhere.

@@ +24,5 @@
> +  reduce: function GestureZoom_reduce(aEvent) {
> +    dump("-->BMW: zoomout delta: " + aEvent.delta + "\n");
> +    //dump("-->BMW: delta = " + aEvent.delta + "\n");
> +    //ZoomManager.reduce();
> +    //this._applySettingToPref();

Please remove the dumps and commented out code here and elsewhere.

@@ +50,5 @@
> +  init: function GestureRotate_init() {
> +  },
> +
> +  destroy: function GestureRotate_destroy() {
> +  },

these functions {init, destroy} can be removed since they are empty.

@@ +56,5 @@
> +  //**************************************************************************//
> +  // Setting Manipulation
> +
> +  twistLeft: function GestureRotate_twistLeft(aEvent) {
> +    if (!content.document.mozSyntheticDocument || (!(content.document instanceof ImageDocument) && !(content.document instanceof VideoDocument)))

nit: line lengths of < 100 preferred, < 80 even better. If you need to wrap a line, keep the binary operator at the end of the line. For example,
if (!content.document.mozSyntheticDocument ||
    (!(content.document instanceof ImageDocument) &&
     !(content.document instanceof VideoDocument)))

With that being said, ImageDocuments and VideoDocuments are synthetic documents, so no need for the double-check here. You can just check if it is an ImageDocument or VideoDocument.

Is this the code that bz asked to be moved to WindowUtils?

@@ +67,5 @@
> +
> +    contentElement.style.transform = "rotate(" + this.rotation + "deg)";
> +  },
> +
> +  twistRight: function GestureRotate_twistRight(aEvent) {

twistRight and twistLeft appear to be the exact same code. I think you can remove one and rename the other to "rotate".

@@ +72,5 @@
> +    if (!content.document.mozSyntheticDocument || (!(content.document instanceof ImageDocument) && !(content.document instanceof VideoDocument)))
> +      return;
> +
> +    let contentElement = this._getContentElement();
> +    if (!contentElement) 

nit, trailing whitespace.

@@ +104,5 @@
> +      transitionRotation = 270;
> +    else
> +      transitionRotation = 360;
> +
> +    contentElement.style.transition = "transform 0.3s ease 0s";

This rule doesn't belong here. It can always be set within the skin CSS, at /toolkit/themes/*stripe/global/media/TopLevel*Document.css. When you add it to the skin CSS, you can make it so it only applies if a 'class' attribute is present. For example:

.completeRotation {
  transition: transform 0.3s ease 0s;
}

and then in this code you would add:
this.classList.add('completeRotation');

@@ +122,5 @@
> +      this.currentRotation += 360;
> +    return this.currentRotation;
> +  },
> +
> +  _clearTransitionRule: function GestureRotate__clearTransitionRule() {

nit: this can just be written as,
_clearTransitionRule: function() {

Starting in Firefox 17 we don't need to name the function if it is directly assigned to a property. The debugger will reference the property name for errors, etc.

@@ +123,5 @@
> +    return this.currentRotation;
> +  },
> +
> +  _clearTransitionRule: function GestureRotate__clearTransitionRule() {
> +    this.style.transition = "";

Since this is now set using a class, you would remove the class here:
this.classList.remove('completeRotation');

@@ +128,5 @@
> +  },
> +
> +  _getContentElement: function GestureRotate__getContentElement() {
> +    if (content.document instanceof ImageDocument)
> +      return content.document.getElementsByTagName("img")[0];

This can be made faster and simpler by just doing,
return content.document.body.firstElementChild;
which should work for both ImageDocument and VideoDocument.

Each call to this function has a previous check for ImageDocument and VideoDocument, so adding those checks here just seems unnecessary.

::: browser/base/content/browser-sets.inc
@@ +82,5 @@
>      <command id="Browser:FocusNextFrame" oncommand="focusNextFrame(event);"/>
>      <command id="cmd_fullZoomReduce"  oncommand="FullZoom.reduce()"/>
>      <command id="cmd_fullZoomEnlarge" oncommand="FullZoom.enlarge()"/>
> +    <command id="cmd_gestureZoomReduce"  oncommand="GestureZoom.reduce(event.sourceEvent)"/>
> +    <command id="cmd_gestureZoomEnlarge" oncommand="GestureZoom.enlarge(event.sourceEvent)"/>

Please keep all the cmd_fullZoom* commands together. You could put these after the cmd_fullZoom* ones.

::: browser/base/content/browser.js
@@ +785,4 @@
>  #ifdef XP_WIN
>          this._setupGesture(aEvent, "pinch", def(25, 0), "out", "in");
>  #else
> +        this._setupGesture(aEvent, "pinch", def(150, 0), "out", "in");

why this change?

Comment 3

[Brandon Waterloo]

Attachment: Simple patch using CSS from new file browser-gestures.js

Simple patch using CSS from new file browser-gestures.js

This is a simple patch that uses JavaScript and CSS to apply rotation.  Rotation snaps to nearest 90 degrees in 0.3s at the end of gesture.

THIS PATCH IS INTENDED FOR DEMONSTRATION PURPOSES ONLY, IT IS NOT COMPLETE.

Must attach commands in about:config:
browser.gesture.twist.end = 'cmd_gestureRotateEnd' (new field)
browser.gesture.twist.left = 'cmd_gestureRotateLeft'
browser.gesture.twist.right = 'cmd_gestureRotateRight'
browser.gesture.twist.threshold = 0

In the future:
--Add momentum to gesture so that you can "throw" the image around
--Video documents can be rotated in this manner, however, the controls rotate as well as the video, which is awkward

Comment 4

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

Comment on attachment 706023 [details] [diff] [review]
Simple patch using CSS from new file browser-gestures.js

Review of attachment 706023 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-gestures.js
@@ +11,5 @@
> +  // Setting Manipulation
> +
> +  zoom: function GestureZoom_zoom(aEvent) {
> +  },
> +};

The GestureZoom is unused and should just be removed from this patch.

@@ +22,5 @@
> +  currentRotation: 0,
> +
> +  // Setting Manipulation
> +
> +  rotate: function GestureRotate_rotate(aEvent) {

nit, please remove these function names "GestureRotate_rotate", as I mentioned in the previous feedback pass.

@@ +30,5 @@
> +    let contentElement = content.document.body.firstElementChild;
> +    if (!contentElement)
> +      return;
> +
> +    this.rotation = Math.round(this.rotation + aEvent.delta);

I think there might be a property of these events that already provides the full rotation amount so you don't have to do this math by hand.

@@ +77,5 @@
> +    return this.currentRotation;
> +  },
> +
> +  _clearTransitionRule: function() {
> +    this.classList.remove("completeRotation");

This should also remove the event listener.
contentElement.removeEventListener("transitionEnd", this._clearTransitionRule, true);

Do you need to pass true as the third argument to these event listener registering functions? See if it works without the third argument.

Comment 5

[Brandon Waterloo]

Attachment: Simple patch using CSS from new file browser-gestures.js

This is a simple patch that uses JavaScript and CSS to apply rotation.  Rotation snaps to nearest 90 degrees in 0.3s at the end of gesture.

THIS PATCH IS INTENDED FOR DEMONSTRATION PURPOSES ONLY, IT IS NOT COMPLETE.

Must attach commands in about:config:
browser.gesture.twist.end = 'cmd_gestureRotateEnd' (new field)
browser.gesture.twist.left = 'cmd_gestureRotateLeft'
browser.gesture.twist.right = 'cmd_gestureRotateRight'
browser.gesture.twist.threshold = 0

In the future:
--Add momentum to gesture so that you can "throw" the image around
--Video documents can be rotated in this manner, however, the controls rotate as well as the video, which is awkward

Comment 6

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

Comment on attachment 706043 [details] [diff] [review]
Simple patch using CSS from new file browser-gestures.js

Review of attachment 706043 [details] [diff] [review]:
-----------------------------------------------------------------

I tested this out and it performs well. Please merge the code in your browser-gestures.js file to the gGestureSupport object in browser.js. Also please set the preference values to be the default values within firefox.js so users won't have to set them themselves.

Comment 7

[Matt Brubeck (:mbrubeck)]

I tried this out on a Windows laptop with a multi-touch screen, it in works very nicely there too.

Comment 8

[Brandon Waterloo]

Attachment: Proposed patch

This is a patch proposed to fix the lack of rotation support.  This particular version of the patch merges code into browser.js, puts the default settings into firefox.js, and also fixes problems relating to tab switching and page reloading.

Also, removed anything relating to the zoom, which is not part of this patch.

Comment 9

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

Comment on attachment 707304 [details] [diff] [review]
Proposed patch

Review of attachment 707304 [details] [diff] [review]:
-----------------------------------------------------------------

Can you please include more context in your diffs? The normal context amount is 4 lines of context on each side of the change.

Comment 10

[Brandon Waterloo]

Attachment: Proposed Patch

This is a patch proposed to fix the lack of rotation support.  This particular version of the patch merges code into browser.js, puts the default settings into firefox.js, and also fixes problems relating to tab switching and page reloading.

Also, removed anything relating to the zoom, which is not part of this patch.

This is a re-upload of 707304, except with 4 lines of context around diffs, per jaws' request.

Comment 11

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

Comment on attachment 707718 [details] [diff] [review]
Proposed Patch

Review of attachment 707718 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +741,5 @@
>  
>  let gGestureSupport = {
> +  currentRotation: 0,
> +  lastDelta: 0,
> +  rotateMomentumThreshold: .75,

These should have an underscore prefix on them to mark them as "private".

Rename lastDelta to lastRotateDelta so it is clear that it is only used by the rotate gesture.

@@ +1038,5 @@
> +      transitionRotation += 90;
> +    else if (this.lastDelta < -1 * this.rotateMomentumThreshold &&
> +             this.rotation < transitionRotation)
> +      transitionRotation -= 90;
> +    

nit: please remove this trailing whitespace here.

This momentum code can get the rotation to be < 0deg or > 360deg. Could the awkward rotation stem from this?

@@ +1071,5 @@
> +  /**
> +   * When the location/tab changes, need to reload the current rotation for the
> +   * image
> +   */
> +  reloadRotation: function() {

reload is a, ahem, loaded term. Maybe "restoreRotationState"?

@@ +1077,5 @@
> +      return;
> +
> +    let contentElement = content.document.body.firstElementChild;
> +    let transformValue = content.window.getComputedStyle(contentElement, null)
> +                                        .getPropertyValue("transform");

let transformValue = content.window.getComputedStyle(contentElement, null).transform;

@@ +1086,5 @@
> +    }
> +
> +    // transformValue is a rotation matrix--split it and do mathemagic to
> +    // obtain the real rotation value
> +    transformValue = transformValue.split("(")[1].split(")")[0].split(",");

To make this a bit easier to read, please place the chained calls on their own line like so:

transformValue = transformValue.split("(")[1]
                               .split(")")[0]
                               .split(",");

@@ +1088,5 @@
> +    // transformValue is a rotation matrix--split it and do mathemagic to
> +    // obtain the real rotation value
> +    transformValue = transformValue.split("(")[1].split(")")[0].split(",");
> +    this.rotation = Math.round(Math.atan2(transformValue[1], transformValue[0]) * 
> +                                          (180/Math.PI));

The alignment here needs to be tweaked. Please remove the trailing whitespace after the asterisk and line up the "(180 / Math.PI)" with the M of Math.atan2. Please also place a space around the division operator.

Comment 12

[Brandon Waterloo]

Attachment: Proposed Patch

This is a patch proposed to fix the lack of rotation support.  This particular version of the patch merges code into browser.js, puts the default settings into firefox.js, and also fixes problems relating to tab switching and page reloading.

Also, removed anything relating to the zoom, which is not part of this patch.

This patch version implements changes per jaws' feedback on previous version.

Comment 13

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

Comment on attachment 708279 [details] [diff] [review]
Proposed Patch

Review of attachment 708279 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. I'll go ahead and check it in for you.

Comment 14

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

I just pushed this to our inbound branch. If all is good it will get merged from our inbound branch to our mozilla-central repository in about a day or two. Once merged, it will show up in our Nightly builds the following day.

https://hg.mozilla.org/integration/mozilla-inbound/rev/73b1860d1fdc

Comment 15

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

The patch was backed out on our inbound branch due to a test failure in browser_gestureSupport.js.

Can you run the test on your machine and fix the failing part of the test? To run the test, you would do the following:

./mach mochitest-browser browser/base/content/test/browser_gestureSupport.js

Backout push: http://hg.mozilla.org/integration/mozilla-inbound/rev/09d868334747

Comment 16

[Brandon Waterloo]

Attachment: Proposed Patch

This version of the patch disables a test with a race condition that caused intermittent failures.

On occasion in browser/base/content/test/browser_gestureSupport.js, creating a MozRotateGestureUpdate event would erroneously cause the received command count to increment, in test step 2 of test_emitLatchedEvents() (at time of writing, lines 294-299).  This would displace the received command count, causing all subsequent tests (steps 2-6) on that gesture to fail until the command count is reset, near the beginning of test_emitLatchedEvents() (at time of writing, line 272).

It isn't known why that erroneous behavior occurred, however, a race condition is strongly suspected because of how intermittent (and rare) the test failure was.  (Rough estimation:  failed 2-5% of the time).

The rotate gesture was never latched, and specifically must *not* be latched for this rotation gesture patch to work.  Additionally, no changes were made to the code responsible for handling latching and thresholds, so the code for bug 833511 should not have affected the test at all; it is suspected this race condition previously existed and hadn't yet been encountered.

It is possible (though seemingly unlikely, based on my interpretation of the logic of latching/thresholds) that changing the threshold to 0 (which was done for this rotation gesture) could have caused the test to fail; however, the test was still not necessary since rotation is not latched.

Comment 17

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

Pushed to try server to make sure that this fixes the intermittent failures:
https://tbpl.mozilla.org/?tree=Try&rev=743156007416

Comment 18

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

Comment on attachment 708768 [details] [diff] [review]
Proposed Patch

The try server push was green (retriggered the test a bunch of times).

Pushed again to inbound, https://hg.mozilla.org/integration/mozilla-inbound/rev/1bc813b9d3a9

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1bc813b9d3a9

Comment 20

[Boris Zbarsky [:bzbarsky]]

Hmm.  So the change the threshold totally mucks with attempts to use twisting gestures for tab-switching...

Should I just switch it to a nonzero value locally, or do we need to do something more complicated in terms of different thresholds for different actions?

Comment 21

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

I'd posit that we could drop support for twist-tab-switching. The gesture isn't intuitive towards switching tabs and hasn't been default-supported for some time (if ever, my hg grep shows that the "browser.gesture.twist.left" line hasn't been changed between this changeset and the CVS->HG merge).

I'd recommend just switching the value locally for your setup. Other users who have got their muscle memory adjusted towards using twist to switch tabs could use an addon or manually adjust prefs to get this functionality back.

Comment 22

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

With that being said, it wouldn't be hard for us to add a migration step that checks for a non-empty twist command and keeps the previous behavior and threshold.