Bug 94349 – [Chromium] detectContentIntentAround has a misleading name

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Leandro Graciá Gil]

The current WebViewClient API exposes a method named detectContentIntentAround. However, this is not designed to detect any "content intent", but to allow the embedder to detect content around a position and fire Android intents if appropriate.

Also, content detection should not be triggered if the touched node listens to events that consume the tap action, like clicks, touch events or mouse events.

[Leandro Graciá Gil]

Created attachment 159126 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Leandro Graciá Gil]

Created attachment 159142 [details]
Patch

[Adam Barth]

Here's a related comment I made on Bug 92346:

The issue here is that we're trying to draw a distinction between the intents from the Android system and Web Intents.  I looked at the documentation for the Intents in Android and learned that the full name was "android.content.intent" hence the name "content intent" as opposed to "web intent".  If that's a misnomer, we should fix it.

[Adam Barth]

Comment on attachment 159142 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159142&action=review

> Source/WebKit/chromium/public/WebViewClient.h:378
> +    virtual WebContentDetectionResult detectContentAround(const WebHitTestResult&) { return WebContentDetectionResult(); }

Should we rename scheduleContentIntent and cancelScheduledContentIntents as well?  I'm surprised that the name "detectContentAround" doesn't have the word "intent" in it.  Is this used for anything besides intents?

> Source/WebKit/chromium/src/WebViewImpl.cpp:3879
> +    for (; node && !node->hasTagName(HTMLNames::bodyTag); node = node->parentNode()) {
> +        if (node->isLink() || (touchType != WebInputEvent::GestureLongPress && (node->hasEventListeners(eventNames().clickEvent)
> +            || node->hasEventListeners(eventNames().touchstartEvent) || node->hasEventListeners(eventNames().touchendEvent)
> +            || node->hasEventListeners(eventNames().mousedownEvent) || node->hasEventListeners(eventNames().mouseupEvent)))) {
> +            return false;
> +        }
> +    }

I didn't upstreaming this part because it's not clear to me whether this is the right way to do this.  Can you explain a bit more what you're trying to accomplish with this code.

[Leandro Graciá Gil]

Comment on attachment 159142 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159142&action=review

>> Source/WebKit/chromium/public/WebViewClient.h:378
>> +    virtual WebContentDetectionResult detectContentAround(const WebHitTestResult&) { return WebContentDetectionResult(); }
> 
> Should we rename scheduleContentIntent and cancelScheduledContentIntents as well?  I'm surprised that the name "detectContentAround" doesn't have the word "intent" in it.  Is this used for anything besides intents?

scheduleContentIntent and cancelScheduledContentIntents do what they say: they request to generate an Android intent based on the provided URL.

On the other hand, detectContentAround doesn't detect intents, but might produce their URLs as part of the WebContentDetectionResult object. We might add "intent" to the name if you have a suggestion, but definitely not as what we're trying to detect.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:3879
>> +    }
> 
> I didn't upstreaming this part because it's not clear to me whether this is the right way to do this.  Can you explain a bit more what you're trying to accomplish with this code.

This is trying to prevent content detection to interfere with any custom behaviour. Since tapping on content might fire an Android intent that can severely disrupt the experience in some sites that expected to handle the tap themselves. For example, if we tap on content that is in a link we want to follow the link, not to fire an intent. The same happens if the content we touch has for example a click listener: we want these cases to work as originally expected. That's why we explicitly check for these events.

Also you will see a reference to the body tag. This is because our testing has shown that many pages exposing content (including Google's) have, for whatever reason, a click listener in the body. We never found a case where this listener was related to the content itself in any way that could be disruptive, but still it completely prevented content detection to work in these sites. That's the reason of that check there.

[Adam Barth]

I wonder if that's a consequence of calling this function before processing the event in WebCore.  Instead we might want to call this function in the default event handler, after the page has had a chance to see the event and call prevent default.

[Leandro Graciá Gil]

(In reply to comment #7)
> I wonder if that's a consequence of calling this function before processing the event in WebCore.  Instead we might want to call this function in the default event handler, after the page has had a chance to see the event and call prevent default.

But if we do that then we will have the problem with the <body> click listeners.

[Adam Barth]

> But if we do that then we will have the problem with the <body> click listeners.

That depends if they call preventDefault.  Do you have examples of sites that caused problems?  We can check to see if they preventDefault.

[Leandro Graciá Gil]

(In reply to comment #9)
> > But if we do that then we will have the problem with the <body> click listeners.
> 
> That depends if they call preventDefault.  Do you have examples of sites that caused problems?  We can check to see if they preventDefault.

Here's one classical example: http://www.google.com/about/company/facts/locations/
I know testing found more cases and that eventually led to this body listener exception, but I don't have other urls at hand right now.

However, we should not rely on the page calling preventDefault or any other method at all as what we want is not to disrupt its normal behaviour with the content detection feature.

[Adam Barth]

> However, we should not rely on the page calling preventDefault or any other method at all as what we want is not to disrupt its normal behaviour with the content detection feature.

In this case, we would perform this default action if they did *not* call preventDefault.

[Adam Barth]

Comment on attachment 159142 [details]
Patch

Ok.  What I'd like to do here is to land this patch in two pieces:

1) The renaming, which is uncontroversial.
2) The loop in detectContentOnTouch.

Notice that this logic here is simliar to the logic related to isEventNode in <https://bugs.webkit.org/attachment.cgi?id=158702&action=review>.  It's likely these two functions should share some of this logic since they're both trying to figure out if something reacts to touching.

[Adam Barth]

Created attachment 159488 [details]
Patch for landing

[Adam Barth]

I spoke with rniwa and he think that the approach of checking for event listeners is fine.  Would you be willing to post that part of the patch in a separate bug?  We should also try to do that in a way that shares code with the patch in Bug 94182.

[Leandro Graciá Gil]

(In reply to comment #14)
> I spoke with rniwa and he think that the approach of checking for event listeners is fine.  Would you be willing to post that part of the patch in a separate bug?  We should also try to do that in a way that shares code with the patch in Bug 94182.

Sure, I'll do that.

[Adam Barth]

Many thanks.

[WebKit Review Bot]

Comment on attachment 159488 [details]
Patch for landing

Clearing flags on attachment: 159488

Committed r126061: <http://trac.webkit.org/changeset/126061>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.