Closed
Bug 926389
Opened 11 years ago
Closed 11 years ago
No handling of touch events for fluffing
Categories
(Core :: DOM: UI Events & Focus Handling, defect)
Tracking
()
RESOLVED
FIXED
mozilla27
People
(Reporter: gerard-majax, Assigned: gerard-majax)
References
Details
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
gerard-majax
:
review+
|
Details | Diff | Splinter Review |
We only have HasMouseListener(content), we need HasTouchListener(content).
Comment 1•11 years ago
|
||
Could you explain what this is about?
Assignee | ||
Comment 2•11 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).
Comment 3•11 years ago
|
||
Assignee | ||
Comment 4•11 years ago
|
||
Please find attached a patch that adds touch in event fluffing clickable detection.
Assignee: nobody → lissyx+mozillians
Attachment #817122 -
Flags: review?(21)
Attachment #817122 -
Flags: review?(21) → review?(roc)
Assignee | ||
Comment 5•11 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•11 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•11 years ago
|
||
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•11 years ago
|
||
Addressing the latest changed by using correct variables names
Attachment #817829 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #818374 -
Flags: review+
Assignee | ||
Comment 11•11 years ago
|
||
Adding also the r=roc in the commit title
Attachment #818374 -
Attachment is obsolete: true
Attachment #818376 -
Flags: review+
Assignee | ||
Comment 12•11 years ago
|
||
And forgot to put this in hg format
Attachment #818376 -
Attachment is obsolete: true
Attachment #818377 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 13•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Updated•6 years ago
|
Component: Event Handling → User events and focus handling
You need to log in
before you can comment on or make changes to this bug.
Description
•