No handling of touch events for fluffing

RESOLVED FIXED in mozilla27

Status

()

Core
Event Handling
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: gerard, Assigned: gerard)

Tracking

26 Branch
mozilla27
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

5 years ago
We only have HasMouseListener(content), we need HasTouchListener(content).

Comment 1

5 years ago
Could you explain what this is about?
(Assignee)

Comment 2

5 years ago
Some magical stuff :)

More seriously, Vivien noticed (when we were looking through bug 921928) that we were only handling mouse events in the fluffing code, and not touch events. Those have an impact for this specific bug, and the fix is quite trivial (I'm just new to writing and testing with mochitests).
(Assignee)

Comment 4

5 years ago
Created attachment 817122 [details] [diff] [review]
0001-Bug-926389-Add-touch-event-handling-during-fluffing.patch

Please find attached a patch that adds touch in event fluffing clickable detection.
Assignee: nobody → lissyx+mozillians
Attachment #817122 - Flags: review?(21)
(Assignee)

Updated

5 years ago
Blocks: 921928
(Assignee)

Comment 5

5 years ago
I've added bug 921928 as depending on this one, because when testing bug 921928, we noticed that the feedback was better with the present bug included.
Comment on attachment 817122 [details] [diff] [review]
0001-Bug-926389-Add-touch-event-handling-during-fluffing.patch

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

::: layout/base/PositionedEventTargeting.cpp
@@ +137,5 @@
> +    return false;
> +  }
> +
> +  Preferences::AddIntVarCache(&touchEventsEnabled,
> +    "dom.w3c_touch_events.enabled", touchEventsEnabled);

Do you really want to call AddIntVarCache on every single call to this function?
Attachment #817122 - Flags: review?(roc) → review-
(Assignee)

Comment 7

5 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> Comment on attachment 817122 [details] [diff] [review]
> 0001-Bug-926389-Add-touch-event-handling-during-fluffing.patch
> 
> Review of attachment 817122 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/PositionedEventTargeting.cpp
> @@ +137,5 @@
> > +    return false;
> > +  }
> > +
> > +  Preferences::AddIntVarCache(&touchEventsEnabled,
> > +    "dom.w3c_touch_events.enabled", touchEventsEnabled);
> 
> Do you really want to call AddIntVarCache on every single call to this
> function?

Not at all, you are absolutely right, this is quite dumb :(.
(Assignee)

Comment 8

5 years ago
Created attachment 817829 [details] [diff] [review]
0001-Bug-926389-Add-touch-event-handling-during-fluffing.patch

Please find a new version of the patch where we only go through the Preferences::AddIntVarCache() call once.
Attachment #817122 - Attachment is obsolete: true
Attachment #817829 - Flags: review?(roc)
Comment on attachment 817829 [details] [diff] [review]
0001-Bug-926389-Add-touch-event-handling-during-fluffing.patch

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

Excellent!!!

::: layout/base/PositionedEventTargeting.cpp
@@ +127,5 @@
>           elm->HasListenersFor(nsGkAtoms::onmouseup);
>  }
>  
> +static bool touchEventsRegistered = false;
> +static int32_t touchEventsEnabled = 0;

Call these gTouchEventsRegistered and gTouchEventsEnabled to follow naming conventions.
Attachment #817829 - Flags: review?(roc) → review+
(Assignee)

Comment 10

5 years ago
Created attachment 818374 [details] [diff] [review]
0001-Bug-926389-Add-touch-event-handling-during-fluffing.patch

Addressing the latest changed by using correct variables names
Attachment #817829 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #818374 - Flags: review+
(Assignee)

Comment 11

5 years ago
Created attachment 818376 [details] [diff] [review]
0001-Bug-926389-Add-touch-event-handling-during-fluffing-.patch

Adding also the r=roc in the commit title
Attachment #818374 - Attachment is obsolete: true
Attachment #818376 - Flags: review+
(Assignee)

Comment 12

5 years ago
Created attachment 818377 [details] [diff] [review]
0001-Bug-926389-Add-touch-event-handling-during-fluffing-_hg.patch

And forgot to put this in hg format
Attachment #818376 - Attachment is obsolete: true
Attachment #818377 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cbdae9e8c229
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Blocks: 789358
You need to log in before you can comment on or make changes to this bug.