Closed Bug 960846 Opened 6 years ago Closed 6 years ago

APZC pinching to zoom is very jittery

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: drs, Assigned: drs)

Details

Attachments

(4 files, 2 obsolete files)

Attached patch multitouch-no-focus.patch (obsolete) — Splinter Review
I noticed when pinching to zoom using Inari and Hamachi devices that movement is very jittery. I traced it back to our pinch focus point correction code. I don't even really know if this is mechanism is necessary on our very low-res devices. I experimented a bit with disabling this, and pinching is much smoother without it (PoC patch attached).

I would expect this functionality on a high-end device, but on the devices FxOS is commercially available on, I don't think it makes sense.

This is definitely more of a UX issue, though. I'll talk with someone on the UX team about it.
Component: Graphics: Layers → Panning and Zooming
This patch disables changing focus points on B2G only. What that means is that, if you move the center point of your fingers around while pinching to zoom, the screen will not pan. This seems to give a much smoother experience, and I'm not sure that anyone was using this functionality to begin with on very low-res devices. We don't want to disable this anywhere other than B2G.

I still need to talk with someone in UX, so I'm not going to push this with r+ until they say it's ok. I'm also going to attach some videos for the before and after.
Assignee: nobody → bugzilla
Attachment #8361432 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8361912 - Flags: review?(bugmail.mozilla)
Attached video Video without patch (obsolete) —
Attached video Video without patch
Please try to ignore the OOM (black screen) that happens a couple of times in this video. It has nothing to do with the problem.
Attachment #8361913 - Attachment is obsolete: true
Attached video Video with patch
Hi Doug, long time no talk. Glad we persuaded you to join full time! So this sounds promising, and I'll check out the video next week (just leaving for US long weekend) but we'd really need to handle it before R+'ing, and we should talk to some of the other leads devs involved with browser work (Ben, Vivien, etc) about the change as well, just to ensure we're not clashing with other planned browser changes. I'll arrange for you to show this to one of the Toronto UX team members. Will follow up in email thread.
Something that's definitely not helping with this is the following float-to-int truncation:
http://mxr.mozilla.org/mozilla-central/source/widget/gonk/nsAppShell.cpp#212

Ideally, we should never be rounding or truncating this, but that would take quite a while to do. At the very least, we should round it instead of truncating it.

Asking r=mwu? based on work on the surrounding code.
Attachment #8362026 - Flags: review?(mwu)
(In reply to Josh Carpenter [:jcarpenter] from comment #5)
> Hi Doug, long time no talk. Glad we persuaded you to join full time! So this
> sounds promising, and I'll check out the video next week (just leaving for
> US long weekend) but we'd really need to handle it before R+'ing, and we
> should talk to some of the other leads devs involved with browser work (Ben,
> Vivien, etc) about the change as well, just to ensure we're not clashing
> with other planned browser changes. I'll arrange for you to show this to one
> of the Toronto UX team members. Will follow up in email thread.

Great, thanks! I saw you in SF yesterday, I should have stopped by and said hi.
Kev Grandon and I will flash a device with this next week and provide UX feedback. Playing with P2Z behavior on competing devices, my subjective experience is that I do drag slightly when performing zoom operations. That ability, even if it is only subtly used, reinforces the "dragging a physical sheet" touch paradigm. I worry that losing it would feel wrong to users. That said, if the perf is leaps and bounds better, and there is no way to improve the perf with the setting left on, then perhaps it is worth the trade off. Will follow up next week.
(In reply to Doug Sherk (:drs) from comment #7)
> (In reply to Josh Carpenter [:jcarpenter] from comment #5)
> > Hi Doug, long time no talk. Glad we persuaded you to join full time! So this
> > sounds promising, and I'll check out the video next week (just leaving for
> > US long weekend) but we'd really need to handle it before R+'ing, and we
> > should talk to some of the other leads devs involved with browser work (Ben,
> > Vivien, etc) about the change as well, just to ensure we're not clashing
> > with other planned browser changes. I'll arrange for you to show this to one
> > of the Toronto UX team members. Will follow up in email thread.
> 
> Great, thanks! I saw you in SF yesterday, I should have stopped by and said
> hi.

Darn! I probably saw you and totally brain farted because I thought you weren't supposed to be here. :)
(In reply to Josh Carpenter [:jcarpenter] from comment #8)
> Kev Grandon and I will flash a device with this next week and provide UX
> feedback. Playing with P2Z behavior on competing devices, my subjective
> experience is that I do drag slightly when performing zoom operations. That
> ability, even if it is only subtly used, reinforces the "dragging a physical
> sheet" touch paradigm. I worry that losing it would feel wrong to users.
> That said, if the perf is leaps and bounds better, and there is no way to
> improve the perf with the setting left on, then perhaps it is worth the
> trade off. Will follow up next week.

Yeah, makes sense. I did notice that, when trying it out on my Hamachi, the dragging barely works at all; the maximum amount of distance I was able to get with it is about an inch on the screen before it loses that it's in a pinch.

I've been messing around with it with this patch applied and it hasn't really bothered me not to have that functionality. Then again, confirmation bias.

It's entirely up to you guys. I wasn't going to push it even once I get r+ until you let me know what you decide on. Let me know if you guys have any questions/comments.
Comment on attachment 8361912 [details] [diff] [review]
Disable APZC's changing focus points.

Dropping review for now until UX weighs in. No point in my reviewing if they don't want the change in behaviour.
Attachment #8361912 - Flags: review?(bugmail.mozilla)
Comment on attachment 8362026 [details] [diff] [review]
round-gonk-touches.patch

r=me cuz this seems right, though it does nothing to fix the actual bug.
Attachment #8362026 - Flags: review?(mwu) → review+
Gentle poke.

Note that we could do something like disable pinch dragging only on low-res devices, perhaps below a certain DPI threshold.
Flags: needinfo?(jcarpenter)
I'm going to close this bug since a patch landed a long time ago and it'll get really confusing with version tracking if we reuse this bug for other patches. Leaving the needinfo for now but if any new issues come up please open a new bug for it.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla29
Flags: needinfo?(jcarpenter)
You need to log in before you can comment on or make changes to this bug.