Work - Implement gesture to select tiles

RESOLVED FIXED

Status

Firefox for Metro
Input
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: sfoster, Assigned: sfoster)

Tracking

Trunk
x86
Windows 8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: feature=work)

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

5 years ago
To select tiles (e.g. a bookmark or download) we have proposed a short swipe gesture. This is known as a "cross-swipe" and documented in MS' guidelines here: 

http://msdn.microsoft.com/en-us/library/windows/apps/hh465299.aspx
(Assignee)

Updated

5 years ago
Blocks: 800996

Comment 1

5 years ago
Tim, would you mind looking into this to see if it's something we can do in the widget backend with a GestureRecognizer?
On our GestureRecognizer we will set the CrossSlide GestureSetting [1] and choose appropriate thresholds [2].  When a cross slide is detected, our GestureRecognizer will send us a CrossSliding event with informative args [3].  I'm not sure if CrossSlide will interfere with our other gestures (most notably swipe), so I'll have to investigate this.

We'll have to figure out how we want to communicate the cross slide status to content.  Perhaps we could create new gecko events MozCrossSlideStarted, MozCrossSlideDragging, MozCrossSlideSelecting, MozCrossSlideCompleted?  It sounds like we don't yet want to support rearrange, but that's an option also [4].


[1] http://msdn.microsoft.com/en-us/library/windows/apps/windows.ui.input.gesturesettings.aspx
[2] http://msdn.microsoft.com/en-us/library/windows/apps/windows.ui.input.crossslidethresholds.aspx
[3] http://msdn.microsoft.com/en-us/library/windows/apps/windows.ui.input.crossslidingeventargs.aspx
[4] http://msdn.microsoft.com/en-us/library/windows/apps/windows.ui.input.crossslidingstate.aspx
Blocks: 831915

Updated

5 years ago
OS: Windows 8 → Windows 8 Metro
Summary: Implement gesture to select tiles → Work - Implement gesture to select tiles
Whiteboard: feature=work

Updated

5 years ago
No longer blocks: 800996
Created attachment 708301 [details] [diff] [review]
Patch v1 (WIP)

mbubeck, fryn, sfoster, and I discussed tile selection yesterday.  We have two options when it comes to recognizing the "CrossSlide" or tile-selection gesture:

  1) We can do the gesture recognition in metro widget code.  The advantage here is that Windows' GestureRecognizer class will do the recognition for us, we simply forward information to front-end code.  Unfortunately, even if we do the recognition in widget, we have to specify our own thresholds for how far the user's finger has to move for the gesture to be considered a "rearrange" versus a "select tile".  It shouldn't be too hard to find values that match the Windows Start Screen.

  2) We can do gesture recognition in front-end code.  The front-end code would examine touchstart, touchmove, and touchend events and decide whether a tile-selection gesture (CrossSlide) is occurring.  The advantage here would be that we can do tile-selection in Firefox for Windows desktop if we choose to in the future.

If we go with option 1 (recognize the CrossSlide in metro widget code), the patch would look like the attached.  This patch also removes our swipe, magnify, and rotate support, which I'm in favor of doing regardless of the gesture recognition approach we choose for this bug.
Assignee: nobody → tabraldes
Status: NEW → ASSIGNED
Attachment #708301 - Flags: feedback?(sfoster)
Attachment #708301 - Flags: feedback?(mbrubeck)
Attachment #708301 - Flags: feedback?(fyan)
Comment on attachment 708301 [details] [diff] [review]
Patch v1 (WIP)

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

(In reply to Tim Abraldes (:tabraldes) from comment #2)
> On our GestureRecognizer we will set the CrossSlide GestureSetting [1] and
> choose appropriate thresholds [2].

According to the GestureRecognizer.CrossSliding docs, "The gesture must occur in a direction that is perpendicular to this panning axis."  How does Windows know which direction is our panning axis, since that depends on the event's target and its container within the Gecko DOM?
http://msdn.microsoft.com/en-us/library/windows/apps/windows.ui.input.gesturerecognizer.crosssliding.aspx

> We'll have to figure out how we want to communicate the cross slide status
> to content.  Perhaps we could create new gecko events MozCrossSlideStarted,
> MozCrossSlideDragging, MozCrossSlideSelecting, MozCrossSlideCompleted?

That sounds reasonable.  Maybe we could make these nsIDOMSimpleGestureEvents, with "direction" and "delta" used to communicate information about the drag distance?

::: widget/windows/winrt/MetroInput.cpp
@@ +1017,3 @@
>    mGestureRecognizer->remove_Tapped(mTokenTapped);
>    mGestureRecognizer->remove_RightTapped(mTokenRightTapped);
> +  mGestureRecognizer->remove_RightTapped(mTokenCrossSliding);

Should this be remove_CrossSliding?
Attachment #708301 - Flags: feedback?(mbrubeck)
Comment on attachment 708301 [details] [diff] [review]
Patch v1 (WIP)

Per discussion in IRC, we're going to take a crack at the JS approach to solving this issue. For this patch to work, we'd need to create a new DOM event that corresponds to nsCrossSlideEvent.  Another option is to piggyback off of nsSimpleGestureEvent.  Either way, this is probably more complicated than simply using JS to implement the tile selection, so we'll try that out first.
Attachment #708301 - Attachment is obsolete: true
Attachment #708301 - Flags: feedback?(sfoster)
Attachment #708301 - Flags: feedback?(fyan)
Assignee: tabraldes → nobody
Status: ASSIGNED → NEW

Comment 6

5 years ago
Is there a standard algorithm for figuring out whether a user is trying to move a finger across a surface strictly in one axis versus obliquely? Is that something we need to differentiate here, or is a simple vertical threshold enough? I'll take a look at the native behavior.
(In reply to Frank Yan (:fryn) from comment #6)
> Is there a standard algorithm for figuring out whether a user is trying to
> move a finger across a surface strictly in one axis versus obliquely?

There's a heuristic for this in our panning code, to decide whether to pan on just one axis:
http://hg.mozilla.org/projects/elm/file/231dec9bb2aa/browser/metro/base/content/input.js#l584

Basically, if you are within some threshold of either axis, we treat your movement as a gesture on that axis only, and ignore movement on the other axis.  Until you pass a certain distance from your starting point you can "break" the lock if you change direction; after you pass that threshold you are locked for good.

I'd do something similar for a vertical cross-swipe gesture.  On the touchstart event we will record the starting point (x0, y0) and start listening to touchmove events.  For each touchmove, if (abs(x - x0) > ui.dragThresholdX) then stop listening and cancel the gesture.  But if (abs(y-y0) > CROSS_SWIPE_THRESHOLD) then mark the gesture "completed" and select the tile on the touchend event.

(And of course if we support horizontal cross-swipe it will be the same but with the axes swapped.)

Updated

5 years ago
Blocks: 831918

Updated

5 years ago
Component: General → Input
Version: unspecified → Trunk
(Assignee)

Comment 8

5 years ago
Taking a crack at this. I'll prototype as far as I can get in some seperate HTML file and then I'll need to consult on integration with our existing TouchModule code.
Assignee: nobody → sfoster
(Assignee)

Comment 9

5 years ago
Created attachment 728345 [details]
Initial prototype of cross-slide gesture

First pass at implementing cross-slide, just in a straight HTML file. 
I didn't implement checking the slide ends within the 90deg. cone (and probably other things too). Thresholds are lifted straight from MSDN docs.
(Assignee)

Comment 10

5 years ago
This is WIP, I've not figured out how to handle the cross-sliding states yet. Or how this will integrate with our existing TouchGesture work.
(Assignee)

Comment 11

5 years ago
Created attachment 729025 [details]
Cross-slide module prototype, using global CrossSlide and a CrossSlideHandler, MockGrid

I've restructured this so we have a CrossSlide global with initListeners and uninitListeners methods (placeholder names) which accept a richgrid or similar element with children that can be selected.

See the MockGrid for example usage: MozCrossSliding is the fine-grained events with crossSlidingState property, MozCrossSlideSelect is the event if all you are interested in is whether a gesture indicating item selection was completed. 

I've also implemented the 90deg. cone boundary here for constrainig cross-sliding. When touches fall outside the boundaries or above the REARRANGING threshold, then what? I'm firing crosssliding with crossSlidingState="cancelled" for now. 
 
This is a straw man implementation, up for debate in all its details
Attachment #728345 - Attachment is obsolete: true
Attachment #729025 - Flags: feedback?(mbrubeck)
Attachment #729025 - Flags: feedback?(jmathies)

Updated

5 years ago
Attachment #729025 - Attachment is patch: true
Attachment #729025 - Attachment mime type: text/html → text/plain

Updated

5 years ago
Attachment #729025 - Attachment is patch: false

Updated

5 years ago
Attachment #729025 - Attachment mime type: text/plain → text/html
Comment on attachment 729025 [details]
Cross-slide module prototype, using global CrossSlide and a CrossSlideHandler, MockGrid

This basic approach seems fine to me; see below for a grab-bag of things I noticed.


>      let maxRadians = 45 * (Math.PI/180);

This should just be "let maxRadians = Math.PI / 180;", right?  The "45" here makes this into maxDegrees, and is causing every drag to pass this test.

Note: We might want to ignore the maxRadians check until the touch point has moved more than a few pixels -- otherwise a 1-pixel jitter near the start of the drag can generate spurious "cancelled" messages.


>        if (newState < 0) {
>          this._fireProgressEvent("cancelled", aEvent, this.node);

Shouldn't we stop listening after sending the "cancelled" event?  Otherwise as the drag moves in and out of the 90-degree cone it can get repeatedly "cancelled" and "un-cancelled".

We should maybe limit transitions between other states, too.  For example, if we go into REARRANGING, I think the cross-slide gesture should end (and drag-and-drop code should take over, if the target supports it) -- we shouldn't allow a return from REARRANGING to any previous states.


>      this._fireProgressEvent("completed", aEvent, this.node);
>      this._fireSelectEvent(aEvent, this.node);

Don't we check the state/threshold before firing these events?

I'm not sure we need two events here; just the "completed" event could be enough.


>      // stash the item/tile node that was drug as the relatedTarget(originalTarget, someOtherTarget?)
>      event.relatedTarget = aEvent.target;

Rather than use relatedTarget or similar, let's just dispatch the event directly to the child item/tile.  It'll bubble up to the parent.


>      event.position = this.drag.position;

We'll also need to know the sign/direction, for drag animation purposes.


>  global.CrossSlide = {
>    _listenerHandlesById: {},
>    initListeners: function(aNode, aHandler, aOptions){
>      let id = aNode.id || aNode.getAttribute("id");
>      let internalHandler = new CrossSlideHandler(aNode, aOptions);
>      aNode.addEventListener("touchstart", internalHandler, false),
>      aNode.addEventListener("touchmove", internalHandler, false),
>      aNode.addEventListener("touchend", internalHandler, false)
>      this._listenerHandlesById[id] = [aNode, internalHandler];
>    },
>    uninitListeners: function(aNode) {
>      let id = aNode.id || aNode.getAttribute("id");
>      let [node, handler] = this._listenerHandlesById[id];
>      if (node) {
>        node.removeEventListener("touchstart", handler);
>        node.removeEventListener("touchmove", handler);
>        node.removeEventListener("touchend", handler);
>      }
>      handler = this._listenerHandlesById[id] = null;
>      delete this._listenerHandlesById[id];
>    }

Just a thought:  If we move this code into the richgrid and/or richgriditem bindings, then we can get rid of the global listener map and all of this manual unbinding code.

On the other hand, keeping this outside of the binding gives us a nice separation of concerns.  I guess that's a worthwhile goal as long as it doesn't add too much boilerplate.

Can we remove the _listenerHandlesById map and the uninit method, and just let the cycle collector destroy our listener if the grid gets removed?  (Especially since we don't plan to frequently add or remove grids...)


>      scrollX = (cs.overflowX=='scroll' || cs.overflowX=='auto');
>      scrollY = (cs.overflowX=='scroll' || cs.overflowX=='auto');
>      if (scrollX || scrollY) {

Maybe we should throw an exception if (scrollX && scrollY).


>        if (pDist <= 0) {
>          return CrossSlidingState.STARTED;
>        }
>        if (pDist < CrossSlideThresholds.SELECTIONSTART) {
>          return CrossSlidingState.DRAGGING;
>        }
>        if (pDist < CrossSlideThresholds.SPEEDBUMPSTART) {
>          return CrossSlidingState.SELECTING;
>        }
>        if (pDist < CrossSlideThresholds.SPEEDBUMPEND) {
>          return CrossSlidingState.SELECT_SPEED_BUMPING;
>        }
>        if (pDist < CrossSlideThresholds.REARRANGING) {
>          return CrossSlidingState.SPEED_BUMPING;
>        }
>      }
>      // out of bounds cross-slide
>      return -1;

I'm confused about this threshold -> state mapping.  

CrossSlideThresholds.REARRANGING is not defined - should be REARRANGESTART?

Should there be a "return CrossSlidingState.REARRANGING" and a "return CrossSlidingState.COMPLETED" somewhere?

Since some targets won't support rearranging, we'll eventually need a way to customise thresholds for each grid.
Attachment #729025 - Flags: feedback?(mbrubeck) → feedback+
(In reply to Matt Brubeck (:mbrubeck) from comment #12)
> >      let maxRadians = 45 * (Math.PI/180);
> 
> This should just be "let maxRadians = Math.PI / 180;", right?  The "45" here
> makes this into maxDegrees, and is causing every drag to pass this test.

Please ignore this, I got confused.  The code is correct as-is.

Comment 14

5 years ago
Comment on attachment 729025 [details]
Cross-slide module prototype, using global CrossSlide and a CrossSlideHandler, MockGrid

General like to code and the direction you are headed. I'd love to see the drag animation and speed bump stuff working. A could things don't seem to be quite right - the 'cone' restriction doesn't seem to work, I can drag at really wide angles to select.  I can also select using very small drag distances. Seems like a good start though. I like the idea of separating the logic out from the grid and having the grid listen to interpreted events.
Attachment #729025 - Flags: feedback?(jmathies) → feedback+
(Assignee)

Comment 15

5 years ago
Created attachment 733364 [details]
Cross-slide module prototype, using global CrossSlide and a CrossSlideHandler, MockGrid

Fixed a load of stuff in the prototype. State management should be fixed so the right events are fired and the right thing happens, cone-checking fixed (ignored until we pass the select threshold). I also added a transform to move the tile along the select axis (max 10px) which I think works pretty well? 

Next steps: integration.
Attachment #729025 - Attachment is obsolete: true
Attachment #733364 - Flags: feedback?(mbrubeck)
Attachment #733364 - Flags: feedback?(jmathies)
(Assignee)

Comment 16

5 years ago
Created attachment 733393 [details]
Cross-slide module prototype, using global CrossSlide and a CrossSlideHandler, MockGrid

Fixed how the grid handles the crosssliding events so the nudging doesn't begin until *after* the dragging threshold (that sounds wrong some maybe got these named wrong or something, but the result is right). This means nothing much happens until you drag 20px or so in the cross-slide direction. 

Also, thresholds are configurable now by passing in an options object to initListeners.
Attachment #733364 - Attachment is obsolete: true
Attachment #733364 - Flags: feedback?(mbrubeck)
Attachment #733364 - Flags: feedback?(jmathies)
Attachment #733393 - Flags: feedback?(mbrubeck)
Comment on attachment 733393 [details]
Cross-slide module prototype, using global CrossSlide and a CrossSlideHandler, MockGrid

Love it!

I think we should have a per-grid way to enable/disable the REARRANGING state.  For a grid with rearranging disabled, there should be no upper bound on the selection gesture (as if CrossSlideThresholds.REARRANGESTART were set to Infinity).  I think this would match the behavior of non-rearrangeable "native" Metro grids, like the "All apps" grid on the Windows Start screen.


>      let cancel = function() {
>        this._fireProgressEvent("cancelled", aEvent, this.node);
>        this.drag = null;
>      }.bind(this);

You could make this a separate method, and then use it in _onTouchEnd too.


>    /**
>     * Dispatches a custom Event on the given target node.
>     * @param aEvent The source event.
>     * @param aType The event type.
>     * @param aTarget The target node that receives the event.
>     */
>    _fireProgressEvent: function CrossSliding_fireEvent(aState, aEvent, aTarget) {

>    /**
>     * Dispatches a custom Event on the given target node.
>     * @param aEvent The source event.
>     * @param aType The event type.
>     * @param aTarget The target node that receives the event.
>     */
>    _fireSelectEvent: function SelectTarget_fireEvent(aEvent, aTarget) {

You could remove the aTarget parameter from both of these, and just use this.node.


>          let offset = Math.max(-10, Math.min(10, aEvent.delta));
>          let getTransform = function(val) {
>            return (aEvent.direction=='x') ?
>              'translateX('+val+'px)' :
>              'translateY('+val+'px)';
>          }
>          aEvent.target.style.transform = getTransform(offset);

10px feels too small here.  This clamping should be based on the speed bump thresholds.  (As a start, we could just use SPEEDBUMPSTART as the limit instead of 10.)
Attachment #733393 - Flags: feedback?(mbrubeck) → feedback+
(In reply to Matt Brubeck (:mbrubeck) from comment #17)
> I think we should have a per-grid way to enable/disable the REARRANGING
> state.

Now that I look closer, I see we can do this by passing { REARRANGESTART: Infinity } to the initListeners function.

By the way, I think something like this should work for speed bumping, though I'm not sure where in the code it should live:

> // This damping function has these important properties:
> // f(0) = 0
> // f'(0) = 1
> // limit as x -> Infinity of f(x) = 1
> function f(aX) {
>   return 2 / (1 + Math.exp(-2 * aX)) - 1;
> }
> 
> function speedbump(aDelta, aStart, aEnd) {
>   let x = Math.abs(aDelta);
>   if (x <= aStart)
>     return aDelta;
>   let sign = aDelta / x;
> 
>   let d = aEnd - aStart;
>   return sign * (aStart + f((x - aStart) / d) * d);
> }
> 
> let offset = speedbump(aEvent.delta, SPEEDBUMPSTART, SPEEDBUMPEND);
(Assignee)

Comment 19

5 years ago
Created attachment 739024 [details] [diff] [review]
Add cross-slide handling to richgrid

As far as the gesture implementation goes, this is basically the code you saw earlier in the prototype. I've added a CrossSlide.js, and it is hooked for richgrids where seltype=multiple. 
This patch looks best on top of the latest from #854960, but should work either way. 
The threshold where a drag ceeses to be treated as a cross-slide is configured in the richgrid's crossslideboundary attribute / crossSlideBoundary setter. initListeners is called in the constructor, uninitListeners in the destructor - I hope that takes care of any nasty surprises XBL/binding can throw at this. 

Selection and the nudge animation is noticeably clunkier in XUL/chrome, vs the HTML prototype btw.
Attachment #739024 - Flags: review?(mbrubeck)
Comment on attachment 739024 [details] [diff] [review]
Add cross-slide handling to richgrid

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

No major problems, but I think this could use just a few minor changes:

::: browser/metro/base/content/CrossSlide.js
@@ +87,5 @@
> +    uninitListeners: function(aNode) {
> +      let id = aNode.id;
> +      let [node, handler] = this._listenerHandlesById[id];
> +      if (node) {
> +        node.removeEventListener("touchstart", handler);

See my question in comment 12 - for simplicity, can we just remove _listenerHandlesById and unitListeners?

@@ +101,5 @@
> +    this.node = aNode;
> +    this.thresholds = Object.create(CrossSlideThresholds);
> +    // apply per-instance threshold configuration
> +    if (aThresholds) {
> +      for(let key in aThresholds)

nit: space after "for"

@@ +250,5 @@
> +      let crossAxis = this.drag.crossAxis;
> +      event.initEvent("MozCrossSliding", true, true);
> +      event.crossSlidingState = aState;
> +      // stash the item/tile node that was drug as the relatedTarget
> +      event.relatedTarget = aTarget;

I think we should dispatch the event directly to aTarget, rather than setting relatedTarget.  (It will bubble up to aEvent.target which is an ancestor of aTarget.)

@@ +267,5 @@
> +    _fireSelectEvent: function SelectTarget_fireEvent(aEvent, aTarget) {
> +      let event = document.createEvent("Events");
> +      event.initEvent("MozCrossSlideSelect", true, true);
> +      // stash the item/tile node that was drug as the relatedTarget(originalTarget, someOtherTarget?)
> +      event.relatedTarget = aTarget;

Same here.

::: browser/metro/base/content/bindings/grid.xml
@@ +414,5 @@
>            if (this.controller && this.controller.gridBoundCallback != undefined)
>              this.controller.gridBoundCallback();
>  
> +          // set up cross-slide gesture handling for multiple-selection grids
> +          if (undefined !== CrossSlide && ("multiple"==this.getAttribute("seltype"))) {

Nits: I prefer "if (CrossSlide && ...)" just for conciseness/readability.

Can we leave out the check completely, and just have a hard dependency on CrossSlide?  I'm open to opinions here...

Please add spaces before and after "==" and feel free to remove the parens.

@@ +426,5 @@
>          ]]>
>        </constructor>
> +      <destructor>
> +        <![CDATA[
> +          if (undefined !== CrossSlide && ("multiple"==this.getAttribute("seltype"))) {

Same here.

@@ +508,5 @@
> +          let state = event.crossSlidingState;
> +          switch(state) {
> +            case "cancelled":
> +              // hopefully nothing else is transform-ing the tile
> +              event.target.style.removeProperty('transform');

Good point in the comment. If we add Windows Start style 3D transforms for :active tiles, we'll need to do some extra work here, but this is good for now.

@@ +517,5 @@
> +              let sign = event.delta < 0 ? -1 : 1;
> +              // we'll trail the touch by ~20px
> +              let delta = Math.abs(event.delta)-CrossSlide.Thresholds.SELECTIONSTART;
> +              // clamp the amout we vertically offset to +/- 15px
> +              let offset = sign*Math.min(15, delta);

As before, 15px seems much too small to me.  Other Metro apps (Windows Start screen, Music) seem to allow much bigger slides.  I think we should clamp to SPEEDBUMPSTART.

::: browser/metro/base/content/browser.xul
@@ +260,5 @@
>              <scrollbox id="start-scrollbox" orient="horizontal" flex="1">
>              <vbox id="start-topsites" class="meta-section">
>                <label class="meta-section-title" value="&startTopSitesHeader.label;"/>
> +              <richgrid id="start-topsites-grid"
> +                  rows="3" columns="3" seltype="multiple" flex="1" crossslideboundary="240"/>

As discussed above, I'd like to set REARRANGESTART to Infinity for grids that don't support rearranging (currently all of them, though we will eventually change that for top sites and possibly bookmarks).  Could we add support for this via crossslideboundary="-1" or with some other attribute?
Attachment #739024 - Flags: review?(mbrubeck) → review-
(Assignee)

Comment 21

5 years ago
Created attachment 740378 [details] [diff] [review]
CrossSlide and richgrid cross-slide handling

I believe I've addressed all comments. 
* The events are now hooked up in the binding constructor (and unhooked in the destructor). 
* The crossSlideBoundary property looks to the crossslideboundary attribute for its value. If its missing or falsy we use Infinity as the default
* The MozCross* events are dispatched from the touch event's target, which should be the richgriditem
* Clamping for the drag behavior more closely matches windows start
* When the gesture is complete or cancelled, there's a transition to snap the tile back to its original position. 
* I tweaked the thresholds a little, so speedbumping is over a bigger range. I anticipate we'll want to tweak these thresholds and the speedbumping implementation this further but its hard to gauge with our current performance.
Attachment #739024 - Attachment is obsolete: true
Attachment #740378 - Flags: review?(mbrubeck)
Comment on attachment 740378 [details] [diff] [review]
CrossSlide and richgrid cross-slide handling

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

::: browser/metro/base/content/CrossSlide.js
@@ +1,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/. */
> +
> +(function(global){

Maybe this should be a JSM file (move it to metro/modules and add EXPORTED_SYMBOLS)?  No major practical difference, but it will enforce some separation between this code and our browser chrome.

Also, I don't always remember this, but I've been trying to add "use strict"; at the start of all new JS files.

@@ +67,5 @@
> +    return { x: touch.clientX, y: touch.clientY };
> +  }
> +
> +  function damp(aX) {
> +    return 2 / (1 + Math.exp(-2 * aX)) - 1;

Could you include my comment explaining what's special about this function?  I'm not sure how helpful it will be to anyone else, but maybe it'll at least help me if I have to reverse-engineer it someday.  :)

::: browser/metro/base/content/bindings/grid.xml
@@ +404,5 @@
>                    onset="this.setAttribute('suppressonselect', val);"/>
>  
> +      <property name="crossSlideBoundary"
> +          onset="this.setAttribute('crossslideboundary', val || Infinity);"
> +          onget="return this.hasAttribute('crossslideboundary')? this.getAttribute('crossslideboundary') : Infinity;"/>

Let's make this property read-only.

@@ +525,5 @@
> +            case "selecting":
> +              event.target.setAttribute("crosssliding", true);
> +              // just track the mouse in the initial phases of the drag gesture
> +              transformValue = (event.direction=='x') ?
> +                                      'translateX('+event.delta+'px)' :

You could combine this with the "speedBumping" case below, since speedbump() is a no-op when delta < SPEEDBUMPSTART.  (If you think it's clearer separate, that's fine too.)

@@ +536,5 @@
> +              // in speed-bump phase, we add inertia to the drag
> +              let offset = CrossSlide.speedbump(
> +                event.delta,
> +                CrossSlideThresholds.SPEEDBUMPSTART,
> +                CrossSlideThresholds.SPEEDBUMPEND

These should be this._xslideHandler.thresholds.SPEEDBUMPSTART/END (doesn't make a difefrence now, but it'll keep things working if someone overrides those in the future).

::: browser/metro/base/jar.mn
@@ -62,4 @@
>  * content/browser-ui.js                        (content/browser-ui.js)
>  * content/browser-scripts.js                   (content/browser-scripts.js)
>    content/ContextCommands.js                   (content/ContextCommands.js)
> -  content/commandUtil.js                       (content/commandUtil.js)

Did you mean to remove this?
Attachment #740378 - Flags: review?(mbrubeck) → review+
(Assignee)

Comment 23

5 years ago
* Re-commented the damp function
* moved CrossSlide.js to modules/CrossSlide.jsm and tested it out - all good. I tweaked the thresholds just a little again, to move the speedbumpstart to closer to the select threshold. 
* got rid of the crossslideBoundary setter
* used drag instance's thresholds 
* left the slight redundancy in the state switch in richgrid. Yes these do pretty much the same thing but its not unlikely we'll need to tweak in here so I prefer the explicit seperate case handling. 

Landed on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/fa5e73d8da49
https://hg.mozilla.org/mozilla-central/rev/fa5e73d8da49
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.