926389 - No handling of touch events for fluffing

Status: RESOLVED FIXED

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

Description

[:gerard-majax]

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

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Could you explain what this is about?

Comment 2

[:gerard-majax]

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

[Olli Pettay [:smaug][[e-mail]]]

Ah, ok, http://mxr.mozilla.org/mozilla-central/source/layout/base/PositionedEventTargeting.cpp#119
is the key.

Comment 4

[:gerard-majax]

Attachment: 0001-Bug-926389-Add-touch-event-handling-during-fluffing.patch

Please find attached a patch that adds touch in event fluffing clickable detection.

Comment 5

[:gerard-majax]

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 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 7

[:gerard-majax]

(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 :(.

Comment 8

[:gerard-majax]

Attachment: 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.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 10

[:gerard-majax]

Attachment: 0001-Bug-926389-Add-touch-event-handling-during-fluffing.patch

Addressing the latest changed by using correct variables names

Comment 11

[:gerard-majax]

Attachment: 0001-Bug-926389-Add-touch-event-handling-during-fluffing-.patch

Adding also the r=roc in the commit title

Comment 12

[:gerard-majax]

Attachment: 0001-Bug-926389-Add-touch-event-handling-during-fluffing-_hg.patch

And forgot to put this in hg format

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cbdae9e8c229