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

VERIFIED FIXED

Status

VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: gnarf, Assigned: gnarf)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:koi+, b2g-v1.2 verified)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
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
(Assignee)

Comment 1

5 years ago
Created attachment 806207 [details] [diff] [review]
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
Attachment #806207 - Flags: review?(mike)
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?
Attachment #806207 - Flags: review?(mike)
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.
Attachment #806207 - Flags: review+
(Assignee)

Comment 4

5 years ago
Created attachment 806763 [details] [diff] [review]
patch v2
Attachment #806207 - Attachment is obsolete: true
Attachment #806763 - Flags: review?(mike)
Comment on attachment 806763 [details] [diff] [review]
patch v2

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

Even better!
Attachment #806763 - Flags: review?(mike) → review+
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.
(Assignee)

Comment 7

5 years ago
master: https://github.com/mozilla-b2g/gaia/commit/41f2c409c8baf9e5375ab0d41d0bbc3fe4ecf492
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Requesting "koi+" because the behavior of this "picker" UI element should match those in the System application as closely as possible.
blocking-b2g: --- → koi?
triage: not blocking, we'll pick this up in 1.3
blocking-b2g: koi? → -
(Assignee)

Comment 10

5 years ago
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)
blocking-b2g: - → koi?
(Assignee)

Comment 11

5 years ago
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.
Attachment #806763 - Flags: approval-gaia-v1.2?
(Assignee)

Updated

5 years ago
Depends on: 912250
(Assignee)

Comment 12

5 years ago
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?
Attachment #806763 - Flags: feedback?(gaye)
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.
Flags: needinfo?(gnarf37)
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.
(Assignee)

Comment 15

5 years ago
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
Status: RESOLVED → REOPENED
Flags: needinfo?(gnarf37) → needinfo?(doliver)
Resolution: FIXED → ---
(Assignee)

Comment 16

5 years ago
Created attachment 810123 [details] [diff] [review]
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.
Attachment #806763 - Attachment is obsolete: true
Attachment #806763 - Flags: feedback?(gaye)
Attachment #806763 - Flags: approval-gaia-v1.2?
Attachment #810123 - Flags: review?(mike)
Attachment #810123 - Flags: feedback?(gaye)
(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.
blocking-b2g: koi? → koi+
Flags: needinfo?(doliver)
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`
Attachment #810123 - Flags: review?(mike)
(Assignee)

Comment 19

5 years ago
Created attachment 811275 [details] [diff] [review]
patch v4

Addresses review comments.
Attachment #810123 - Attachment is obsolete: true
Attachment #810123 - Flags: feedback?(gaye)
Attachment #811275 - Flags: review?(mike)
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.
Attachment #811275 - Flags: review?(mike) → review+
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?
Flags: needinfo?(gnarf37)
(Assignee)

Comment 22

5 years ago
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.
Flags: needinfo?(gnarf37)
(Assignee)

Comment 23

5 years ago
master: https://github.com/mozilla-b2g/gaia/commit/f6dde1da8c8a5693a6e18e26d77823726805dfa1
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
(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.
(Assignee)

Comment 25

5 years ago
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"
(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.
Uplifted f6dde1da8c8a5693a6e18e26d77823726805dfa1 to:
v1.2: e5b6ea80889719196506f0c5f5dbc3d4b51a5e0d
status-b2g-v1.2: --- → fixed
(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?
Flags: needinfo?(gnarf37)
(Assignee)

Updated

5 years ago
Blocks: 922881
(Assignee)

Comment 29

5 years ago
bug 922881 just now submitted
(Assignee)

Updated

5 years ago
Flags: needinfo?(gnarf37)

Comment 30

5 years ago
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
status-b2g-v1.2: fixed → verified

Comment 31

5 years ago
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.