Add gesture information to NavigationDelegate.onLoadRequest()

RESOLVED FIXED in Firefox 61

Status

enhancement
P1
normal
RESOLVED FIXED
Last year
7 months ago

People

(Reporter: ekager, Assigned: esawin)

Tracking

(Blocks 1 bug)

unspecified
mozilla61
Dependency tree / graph

Firefox Tracking Flags

(firefox58 wontfix, firefox59 wontfix, firefox60 wontfix, firefox61 fixed)

Details

(Whiteboard: [geckoview:klar])

Attachments

(3 attachments, 2 obsolete attachments)

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
Whiteboard: [geckoview:klar]
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?
(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 ^
Flags: needinfo?(ekager)
Priority: -- → P1
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
Flags: needinfo?(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
P2 because Barbara says that this is not a ship blocker for Klar+GeckoView but this is a user-facing bug.
Priority: P1 → P2
Summary: Consider adding shouldInterceptRequest method to GeckoView → Add gesture information to NavigationDelegate.onLoadRequest()
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.
Yes the summary is perfect, thanks!
P1 because not fixing this bug would be a UX regression from Klar+WebView.
Priority: P2 → P1
Assignee: nobody → esawin
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.
Attachment #8970206 - Flags: review?(bugs)
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"
Attachment #8970206 - Flags: review?(bugs) → review-
> 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.
Flags: needinfo?(bugs)
Passing the value to nsILinkHandler sounds reasonable.
Flags: needinfo?(bugs)
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?
Attachment #8970206 - Attachment is obsolete: true
Attachment #8970887 - Flags: review?(bugs)
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
Attachment #8970887 - Flags: review?(bugs) → review+
Addressed comment.
Attachment #8970887 - Attachment is obsolete: true
Attachment #8971328 - Flags: review+
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.
Attachment #8971329 - Flags: review?(snorp)
Attachment #8971329 - Flags: review?(droeh)
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.
Attachment #8971329 - Flags: feedback?(ekager)
This definitely looks like it would work! Excited to try it out!
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.
Attachment #8971329 - Flags: review?(droeh) → review+
Let's try to get all tests updated before landing this time :)
Attachment #8971463 - Flags: review?(nchen)
Attachment #8971463 - Flags: review?(nchen) → review+
Pushed by esawin@mozilla.com:
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
Attachment #8971329 - Flags: feedback?(ekager) → feedback+
Blocks: 1487542
No longer blocks: 1487542
Depends on: 1487542
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 61 → mozilla61
You need to log in before you can comment on or make changes to this bug.