Closed Bug 926389 Opened 7 years ago Closed 7 years ago

No handling of touch events for fluffing

Categories

(Core :: DOM: UI Events & Focus Handling, defect)

26 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: gerard-majax, Assigned: gerard-majax)

References

Details

Attachments

(1 file, 4 obsolete files)

We only have HasMouseListener(content), we need HasTouchListener(content).
Could you explain what this is about?
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).
Please find attached a patch that adds touch in event fluffing clickable detection.
Assignee: nobody → lissyx+mozillians
Attachment #817122 - Flags: review?(21)
Blocks: 921928
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-
(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 :(.
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+
Addressing the latest changed by using correct variables names
Attachment #817829 - Attachment is obsolete: true
Attachment #818374 - Flags: review+
Adding also the r=roc in the commit title
Attachment #818374 - Attachment is obsolete: true
Attachment #818376 - Flags: review+
And forgot to put this in hg format
Attachment #818376 - Attachment is obsolete: true
Attachment #818377 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cbdae9e8c229
Status: NEW → RESOLVED
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.