1181763 - [Event fluffing] Clickable children should be prioritized over their parents

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Julien Wajsberg [:julienw]]

In Firefox OS sms application, it's sometimes difficult to tap the 'send' button because its container has a click event too.

I don't understand why the code in [1] is useful, because this favors a container if we tap near one of its clickable children.

[1] https://dxr.mozilla.org/mozilla-central/source/layout/base/PositionedEventTargeting.cpp#506-512

Comment 1

[Julien Wajsberg [:julienw]]

Attachment: 0001-Bug-1181763-Event-fluffing-Clickable-children-should.patch

Comment 2

[Julien Wajsberg [:julienw]]

(See also the testcase in https://bug946731.bugzilla.mozilla.org/attachment.cgi?id=8595972)

Comment 3

[Julien Wajsberg [:julienw]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=38c7d2abe807
(hope I got it right)

Comment 4

[Julien Wajsberg [:julienw]]

Comment on attachment 8631237 [details] [diff] [review]
0001-Bug-1181763-Event-fluffing-Clickable-children-should.patch

Hey roc, what do you think of this simple patch?

This is only the easiest part of my older patch from bug 946731 that you already f+ some months ago ;)

Comment 5

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

Comment on attachment 8631237 [details] [diff] [review]
0001-Bug-1181763-Event-fluffing-Clickable-children-should.patch

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

kats reviewed the patch that added this code.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

The purpose of the code is that if you directly click on something that is clickable, we don't do any retargeting (unless it is clickable because of a listener at the body or html level). Retargeting only happens if you click on something that is not directly clickable. I'm not sure we want to remove this behaviour because it will lead to users clicking directly on one thing but activating something else.

If you have a situation where there is a clickable element inside a clickable container and the user is accidentally hitting the container instead of the element, maybe the element needs to be made bigger and easier to hit?

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Also, not sure if the listeners you have registered are touch listeners or mouse listeners, but bug 1091889 may be relevant here.

Comment 8

[Julien Wajsberg [:julienw]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)
> The purpose of the code is that if you directly click on something that is
> clickable, we don't do any retargeting (unless it is clickable because of a
> listener at the body or html level). Retargeting only happens if you click
> on something that is not directly clickable. I'm not sure we want to remove
> this behaviour because it will lead to users clicking directly on one thing
> but activating something else.

The only behavior change is when the user clicks near a child. My logic here is that if the user clicks close enough to a child, then chances are that he really wanted this child. The logic should not be different whether he clicked in a clickable container, or a non-clickable container.

Actually I can't really think of a case where this would be desirable. But clearly I know a lot of ugly things that I can't imagine can happen on the Web :)

> 
> If you have a situation where there is a clickable element inside a
> clickable container and the user is accidentally hitting the container
> instead of the element, maybe the element needs to be made bigger and easier
> to hit?

Why would this be different if the element is in a clickable container vs if the element is in non-clickable container ?

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> Also, not sure if the listeners you have registered are touch listeners or
> mouse listeners, but bug 1091889 may be relevant here.

We use only "click" events in our case.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

(In reply to Julien Wajsberg [:julienw] from comment #8)
> Why would this be different if the element is in a clickable container vs if
> the element is in non-clickable container ?
> 

Because it has the potential to end up making the container completely unclickable. Consider this (exaggerated) case:

data:text/html,<div style="width:90px; height:90px; background-color: red; padding: 5px;"><div style="width: 90px; height: 90px; background-color: green">

If both divs have click listeners on them, then your patch will make it impossible to click the red area because even if the user taps directly on the narrow strip the green thing will steal the clicks. In my exaggerated case it's obviously bad design on the part of the author but I'm sure there are places where this sort of pattern occurs on the web legitimately, or perhaps even accidentally.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

To put it another way, the fluffing code is not meant to involve tradeoffs - it's meant to make things strictly easier to click, without any loss in correctness. In my example above, with your patch, we're trading off easier clickability on the green area for harder clickability on the red area, and potentially making it unclickable entirely. That's breaking the assumptions of the fluffing code. Does that make sense?

Comment 11

[Julien Wajsberg [:julienw]]

But wouldn't the "cluster" logic help in such cases ? I know it's not implemented in Firefox OS yet, but when there is uncertainty, that's when the "cluster" logic should shine.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Yes, that's true. And with the current code, I believe clicking on the red area would in fact return true for hitting a cluster, so Firefox OS supported some UI for it, it would totally solve this problem. Maybe that's the correct final solution here.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

(Actually with the current code it wouldn't return true for hitting a cluster. But it should, and we can fix that)

Comment 14

[Julien Wajsberg [:julienw]]

> If you have a situation where there is a clickable element inside a clickable 
> container and the user is accidentally hitting the container instead of the element, 
> maybe the element needs to be made bigger and easier to hit?

Also, for my app, maybe I could do this (even if it means I have some work to do), but I'm sure there are similar cases on the web that we can't change as easily. For sure there is a trade-off to find here. I believe it's better with my patch, but I think we need to test on real websites during some time to be sure.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Dominique, you've been working with fluffing the most recently, any thoughts on this?

Comment 16

[Dominique Vincent [:domivinc]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #15)
> Dominique, you've been working with fluffing the most recently, any thoughts
> on this?

When a list of links is inside a clickable container, the list of links is not a cluster of links. Whatever the position of the click/touch is, there is no ambiguity on the element receiving the event. All the elements (the links and the container) are clickable. We cannot change the click position and decide that the user wants to click near a link or in the container.

If you want to get the cluster behavior for your links, you have to change the html structure :
1- Move your links inside an html container without any clickable elements in the parent elements.
2- Around this unclickable element, display some clickable areas (top/bottom/right/left positions).
With this structure, the unclickable area around the links, will force the cluster detection when the user clicks near the links.

I didn't find any details in the bug comments about the original html structure with the issue. If you post the html structure or a link, I could try to propose a solution.

Comment 17

[Julien Wajsberg [:julienw]]

Let me summarize the issue I'm trying to solve here.

First, enable event fluffing for the mouse cursor in your firefox by setting the following prefs:

ui.mouse.radius.enabled => true
ui.mouse.radius.inputSource.touchOnly => false

Then navigate to the following testcase:
https://bug946731.bugzilla.mozilla.org/attachment.cgi?id=8595972

And try to hover the red squares using your mouse cursor.

Do you see the issue ?

Now, my take here is that there shouldn't be a difference between the unclickable-container and clickable-container cases. That's what my patch is doing.

So the debate is mostly about this: is this an issue we want to solve?
Kats was saying in comment 6, comment 9, comment 10 that we should not solve this. The main argument is in comment 9: if the clickable container has a small but not empty surface just around a clickable child, the user wouldn't be able to click it. Some code example is:

  data:text/html,<div style="width:90px; height:90px; background-color: red; padding: 5px;"><div style="width: 90px; height: 90px; background-color: green">

If we really want the clickable container to work in such a case, I think that the clustering logic could help to resolve this. And we should fix the initial issue I see. I'm not so sure we should let the clustering logic run in that case though.


In case of the SMS application itself there is no such uncertainty; I definitely want to favor the clickable children over the clickable container in case the user clicks near the clickable children. I can also likely fix it by changing some logic, still I think this is a issue that can happen in the real-world web. That's why I'm determined to change this behavior in Gecko.


Kats, I just thought of something else too. The clickable container's handler would actually run in the capturing or bubbling phase (unless stopPropagation is called in the clickable child's handler). So most of the time we wouldn't even lose this.

Comment 18

[Dominique Vincent [:domivinc]]

Attachment: Test fluffing with clickable background

Thanks Julien for the details.
I still think that fluffing is not the correct behavior in the case of a container with a click handler. In this design, there is no ambiguity on the target. See the second case in the attached file, I added the correct pointer on mouse hover (and a color change) on the container.

If your page needs the fluffing behavior, the design of your page has to be changed (see the third case with gray containers in the attached file). In this case the html/css is more complex, but you will have the desired behavior: a fluffing area around your red targets and a clickable background.

Comment 19

[Julien Wajsberg [:julienw]]

The main issue I have with the current implementation is that it's really usual in real-world to have clickable containers. Not because the author wants the user to click them directly, but because this implements event delegation. In the click handler we can have something like "if (!e.target.matches('.click-link')) { return; }".
This is implemented by jQuery with the "on" function (see [1] how they recommend the pattern).

[1] http://learn.jquery.com/events/event-delegation/


That's why I suggested this change: because event delegation is common and a recommended pattern on the web for a long time (see [2] for a 2006 blog post by Chris Heilmann -- incidentally the very post that made me know about this :) ), while a clickable container with a small surface where it's useful to click is uncommon AND bad practice.

[2] http://christianheilmann.com/2006/09/24/event-handling-versus-event-delegation/

Of course it's possible, as an author, to do otherwise, like you suggest in comment 18. But I don't think it's common on the web. And in the end of the day event fluffing is there to help the user to navigate the existing web, not an ideal web.

That's my issue here: why should we help bad practices instead of good practices ?


Here is what I found on the existing web:
* maps.google.com: the "omnibox" (top-left box with information) uses event delegation. So even with event fluffing the links are not easily clickable. (I used the desktop version here, maybe the mobile version is different)
* maps.google.com: the bottom-right box does not seem to benefit from event fluffing; because of document.body's events ?
* Twitter heavily uses clickable containers. All buttons are difficult to click even with event fluffing: buttons in tweets, top navigation.
* Facebook: basically the same. It's difficult to click on the links for "Like", "Comment", "Share". And on a lot of other icons (like the cross to mask a comment).
* GMail: same for the left menu and the "compose" button, and the top buttons and the threads themselves (for the threads themselves it's less a big deal, but for the various buttons it is). I think they have a "mousedown" event on a container.

As you see, nearly all moderately complex apps suffer from the issue. I also checked that with the patch all these cases are better.
(I still see some issues: for example in GMail the button "More" is not activated when clicking close to the button on the right because there is a visually underlying container that is not parent to the button -- and that is clickable because a common parent is clickable ! I don't think we should fix this this case though, at least not now.)


I think our disagreement comes from this fact: I'm used to event delegation because I use it for a long time now, therefore clickable containers are a common thing to me, but don't mean they're important over their children. While for you, if there is a clickable container, it means the author really wants to have a specific action on it. But I believe this is untrue.


If you still think we should not change this behavior, then I think we can close this bug and bug 946731 WONTFIX. I really hope you'll change your mind though :)

Comment 20

[Julien Wajsberg [:julienw]]

Now that I've seen how widespread the issue is, I'm even more excited about it; I think it's a really nice quick-win hanging fruit that would benefit both Firefox/Android and Firefox OS on real-world cases!

Comment 21

[Dominique Vincent [:domivinc]]

The issue with your fix is that it doesn’t respect the “normal” rule: the user clicks on an element (the container) and the event is done on another element (the small button inside the container for instance).
I share all your points about this kind of designs (a clickable container with buttons inside it).

I think that the best level to fix such cases is on the html code. The designer of the interface has to accept or not the fluffing on the child elements of the clickable container.
In a perfect world, a new html attribute could be proposed to manage correctly the situation. For instance in your html sample code, I would replace the following code:
<a class='small-size' href='#3'></a>
by this one:
<a class='small-size' href='#3' fluffing=’3px’></a>

Until this perfect world, if you really want to provide a solution from the user side (browser options), you can propose to add a flag :
ui.mouse.radius.ignoreClickableContainer.enabled : false
And insert a test of this flag around the code you removed in your patch.

In this case, the user will have to change 3 flags to get your fluffing behavior:
ui.mouse.radius.enabled => true
ui.mouse.radius.inputSource.touchOnly => false
ui.mouse.radius.ignoreClickableContainer.enabled => true

Kats, what do you think about the creation of a new flag (ui.mouse.radius.ignoreClickableContainer) ?

Comment 22

[Kartikaya Gupta (email:[e-mail])]

Attachment: WIP - Restrict target to descendant of directly-clicked element

Thanks for the discussion guys, it was very helpful! Given the existing use of this pattern, as Julien said in comment 19, I agree we do need to do something to handle this scenario. I didn't realize it was such a common pattern. Adding a "fluffing" attribute/more prefs is not something I want to do because it won't help users now - it's a much more long-term change as it will take time for websites to adopt this. And even then I'm not sure it's worth it.

I think we still want to maintain the invariant that "if the user directly clicks on an element X which is clickable, then that element X should receive a click event". However, as Julien said, we should be able to fire the click event on a *descendant* of X and X will still receive the click event due to bubbling. The patch Julien posted is I think too broad because it means if you have e.g. a absolute-positioned clickable item from somewhere else in the DOM hierarchy on top of your clickable container, then it might end up stealing the click, and the event bubbling will not hit the clickable container.

So I have a different variation of his patch which I hope will fix the issue he's seeing but also maintain this invariant. I think that it should be safer than what he has now. I haven't tested (or even compiled) the patch but I'm attaching it here so it illustrates the idea - Julien, if you have time, are you willing to steal this patch and finish it off? (i.e. make sure it compiles, fixes the issues, and update any tests that need updating). If not I can try to find some time to do it but it might not be this week.

Comment 23

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8631237 [details] [diff] [review]
0001-Bug-1181763-Event-fluffing-Clickable-children-should.patch

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

Minusing this patch because it has the issue I described above, where abs-pos items can steal the click into a different part of the DOM. That might result in the directly-clicked container not getting any click event even via bubbling.

Comment 24

[Kartikaya Gupta (email:[e-mail])]

Oh, also the patch I posted should fix bug 1138335, I think. The problem there is related in that the page has a clickable container with a child that is not explicitly clickable. When the user directly clicks on the child, we use the container as the target instead. The JS code uses this same pattern (the click listener on the container checks the .target of the click event) and it's expecting the descendant rather than the container so we end up with incorrect behaviour. Since my patch allows the target of the event to be the child, it should fix the incorrect behaviour.

Comment 25

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

I had some time today so I updated the test cases and commit message. Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=77d867f6bf63 and I need to do some manual testing on it too but otherwise it should be good to go.

Comment 26

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

Slight update to the test. Here's the green try run for this:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9af76054c9f9

Julien, do you mind trying this patch to see if it fixes the problem in the messages app? I tested the reduced case that you attached and it corrected the behaviour there.

Comment 27

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

Whoops, had an unrelated hunk in there.

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec3acc5237a8

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Backed out for WinXP test_event_target_radius.html failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=11830048&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/ecd916f018fd

Comment 30

[Kartikaya Gupta (email:[e-mail])]

I think the problem here is that the WinXP machine has 1mm = 5pixels which is probably higher than the rest of the test machines/platforms. Since the leftmm touch radius is 12mm that results in a 60-pixel radius around t6_inner to grab clicks, and the test I added assumed that 55 pixels would be far enough away from t6_inner. I'll update the test and do a try push to confirm.

Comment 31

[Kartikaya Gupta (email:[e-mail])]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=806791475434

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/285d33e0631d

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/285d33e0631d

Comment 34

[Julien Wajsberg [:julienw]]

Hi, sorry that I was not so available during this week as I needed to focus on something else. Kats, I really appreciate your work here and I'll report about this as soon as I can look at the change on a device with a nightly build.

Comment 35

[Julien Wajsberg [:julienw]]

Comment on attachment 8634672 [details] [diff] [review]
Patch

It's not extra clear because it's quite subjective but I _think_ it improves the situation. We'll see if foxfooders still complain :)

Thanks again.