1439013 - Add gesture information to NavigationDelegate.onLoadRequest()

Status: RESOLVED FIXED

(GeckoView :: General, enhancement, P1)

Description

[Emily Kager [:ekager]]

We use this method to determine whether a user has actively clicked away from the search page with request.hasGesture(). Anything that would do the equivalent would work for the callback, but only checking if the URL changed does not work through redirects. 

Relevant WebView method:
https://developer.android.com/reference/android/webkit/WebViewClient.html#shouldInterceptRequest(android.webkit.WebView,%20android.webkit.WebResourceRequest)

Where we use this in Focus:
https://github.com/mozilla-mobile/focus-android/issues/2103

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Can you give an example for "only checking if the URL changed does not work through redirects"? Do you mean if the search engine redirects we would clear the search terms prematurely?

Comment 2

[Chris Peterson [:cpeterson]]

(In reply to Jim Chen [:jchen] [:darchons] from comment #1)
> Can you give an example for "only checking if the URL changed does not work
> through redirects"? Do you mean if the search engine redirects we would
> clear the search terms prematurely?

needinfo'ing Emily for Jim's question ^

Comment 3

[Emily Kager [:ekager]]

Yes exactly, Jim. I believe there are cases where a search would redirect (maybe for a locale) so we can't just go off of the URL changing, and we want to record when the use actively navigates away from the search results. 

We added this to Focus here https://github.com/mozilla-mobile/focus-android/commit/644d0c0b7e45d80e9827b7c02ca64e738475bf95

Comment 4

[Emily Kager [:ekager]]

I have also found another instance where this method parity with WebView would be useful.
 
We currently are detecting live changes in the URL including redirects in onLocationChange for GeckoView but this method also picks up the web content URIs as well. Meaning sometimes the URL is displaying strange .svg or .jpg URLs. 

In WebView, we are able to detect if the request is for the main frame in shouldInterceptRequest and update the URL accordingly.

Relevant WebView method: https://developer.android.com/reference/android/webkit/WebResourceRequest.html#isForMainFrame()

GitHub discussion: https://github.com/mozilla-mobile/focus-android/issues/2156

Where we use this WebView method in Focus: https://github.com/mozilla-mobile/focus-android/blob/master/app/src/webview/java/org/mozilla/focus/webview/FocusWebViewClient.java#L117

Comment 5

[Chris Peterson [:cpeterson]]

P2 because Barbara says that this is not a ship blocker for Klar+GeckoView but this is a user-facing bug.

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

Emily, I changed the summary to (AFAICT) better reflect what we need to do here. If that seems wrong to you, let me know!

I also cloned this to bug 1444138 for the busted onLocationChange() behavior you described.

Comment 7

[Emily Kager [:ekager]]

Yes the summary is perfect, thanks!

Comment 8

[Chris Peterson [:cpeterson]]

P1 because not fixing this bug would be a UX regression from Klar+WebView.

Comment 9

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1439013-1.0-Pass-isUserTriggered-up-via-an-inter.patch

Gecko currently doesn't expose isHandlingUserInput or related flags in event callbacks.

To support this feature in GeckoView, we could pass the isUserTriggered flag from nsContentUtils::TriggerLink all the way up to "Geckoview:OnLoadRequest" through nsDocShell::InternalLoad using an internal load flag.

Comment 10

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

Comment on attachment 8970206 [details] [diff] [review]
0001-Bug-1439013-1.0-Pass-isUserTriggered-up-via-an-inter.patch

I don't think this does what you want since 
nsContentUtils::TriggerLink gets aIsUserTriggered==true even when user hasn't triggered anything, as far as I see. 
Something somewhere probably should use EventStateManager::IsHandlingUserInput();
But note, that has different meaning than blinks "user gesture"

Comment 11

[Eugen Sawin [:esawin]]

> I don't think this does what you want since 
> nsContentUtils::TriggerLink gets aIsUserTriggered==true even when user
> hasn't triggered anything, as far as I see.

For historic context, the IsUserTriggered argument had been introduced in bug 41907 for auto-XLink support and then has been made redundant in bug 516906 by dropping XLink support without removing the argument.

At this point, we can remove the argument and default to nsIScriptSecurityManager::STANDARD in nsContentUtils::TriggerLink.

> Something somewhere probably should use
> EventStateManager::IsHandlingUserInput();

How about passing down the EventStateManager::IsHandlingUserInput() value from nsContentUtils to the nsILinkHandler? Or would setting it in the link handler be more appropriate?

> But note, that has different meaning than blinks "user gesture"

I think that's the closest thing we can currently expose, it would be passed up as an IsHandlingUserInput flag to the onLoadRequest handler.

Comment 12

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

Passing the value to nsILinkHandler sounds reasonable.

Comment 13

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1439013-1.1-Add-isUserTriggered-argument-to-nsIL.patch

With this we pass EventStateManager::isHandlingUserInput() to nsILinkHandler instead of relying on the isUserTriggered argument passed from Element.

Do we want to remove the redundant original isUserTriggered argument?

Comment 14

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

Comment on attachment 8970887 [details] [diff] [review]
0001-Bug-1439013-1.1-Add-isUserTriggered-argument-to-nsIL.patch

>     rv = linkHandler->OnLinkClickSync(this, actionURI,
>                                       target.get(),
>                                       VoidString(),
>                                       postDataStream, postDataStreamLength,
>                                       nullptr, false,
>                                       getter_AddRefs(docShell),
>-                                      getter_AddRefs(mSubmittingRequest));
>+                                      getter_AddRefs(mSubmittingRequest),
>+                                      EventStateManager::IsHandlingUserInput()
>+                                      );
Nit, I'd keep ); at the end of previous line

Comment 15

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1439013-1.2-Add-isUserTriggered-argument-to-nsIL.patch

Addressed comment.

Comment 16

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1439013-2.0-Add-onLoadRequest-load-flags.-r-snor.patch

This adds load request flags to onLoadRequest, currently with the only flag being LOAD_REQUEST_IS_USER_TRIGGERED. We might have more to add in future.

Comment 17

[Eugen Sawin [:esawin]]

Comment on attachment 8971329 [details] [diff] [review]
0002-Bug-1439013-2.0-Add-onLoadRequest-load-flags.-r-snor.patch

Emily, would this API be adequate to resolve your issue?

We extend NavigationDelegate.onLoadRequest by adding a load request flags argument, where the value LOAD_REQUEST_IS_USER_TRIGGERED signals that the load request was initiated by user input.

Comment 18

[Emily Kager [:ekager]]

This definitely looks like it would work! Excited to try it out!

Comment 19

[Dylan Roeh (:droeh) (he/him)]

Comment on attachment 8971329 [details] [diff] [review]
0002-Bug-1439013-2.0-Add-onLoadRequest-load-flags.-r-snor.patch

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

Looks good to me.

Comment 20

[Eugen Sawin [:esawin]]

Attachment: 0003-Bug-1439013-3.0-Update-tests-to-use-new-onLoadReques.patch

Let's try to get all tests updated before landing this time :)

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e03927d418ba
[1.2] Add isUserTriggered argument to nsILinkHandler and expose it as an internal load flag. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/a26881251c96
[2.0] Add onLoadRequest load flags. r=snorp,droeh
https://hg.mozilla.org/integration/mozilla-inbound/rev/653f16ce003a
[3.0] Update tests to use new onLoadRequest interface. r=jchen

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc0d4ccef976
Fix assertion. r=me

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/23d3d208fcaa
Fix assertion for real. r=me

Comment 24

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/e03927d418ba
https://hg.mozilla.org/mozilla-central/rev/a26881251c96
https://hg.mozilla.org/mozilla-central/rev/653f16ce003a
https://hg.mozilla.org/mozilla-central/rev/dc0d4ccef976
https://hg.mozilla.org/mozilla-central/rev/23d3d208fcaa