Flame: the date/time selection flywheel is quite jerky when adding an event

VERIFIED FIXED in Firefox OS v2.2

Status

Firefox OS
Gaia::System
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: njpark, Assigned: birtles)

Tracking

({regression})

unspecified
2.2 S8 (20mar)
ARM
Gonk (Firefox OS)
regression

Firefox Tracking Flags

(blocking-b2g:2.2+, b2g-v2.1 unaffected, b2g-v2.2 verified, b2g-master verified)

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
STR:
Open Calendar app, and tap + to add events
Tap the date or time field to open the flywheel selector, and scroll to pick date/time

Expected:
Flywheel motion is smooth.
Actual:
Motion is quite jumpy, highlighting numbers that's not even selected


Version Info:
2.2:
Gaia-Rev        cd62ff9fe199fb43920ba27bd5fdbc5c311016fc
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/11d93135c678
Build-ID        20150203002504
Version         37.0a2
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150203.041704
FW-Date         Tue Feb  3 04:17:15 EST 2015
Bootloader      L1TC000118D0

Master:
Gaia-Rev        70922d07e1de10aea3381aa954add71ce75c2e2d
Gecko-Rev       
Build-ID        20150204104037
Version         38.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.mozilla.20150204.102335
FW-Date         Wed Feb  4 10:23:48 EST 2015
Bootloader      L1TC000118D0
(Reporter)

Comment 1

3 years ago
Hi Edward, I was told you're the qa person for calendar app.  Could you assess this issue and nominate if necessary?  thanks!
Flags: needinfo?(edchen)
just noticed this as well. we would need a regression window. doesn't looks like calendars fault.
blocking-b2g: --- → 2.2?
Keywords: regressionwindow-wanted
QA Contact: bzumwalt
Let's get a branch check on this first then we will get the regression window.
Keywords: qawanted
QA Contact: bzumwalt → ktucker
QA Contact: ktucker
This issue is similar to bug 1093449. It is a matter of opinion how easy/difficult the time selection wheel works for everyone. See bug 1093449 comment 3 and bug 1093449 comment 5.

For this issue I compared latest 3.0's central behavior with base image v18D-1's behavior and their differences are minimal. On 3.0 it seems to be scrolling more aggressively but the difference is just not noticeable enough to call for a regression window. For regression windows we need to be extremely precise about bug behavior and clearly distinguishing between working and broken; for this case it's more like a feeling for each instance, and we can't rely on such volatile data.

Unless original reporter is talking about a completely different and much more noticeable bug behavior, I'm afraid we can't be of much help here.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Keywords: qawanted, regressionwindow-wanted
No-Jun, I am not really seeing a difference either with the smoothness of the wheel between the branches to get a regression window. Can we get a video for this?
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker) → needinfo?(npark)
(Reporter)

Comment 6

3 years ago
Hi, here is the video link: http://youtu.be/sDLLakgANXo
Flags: needinfo?(npark)
(Reporter)

Comment 7

3 years ago
If you open a clock app and scroll the flywheel on the Timer tab, the behavior is smooth, not exhibiting the behaviour seen in Calendar app. (Comment 6).  Edward, what do you think?
Timer uses a different component than Alarm (try to create a new alarm on the clock app and set the time).. The problem should be on the shared components (`<input type="time" />` and `<input type="date" />`), this is not a Calendar specific bug.

To see the bug you need to scroll slowly - it will highlight the "destination" date/time and will only animate after `touchend` - it won't update the Y-position during the drag.
Component: Gaia::Calendar → Gaia::Components

Comment 9

3 years ago
This case about user experience, even the issue is not bug we still need to improve it.
Flags: needinfo?(edchen)
(Reporter)

Comment 10

3 years ago
This is also reproducing in Settings -> Date and Time and when manually entering the date or time.  KTucker, can you repro what's seen in Comment 6?  IMO it makes difficult for the user to quickly set to the correct time.
Flags: needinfo?(ktucker)
This issue does NOT reproduce on Flame 2.1.

Result: Highlighting the number and scrolling on the flywheel are consistent.

Environmental Variables:
Device: Flame 2.1
BuildID: 20150211170050
Gaia: 88084bc7ef5ba6627dd09c774ef2f7fa96cbed71
Gecko: e395bfad7bc9
Version: 34.0 (2.1) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
status-b2g-v2.1: --- → unaffected
status-b2g-v2.2: --- → affected
status-b2g-master: --- → affected
Keywords: regression
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Keywords: regressionwindow-wanted
Mozilla-inbound Regression Window:

Last Working Environmental Variables:
Device: Flame 2.2
BuildID: 20150104211243
Gaia: c2bf20d23851d5fda9f8f0ef0267db5f49152376
Gecko: b9f40d0310d5
Version: 37.0a1 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

First Broken Environmental Variables:
Device: Flame 2.2
BuildID: 20150105033543
Gaia: 4ceeff19086b2a2955f044ad923dcfa63a293de3
Gecko: b9ba4d717f8a
Version: 37.0a1 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Last Working Gaia First Broken Gecko: Issue DOES reproduce 
Gaia: c2bf20d23851d5fda9f8f0ef0267db5f49152376
Gecko: b9ba4d717f8a

First Broken Gaia Last Working Gecko: Issue does NOT reproduce
Gaia: 4ceeff19086b2a2955f044ad923dcfa63a293de3 
Gecko: b9f40d0310d5

http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b9f40d0310d5&tochange=b9ba4d717f8a

possibly caused by bug 927349
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Keywords: regressionwindow-wanted
Brian, can you take a look at this please? Looks like the work done on bug 927349 might be the culprit here.
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker) → needinfo?(bbirtles)
(Assignee)

Comment 14

3 years ago
Sorry, I'm in meetings all week and don't have a Flame handy so I won't be able to look into this until next week (although I see similar behavior on my 2.0 phone here).

Can you point me to the source for Gaia's <input type="date"> widget in the meantime?

Also, in the meantime we should check that we're using will-change on the scrollable component and check what easing we're using.
Flags: needinfo?(ktucker)
I think the source for the inputs are inside the system app (https://github.com/mozilla-b2g/gaia/tree/master/apps/system/js/value_selector)
(Assignee)

Comment 16

3 years ago
For my own reference, I think the first thing to try is to add "will-change: transform" to:

  https://github.com/mozilla-b2g/gaia/blob/14d3ea7df3d8e8bd14f326fa15b8f017044d79f9/shared/style/date_selector.css#L172

Basically, bug 927349 means that we wait for layerization to complete before we start the animation. This means animations start smoothly but it also means that animations can appear to take longer to start. By setting will-change: transform, we hint to Gecko to do layerization in advance so we don't have to wait for it when animation starts.

It's possible that there's a bug introduced by bug 927349, but my first hunch would be the above.
Flags: needinfo?(ktucker)
(Assignee)

Comment 17

3 years ago
I've spent over a day digging into this. It seems like we aren't using OMTA here because the layers are much bigger than the screen. When I use will-change: transform we often don't render enough in time and get big blank patches.

I've spoken to Matt Woodrow about it and this approach of doing finger tracking on the main thread seems suboptimal (there's already plenty of lag even before bug 927349 and it's almost impossible to move a wheel one notch). We really should just set up each column as a scrollable container so we can do the tracking on the compositor. Then we could use scrollTo to provide the snapping behavior. I started working on this but it's a pretty significant change and I'm not sure if it would be accepted onto the 2.2 branch (and I'm probably not the best one to work on this).

KTucker, would a rewrite of value picker be feasible for 2.2?
Flags: needinfo?(bbirtles) → needinfo?(ktucker)
(Assignee)

Comment 18

3 years ago
(In reply to Brian Birtles (:birtles) from comment #17)
> KTucker, would a rewrite of value picker be feasible for 2.2?

Sorry that question was unclear. If someone were available to rework value picker to use scrolling (instead of translating) how likely would it be accepted into 2.2?
(Reporter)

Comment 19

3 years ago
adding Bhavana for the question raised in Comment 18, since this bug is in 2.2? state for a while.  My personal opinion is that it should be fixed since it's a regression, and it's now difficult to smoothly change value.  Would really annoy the user.
Flags: needinfo?(bbajaj)
Flags: needinfo?(ktucker)
(Assignee)

Comment 20

3 years ago
Created attachment 8565801 [details] [diff] [review]
Initial attempt to use scroll containers

Hi Kip, here is my initial attempt to start using scroll containers for this widget. As discussed on IRC, after calling scrollTo, I still see other scroll events coming in. I'm guessing the APZC stuff is still animating the scroll. Any idea where I should look to confirm this? Or is there anything obviously amiss in this Gaia patch (apart from all the buggy layout (clipped container, scrollbars visible etc.)?

I'm seeing output like this:

(...fling year column...)
Got scroll, scrollTop: 6559
Got scroll, scrollTop: 6563
Got scroll, scrollTop: 6565
Updating index from 117 to 118 <-- crossed a boundary
Got scroll, scrollTop: 6572
Calling scrollTo: 56 <-- setting month (no change)
Calling scrollTo: 952 <-- setting day (no change)
Calling scrollTo: 6608 <-- setting year, we should scroll and land on 6608
Got scroll, scrollTop: 6578 <-- so far so good 
Got scroll, scrollTop: 6583
Got scroll, scrollTop: 6589
Got scroll, scrollTop: 6595
Got scroll, scrollTop: 6600
Got scroll, scrollTop: 6605
Got scroll, scrollTop: 6610 <-- the scroll keeps on going!
Got scroll, scrollTop: 6615
Got scroll, scrollTop: 6619
Got scroll, scrollTop: 6624
Got scroll, scrollTop: 6628
Got scroll, scrollTop: 6632
Got scroll, scrollTop: 6636 <-- and going
... (more events in between)
Got scroll, scrollTop: 6687
Got scroll, scrollTop: 6688
Got scroll, scrollTop: 6689
Got scroll, scrollTop: 6690 <-- and going

The initial call to scrollTo, however, seems to work.
Flags: needinfo?(kgilbert)
(Assignee)

Comment 21

3 years ago
Also, I tried to work out why we're getting unpainted content with will-change: transform but couldn't get any sensible answers. It seems like we exhaust out will-change budget so ShouldPrerender returns false but even if I force it to true and disable progressive painting we still seem to get unpainted content.

I'm going to give up on that approach for now in the hope of using the scroll container approach.
(Assignee)

Comment 22

3 years ago
For reference, you need to rebuild the system app when applying these changes to Gaia (i.e. 'APP=system make') then run the calendar app (e.g. 'dist/bin/b2g -profile ~/gaia/profile-debug -runapp calendar')

Updated

3 years ago
blocking-b2g: 2.2? → 2.2+
Flags: needinfo?(bbajaj)

Comment 23

3 years ago
Hi Brian, do you plan to take this bug? thanks.
Flags: needinfo?(bbirtles)
(Assignee)

Comment 24

3 years ago
I'm still waiting on Kip to respond (and I haven't been in the office to chase him up).
Flags: needinfo?(bbirtles)
(Assignee)

Comment 25

3 years ago
But, yes, I'll probably end up taking it unless we decide to take a different approach. I'll look into it early next week.
(In reply to Brian Birtles (:birtles, away most of 21 Feb~6 Mar) from comment #20)
> Created attachment 8565801 [details] [diff] [review]
> Initial attempt to use scroll containers
> 
> Hi Kip, here is my initial attempt to start using scroll containers for this
> widget. As discussed on IRC, after calling scrollTo, I still see other
> scroll events coming in. I'm guessing the APZC stuff is still animating the
> scroll. Any idea where I should look to confirm this? Or is there anything
> obviously amiss in this Gaia patch (apart from all the buggy layout (clipped
> container, scrollbars visible etc.)?
> 
> I'm seeing output like this:
> 
> (...fling year column...)
> Got scroll, scrollTop: 6559
> Got scroll, scrollTop: 6563
> Got scroll, scrollTop: 6565
> Updating index from 117 to 118 <-- crossed a boundary
> Got scroll, scrollTop: 6572
> Calling scrollTo: 56 <-- setting month (no change)
> Calling scrollTo: 952 <-- setting day (no change)
> Calling scrollTo: 6608 <-- setting year, we should scroll and land on 6608
> Got scroll, scrollTop: 6578 <-- so far so good 
> Got scroll, scrollTop: 6583
> Got scroll, scrollTop: 6589
> Got scroll, scrollTop: 6595
> Got scroll, scrollTop: 6600
> Got scroll, scrollTop: 6605
> Got scroll, scrollTop: 6610 <-- the scroll keeps on going!
> Got scroll, scrollTop: 6615
> Got scroll, scrollTop: 6619
> Got scroll, scrollTop: 6624
> Got scroll, scrollTop: 6628
> Got scroll, scrollTop: 6632
> Got scroll, scrollTop: 6636 <-- and going
> ... (more events in between)
> Got scroll, scrollTop: 6687
> Got scroll, scrollTop: 6688
> Got scroll, scrollTop: 6689
> Got scroll, scrollTop: 6690 <-- and going
> 
> The initial call to scrollTo, however, seems to work.

Hi Brian,

Sorry for the delay in getting back to you.

I will need to try this from source to see what is going on.  Also, perhaps some of my smooth scrolling related fixes landing in Bug 945584 could have an effect.

When you're back, I'd be glad to hash it out with you here.

- Kip
Flags: needinfo?(kgilbert)
Created attachment 8574518 [details] [review]
[gaia] birtles:fixspinner-v2 > mozilla-b2g:master
(Assignee)

Comment 28

3 years ago
Comment on attachment 8574518 [details] [review]
[gaia] birtles:fixspinner-v2 > mozilla-b2g:master

Details of this patch are in the PR. We could further tweak this by adjusting the transition duration to 0.4s to better match the previous behavior.
Attachment #8574518 - Flags: review?(mmedeiros)
(Assignee)

Comment 29

3 years ago
For reference, I also tried the following:

a) Setting will-change: transform on the translated container. This should mean we do the layerization up-front and remove any lag. However, it exposed invalidation problems. The prerendered area would get clipped to its container. I tried tweaking the CSS, disabling progressive painting, and forcing prerendering in all code paths but I kept hitting the same problem. After looking into it with Matt and sinking about a day into working out what was going wrong I wasn't getting anywhere so I decided to leave this for now.

b) Making each dial a scroll container. This what we really should do since we can then do it all on the compositor thread. I tried doing this but ran into the problems described in comment 20. Kip kindly offered to help but I realised that this was going to be a significant rewrite of the existing code and risky to accept into 2.2. Furthermore, going forward we should really use the scroll snapping introduced in bug 945584 which is not available in 2.2.

In the end I realised the real problem was just that now we are seeing the ease-in built into the default transition timing function. I've added a patch that fixes that timing function. If that's not enough we can further tweak this by adjusting the transition duration and perhaps even adding a short negative delay to skip the first part of the transition.
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
(Assignee)

Comment 30

3 years ago
For reference, the cubic bezier arrangement is described here:
  http://w3c.github.io/web-animations/#scaling-using-a-cubic-bezier-curve
Comment on attachment 8574518 [details] [review]
[gaia] birtles:fixspinner-v2 > mozilla-b2g:master

even with the cubic-bezier the drag is still not as smooth as before. I think the easiest solution would be to actually add a class during `vp_touchstart`, that disables the transition, and remove the class on `vp_touchend`, re-enabling the transition.

If we do remove the transition it would also require some updates to the current snap logic - setting the selected value also updates the translateY, so it jumps to the destination position and looks really weird.
Attachment #8574518 - Flags: review?(mmedeiros) → review-
(Assignee)

Comment 32

3 years ago
(In reply to Miller Medeiros [:millermedeiros] from comment #31)
> even with the cubic-bezier the drag is still not as smooth as before.

How are you testing? What difference are you seeing? I've been running two Flame devices side by side: one with 2.0 and one with trunk + this patch and I can't find any difference in behavior for slow drags, medium drags, or fast flicks. Perhaps you're testing some gesture I haven't tried? What do you mean by not as smooth?

> I
> think the easiest solution would be to actually add a class during
> `vp_touchstart`, that disables the transition, and remove the class on
> `vp_touchend`, re-enabling the transition.
> 
> If we do remove the transition it would also require some updates to the
> current snap logic - setting the selected value also updates the translateY,
> so it jumps to the destination position and looks really weird.

Yes, I agree this whole widget needs to be rewritten. It's weird even in 2.0.
Flags: needinfo?(mmedeiros)
Created attachment 8574942 [details] [review]
[gaia] millermedeiros:1129494-date-time-picker > mozilla-b2g:master
Comment on attachment 8574942 [details] [review]
[gaia] millermedeiros:1129494-date-time-picker > mozilla-b2g:master

check this patch. to me it feels way more responsive than just changing the cubic-bezier, specially when you drag slowly and try to snap to some value that is 2-3 steps before/after the one that is currently selected (easier to notice on the month picker).

it took me a while to figure out what was going on (why the snapping was wrong).. it all boils down to the SpinDatePicker updating the "range" each time the year/month changes, which triggers a new `setSelectedIndex` call and invalidated the logic that avoided the snap.
Flags: needinfo?(mmedeiros)
Attachment #8574942 - Flags: feedback?(bbirtles)
changing component to Gaia::System so we can mark someone from the system team to review the patch and autoland.. specially since I had to make changes to the JS file inside "apps/system" (while CSS is at "shared/style")
Component: Gaia::Components → Gaia::System
(Assignee)

Comment 36

3 years ago
Comment on attachment 8574942 [details] [review]
[gaia] millermedeiros:1129494-date-time-picker > mozilla-b2g:master

This certainly feels better but:
* It often ends up landing between items (screenshot coming).
* This drops the ignorePicker parameter from setSelectedIndex but doesn't update all call sites.
* This bug is for the regression from bug 927349. We should land attachment 8574518 [details] [review] as a separate patch to fix the regression then improve on top of that. Then, if we find bugs in any additional improvements we can back them out and still keep the regression fix.
Attachment #8574942 - Flags: feedback?(bbirtles) → feedback-
(Assignee)

Comment 37

3 years ago
Created attachment 8575008 [details]
Missed selection screenshot

With attachment 8574942 [details] [review] the day selection often seems to land between items both on initial load and when interacting with the dial.
(In reply to Brian Birtles (:birtles) from comment #36)
> Comment on attachment 8574942 [details] [review]
> [gaia] millermedeiros:1129494-date-time-picker > mozilla-b2g:master
> 
> This certainly feels better but:
> * It often ends up landing between items (screenshot coming).
> * This drops the ignorePicker parameter from setSelectedIndex but doesn't
> update all call sites.
> * This bug is for the regression from bug 927349. We should land attachment
> 8574518 [details] as a separate patch to fix the regression then improve on
> top of that. Then, if we find bugs in any additional improvements we can
> back them out and still keep the regression fix.

I was not able to reproduce the "landing between items" on my flame but agree that maybe this kind of work would be better as a separate patch. please flag someone from Gaia::System for the review otherwise autoland won't do it's magic and they might also have a better feedback. cheers.
Created attachment 8575765 [details] [review]
[gaia] birtles:fixspinner-v3 > mozilla-b2g:master
(Assignee)

Updated

3 years ago
Attachment #8574518 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8575765 - Flags: review?(alive)
(Assignee)

Comment 40

3 years ago
(In reply to Miller Medeiros [:millermedeiros] from comment #38)
> I was not able to reproduce the "landing between items" on my flame

I just updated to trunk Gaia, reapplied that PR but I still see it landing between items for the day of the month picker only. Although, it's less obvious than it was before. Perhaps because it's a different day of the month now. It would be neat to fix this since your patch definitely makes this control a *lot* more useable.

> please flag someone from Gaia::System for the review otherwise autoland
> won't do it's magic and they might also have a better feedback. cheers.

I'm not too familiar with autolander so let me know if I've done it wrong. I didn't put the 'r=alive' in the PR title because I'm not sure who is the best reviewer. I hope that doesn't get in autolander's way.
Comment on attachment 8575765 [details] [review]
[gaia] birtles:fixspinner-v3 > mozilla-b2g:master

r=me
Attachment #8575765 - Flags: review?(alive) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed

Updated

3 years ago
Keywords: checkin-needed

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Comment 43

3 years ago
Comment on attachment 8575765 [details] [review]
[gaia] birtles:fixspinner-v3 > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): bug 927349
[User impact] if declined: The date/time spinner is quite jerky
[Testing completed]: Confirmed locally, landed on master
[Risk to taking this patch] (and alternatives if risky): Minimal change, simply updates the timing function on the transition to remove ease-in
[String changes made]: None
Attachment #8575765 - Flags: approval-gaia-v2.2?

Updated

3 years ago
Attachment #8575765 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+

Updated

3 years ago
status-b2g-master: affected → fixed
v2.2: https://github.com/mozilla-b2g/gaia/commit/294015fd41e04a8ae175f87b416b13e61396ac03
status-b2g-v2.2: affected → fixed
Target Milestone: --- → 2.2 S8 (20mar)

Comment 45

3 years ago
Created attachment 8632726 [details]
Verified video:Flame_v2.2.3gp

This bug has been verified as "pass" on latest build of Flame v2.2&master.
STR same as in comment 0.
Actual result: When you scroll to pick date/time in Calendar app,Flywheel motion is smooth.

Reproduce rate: 0/10
See attachment: Flame_v2.2.3gp

Device information:
Device Flame 2.2:(pass)
Build ID               20150712162503
Gaia Revision          84d0c76370dcd3d25813b00de55194730884355b
Gaia Date              2015-07-09 13:09:14
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/8d59402ba85a
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150712.201340
Firmware Date          Sun Jul 12 20:13:51 EDT 2015
Bootloader             L1TC000118D0

Device Flame master:(pass)
Build ID               20150712160203
Gaia Revision          e4b63559eba364892867eb381c3002d6518e5d6a
Gaia Date              2015-07-10 14:29:23
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/eab21ec484bb
Gecko Version          42.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150712.193621
Firmware Date          Sun Jul 12 19:36:34 EDT 2015
Bootloader             L1TC000118D0

Updated

3 years ago
status-b2g-v2.2: fixed → verified
status-b2g-master: fixed → verified

Updated

3 years ago
Status: RESOLVED → VERIFIED

Updated

3 years ago
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][MGSEI-Triage+]
You need to log in before you can comment on or make changes to this bug.