727443 - Do not fire a click when the user stops the kinetic panning by touching the screen

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

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

Attachment: Patch

This also fix some others logics problems and check |content.getComputedStyle| to fix an error when clicking on some input fields.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 597399 [details] [diff] [review]
Patch

Please separate orthogonal changes into separate patches in the
future.  Makes review easier.

>diff --git a/b2g/chrome/content/webapi.js b/b2g/chrome/content/webapi.js

>   onTouchStart: function cp_onTouchStart(evt) {
>     this.dragging = true;
>-    KineticPanning.stop();
>+    if (KineticPanning.active) {
>+      KineticPanning.stop();
>+      this.preventNextClick = true;
>+    }

It took me a while to figure out what this is checking for. AFAICT
it's for the user tapping their finger down to stop a pan animation
that continues after a "fling".  Is that right?

Please add a comment.

>+    let pan = KineticPanning.isPan();
>+    if (evt.detail && (pan || this.preventNextClick))
>+      evt.target.addEventListener('click', this, true);
>+

The meaning of |evt.detail| here isn't very clear to me.  Please keep
the previous comment.

>   getPannable: function cp_getPannable(node) {

>     let content = node.ownerDocument.defaultView;
>+    if (!content.getComputedStyle)
>+      return null;
> 

What is this fixing?  I don't know what this idiom means, please add a
comment.

>+  get active() {
>+    return !!this.target;

I think |!== null| is what you want.

r=me with those fixes, as far as I understand them.

Comment 2

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

Attachment: Patch for check-in

Comment 3

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

(In reply to Chris Jones [:cjones] [:warhammer] from comment #1)
> Comment on attachment 597399 [details] [diff] [review]
> Patch
> 
> Please separate orthogonal changes into separate patches in the
> future.  Makes review easier.
>

Sorry for that.

I have uploaded the version to push. Your understanding of the patch was correct and I have removed the content.getComputedStyle null check.

Comment 4

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/30939bc8aa92

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/30939bc8aa92