14.93 KB, patch
|Details | Diff | Splinter Review|
8.99 KB, patch
|Details | Diff | Splinter Review|
9.94 KB, patch
|Details | Diff | Splinter Review|
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
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 ^
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
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.
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.
Passing the value to nsILinkHandler sounds reasonable.
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 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+
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) → review+
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 firstname.lastname@example.org: 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
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/dc0d4ccef976 Fix assertion. r=me
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/23d3d208fcaa Fix assertion for real. r=me
Attachment #8971329 - Flags: feedback?(ekager) → feedback+
You need to log in before you can comment on or make changes to this bug.