917463 - [Clock] Implement scrolling inertia in inline time picker for Timers

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Clock, defect)

Description

[Corey Frang [:gnarf]]

The standard <input type=time> has a little scrolling inertia when you try to scroll quickly through values, our time picker should implement the same sort of scrolling inertia when selecting the time for a Timer

Comment 1

[Corey Frang [:gnarf]]

Attachment: patch v1

- Removes "slow down" effect when under threshold, is awkward anyway
- Adds "inertia" by detecting the speed of the last pan event when a swipe finishes and adjusts the index properly
- Separated out the "stopInteraction" method from the onswipe

Comment 2

[Mike Pennisi [:jugglinmike]]

Comment on attachment 806207 [details] [diff] [review]
patch v1

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

Hey Corey,

Merging your patch for bug 917337 introduced a conflict for this patch. Would you mind resolving it and uploading a new version?

Comment 3

[Mike Pennisi [:jugglinmike]]

Comment on attachment 806207 [details] [diff] [review]
patch v1

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

Actually, the resolution was obvious--the previous patch and this one added two distinct functions to the same line. I kept both functions and reviewed based on that. Assuming you resolve it in this way, this patch should be fine to land.

Comment 4

[Corey Frang [:gnarf]]

Attachment: patch v2

Comment 5

[Mike Pennisi [:jugglinmike]]

Comment on attachment 806763 [details] [diff] [review]
patch v2

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

Even better!

Comment 6

[Mike Pennisi [:jugglinmike]]

While reviewing this patch, I noticed a bug in the spinner behavior. Since the erroneous behavior exists in `master`, I've created "Bug 917956 - [Clock] Timer spinner allows for out-of-bound input" to track its resolution separately. This might be a good followup for you, Corey.

Comment 7

[Corey Frang [:gnarf]]

master: https://github.com/mozilla-b2g/gaia/commit/41f2c409c8baf9e5375ab0d41d0bbc3fe4ecf492

Comment 8

[Mike Pennisi [:jugglinmike]]

Requesting "koi+" because the behavior of this "picker" UI element should match those in the System application as closely as possible.

Comment 9

[Dylan Oliver [:doliver]]

triage: not blocking, we'll pick this up in 1.3

Comment 10

[Corey Frang [:gnarf]]

Considering that this was a new feature added for 1.2, this change is pretty important in making the time selection in the Timer panel "usable" and "consistent" with the UX of the standard time input.   I'd like to request considering this for the 1.2 branch again (especially since the code is already in master)

Comment 11

[Corey Frang [:gnarf]]

Comment on attachment 806763 [details] [diff] [review]
patch v2

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): 912250
[User impact] if declined: It is very hard to scroll through to set time in timer when inertia isn't there.  This bug ensures the timer value picking feel similar to the <input type=time> time picker.
[Testing completed]: Tested on devices and desktop
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]: none

In case we don't koi+ this, I still want to land it in 1.2, this seriously improves the usability of the timer time picker.

Comment 12

[Corey Frang [:gnarf]]

Comment on attachment 806763 [details] [diff] [review]
patch v2

Garteh, Would you mind looking this patch over from the risk in uplifting to 1.2 perspective?

Comment 13

[Gareth Aye [:gaye] (back from PTO)]

Hi Gnarf,

No JavaScript changes should ever land without unit tests in Gaia. Also, when there are changes to the app's user interface [and now that we have the infrastructure to test these things], patches should not land without integration tests.

I know that we are in a bit of a time crunch, but this should not have landed on master in its current state. Please back this out, write tests, and open a new pull request. I'd be happy to provide a second review, test on my device, and advise about landing in 1.2 when the patch is ready.

Comment 14

[Mike Pennisi [:jugglinmike]]

Hey Gareth,

Three factors motivated me to approve this patch without an integration test
(and request "koi+"):

1. The change is straightforward
2. It significantly improves the user interface
3. The integration test suite for Clock (tracked via bug 907177) is blocked by
   an as-yet unidentified race condition that is only reproducable on TravisCI,
   which has impeded the debugging process.

(For the record, Corey took some convincing to land code without tests :P)

I was unaware of the strictness of the testing policy, and I don't expect this
rationale to stand up against a hard-and-fast rule. I wanted it to be clear
that re-landing this patch with an integration test may take longer than it
seems, and the usability of the Clock application will be lessened in the mean
time.

Comment 15

[Corey Frang [:gnarf]]

reverted: https://github.com/mozilla-b2g/gaia/commit/08450a03844c17b6fefffdaa1372fa2152463994

Yes, I took some convincing, the picker/ chunk of code was untested, and mike convinced me to just land it without writing a suite for everything spinner/picker.

I'm going to write the units for this code and resubmit this patch. I had already started a branch to do this because my conscious was still bugging me.

Now that the commit has been backed out of master, feel free to try setting a 55 minute timer without this code and tell me that it shouldn't be koi+ :)

-Corey

Comment 16

[Corey Frang [:gnarf]]

Attachment: patch v3

Thanks for requesting some new units here, I managed to fix a couple of other bugs in the Spinner implementation while I was writing these.

As far as integration tests go, I believe we are still blocked by the bug mike mentioned earlier.

Comment 17

[Dylan Oliver [:doliver]]

(In reply to Corey Frang [:gnarf] [:gnarf37] from comment #15)
> Now that the commit has been backed out of master, feel free to try setting
> a 55 minute timer without this code and tell me that it shouldn't be koi+ :)

Yeah, this is pretty rough. Agreed we need to fix this.

Comment 18

[Mike Pennisi [:jugglinmike]]

Comment on attachment 810123 [details] [diff] [review]
patch v3

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

Hey Corey

Other than the in-line comments, I noticed a tTypo in the commit message: "caluclations" -> "calculations

::: apps/clock/js/picker/spinner.js
@@ +1,4 @@
>  (function(exports) {
>    'use strict';
>  
> +  // units covered per milisecond threshold to kick off inertia

"milisecond" -> "millisecond"

@@ +55,5 @@
> +          this.select(this.values.indexOf(value));
> +        }
> +      },
> +      container: {
> +        value: setup.element.parentNode || setup.element

Can we remove the alternate value here? The tests pass without it, and the application functions. Understanding the code becomes a little more difficult if `container === element` may hold true.

@@ +189,3 @@
>      this.update();
>  
>      clearInterval(this.timeout);

I know they're functionally equivalent, but do you mind updating this to `clearTimeout`. It will help readability/grep-ability.

@@ +189,5 @@
>      this.update();
>  
>      clearInterval(this.timeout);
>  
> +    this.timeout = setTimeout(this.stopInteraction.bind(this), 200);

Is there a good name for the value `200`? `THROTTLE_PERIOD`, maybe...?

::: apps/clock/test/unit/picker/spinner_test.js
@@ +13,5 @@
> +      Element.prototype, 'clientHeight');
> +    Object.defineProperty(Element.prototype, 'clientHeight', {
> +      configurable: true,
> +      enumerable: true,
> +      get: function() {

This is super scary. Lets use a named function expression to further aid debugging in the unlikely event that this leaks. Preferably something long, obnoxious, and descriptive. i.e. `clockTestClientHeightStub`

Comment 19

[Corey Frang [:gnarf]]

Attachment: patch v4

Addresses review comments.

Comment 20

[Mike Pennisi [:jugglinmike]]

Comment on attachment 811275 [details] [diff] [review]
patch v4

Looks good to me, Corey.

I appreciate your taking the time to write unit tests for this even though integration tests would have been simpler. Lets plan on removing some of the more invasive unit tests once bug 907177 lands.

Comment 21

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Hey Corey,
Is there any reasons to not have use the native scrolling which is provided by the platform when the element content is bigger than the container?

Comment 22

[Corey Frang [:gnarf]]

Viven: The idea here is to replicate the interface used by <input type=time> which doesn't have 3 internal scroll bars, it just traps touch events and generates it's own "scrolling" effect much the same way that we do.

The native scrolling will add the scroll bars to the UI which is probably is not desired, also makes it a little harder to track the "value" of the input.

Comment 23

[Corey Frang [:gnarf]]

master: https://github.com/mozilla-b2g/gaia/commit/f6dde1da8c8a5693a6e18e26d77823726805dfa1

Comment 24

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Corey Frang [:gnarf] [:gnarf37] from comment #22)
> Viven: The idea here is to replicate the interface used by <input type=time>
> which doesn't have 3 internal scroll bars, it just traps touch events and
> generates it's own "scrolling" effect much the same way that we do.
> 
> The native scrolling will add the scroll bars to the UI which is probably is
> not desired, also makes it a little harder to track the "value" of the input.

What about hiding the scrollbars by having each list of numbers beeing a little big bigger than the box that contains them. Them by ordering z-index correcly it may works. FWIW I would be fairly happy to fix the current <input type="time"> as well. Right now I have not seen it reach 60 fps because of some repaints issues and I would like it to fix it.

I believe that extrapolating the current value of the input will not be that hard with some APIs like https://developer.mozilla.org/en-US/docs/Web/API/document.elementFromPoint or or any other heuristic but I may not understand the issue here.

Comment 25

[Corey Frang [:gnarf]]

Viven: I'd be interested in seeing this approach work if you want to take a stab at it.  Perhaps we should open a new bug for it though: "Implement scrollable area via device scrolling in value selectors"

Comment 26

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Corey Frang [:gnarf] [:gnarf37] from comment #25)
> Viven: I'd be interested in seeing this approach work if you want to take a
> stab at it.  Perhaps we should open a new bug for it though: "Implement
> scrollable area via device scrolling in value selectors"

I would be happy if you an take a stab at it ;) And yes a separate bug would be better.

Comment 27

[James Lal [:lightsofapollo]]

Uplifted f6dde1da8c8a5693a6e18e26d77823726805dfa1 to:
v1.2: e5b6ea80889719196506f0c5f5dbc3d4b51a5e0d

Comment 28

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Corey Frang [:gnarf] [:gnarf37] from comment #25)
> Viven: I'd be interested in seeing this approach work if you want to take a
> stab at it.  Perhaps we should open a new bug for it though: "Implement
> scrollable area via device scrolling in value selectors"

Corey have you already opened a bug for that?

Comment 29

[Corey Frang [:gnarf]]

bug 922881 just now submitted

Comment 30

[Lucas Aliaga]

Verified fixed in Aurora 1.2 Buri. Inertial scrolling for the choose time wheels is present. Scroll bars also don't display when choosing time as intended.

Environment:
Gaia   dbcc171eae6b4d9d168a48291d5ea54c7580561a
SourceStamp 7ca5d1e81d37
BuildID 20131017004001
Version 26.0a2

Comment 31

[William Hsu [:whsu]]

Thanks! Verified on V 1.2

* Test Build:
 - Gaia:     2ef9bc3c7a6de228b63e6ba3613eb0c0dd639c59
 - Gecko:    http://hg.mozilla.org/releases/mozilla-aurora/rev/4a94d2ea9d37
 - BuildID   20131028004002
 - Version   26.0a2