Closed Bug 727443 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
ProductDemo

People

(Reporter: vingtetun, Assigned: vingtetun)

References

Details

Attachments

(2 files)

Attached patch PatchSplinter Review
This also fix some others logics problems and check |content.getComputedStyle| to fix an error when clicking on some input fields.
Attachment #597399 - Flags: review?(jones.chris.g)
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.
Attachment #597399 - Flags: review?(jones.chris.g) → review+
Assignee: nobody → 21
Status: NEW → ASSIGNED
(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.
https://hg.mozilla.org/mozilla-central/rev/30939bc8aa92
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.