GeckoView: Expose "history stack" of a session and allow navigating to index

RESOLVED FIXED in Firefox 68

Status

enhancement
P1
normal
RESOLVED FIXED
8 months ago
2 months ago

People

(Reporter: sebastian, Assigned: droeh)

Tracking

unspecified
mozilla68
Unspecified
Android
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 wontfix, firefox65 wontfix, firefox66 wontfix, firefox67 wontfix, firefox68 fixed)

Details

(Whiteboard: [gvtv:p1] [geckoview:fenix:m2], )

Attachments

(2 attachments)

In Android Components and the Reference browser we want to show a bottom panel with the "history" stack of a tab and allow the user to navigate to a specific page in the tab's history.

As an example long press on the "back" button in Fennec.
Screenshot: https://user-images.githubusercontent.com/28052/47804929-6fcf4200-dd0c-11e8-99c1-b42f7c88fe43.jpeg

This bug is more about a discussion around how a consumer of GV can implement such a feature than a concrete API request.

For developing the component we most likely want:

* (1) A history stack with all URLs visited in a session
* (2) An index that points to the position in the stack the session is currently in 
* (3) A way to navigate to a specific index or multiple steps back/forward

For (1) there was some discussion on Slack whether GV should provide this list or whether the app is reposible for maintaining this list itself with the help of the HistoryDelegate. A risk here is that the app list and the list gecko has could get out of sync and then navigating the history would not be possible anymore. WebView offers copyBackForwardList().

For (2) I couldn't find anything in WebView. There's canGoBackOrForward(steps) but you'd have to do some kind of binary search on that to determine the index you are at.

For (3) WebView has goBackOrForward (int steps).
Points: --- → 3
Whiteboard: [geckoview:fenix:p1]
The Fenix team says this is a P2 for Fenix MVP.
Whiteboard: [geckoview:fenix:p1] → [geckoview:fenix:p2]
Priority: -- → P2
When FireTV moves to GeckoView, they would also need this for some custom handling of YoutubeTV links.

See [0] and [1].


[0]: https://github.com/mozilla-mobile/firefox-tv/issues/1542
[1]: https://github.com/mozilla-mobile/firefox-tv/issues/1543
Product: Firefox for Android → GeckoView
Whiteboard: [geckoview:fenix:p2] → [geckoview:fenix:p2] [gvtv]

What do people think about the following API?

interface HistoryDelegate {
    ...
    void onHistoryListUpdate(String[] uris, int currentIndex);
}

class GeckoSession {
    ...
    void goto(int index);
} 

Instead of an array of String in onHistoryListUpdate we could have a HistoryItem which would look a lot like the arguments to HistoryDelegate.onVisited, which looks like:

@Nullable GeckoResult<Boolean> onVisited(@NonNull GeckoSession session,
                                         @NonNull String url,
                                         @Nullable String lastVisitedURL,
                                         @VisitFlags int flags)

In particular, I think the flags would be useful in onHistoryListUpdate. The WebView history item[0] has some stuff like title and favicon, but I don't think those really belong here.

[0] https://developer.android.com/reference/android/webkit/WebHistoryItem

Flags: needinfo?(s.kaspari)

Do we need onVisited if we have onHistoryListUpdate? (I'm assuming lastVisitedURL would be uris[currentIndex + 1] on the new API)

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #3)

What do people think about the following API?

interface HistoryDelegate {
    ...
    void onHistoryListUpdate(String[] uris, int currentIndex);
}

class GeckoSession {
    ...
    void goto(int index);
} 

Instead of an array of String in onHistoryListUpdate we could have a HistoryItem which would look a lot like the arguments to HistoryDelegate.onVisited, which looks like:

@Nullable GeckoResult<Boolean> onVisited(@NonNull GeckoSession session,
                                         @NonNull String url,
                                         @Nullable String lastVisitedURL,
                                         @VisitFlags int flags)

In particular, I think the flags would be useful in onHistoryListUpdate. The WebView history item[0] has some stuff like title and favicon, but I don't think those really belong here.

[0] https://developer.android.com/reference/android/webkit/WebHistoryItem

In my opinion, GeckoSession.goto() is a pretty vague name; WebView uses goBackOrForward which is a bit verbose but a lot clearer. Also, assuming that onHistoryListUpdate and goto use the same indexing scheme, this requires the app to store the current index in order for goto to be useful in most cases; having goto use relative indexing (so that the current history entry is index 0) makes goto a bit more useful on its own.

Also, I know it's come up before, but I think this is a good time to reconsider having canGoBack(), canGoForward(), and maybe something like gotoRange() (which would give the range of valid input for goto) so the app doesn't have the overhead of maintaining these and the possible bugs associated with getting them out of sync. I suppose you could add something like currentHistoryIndex() in here as well to address the indexing issue above.

Points: 3 → 5
OS: Unspecified → Android

Sebastian, does this history API need to be synchronous?

(In reply to Dylan Roeh (:droeh) from comment #5)

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #3)

What do people think about the following API?

interface HistoryDelegate {
    ...
    void onHistoryListUpdate(String[] uris, int currentIndex);
}

class GeckoSession {
    ...
    void goto(int index);
} 

Instead of an array of String in onHistoryListUpdate we could have a HistoryItem which would look a lot like the arguments to HistoryDelegate.onVisited, which looks like:

@Nullable GeckoResult<Boolean> onVisited(@NonNull GeckoSession session,
                                         @NonNull String url,
                                         @Nullable String lastVisitedURL,
                                         @VisitFlags int flags)

In particular, I think the flags would be useful in onHistoryListUpdate. The WebView history item[0] has some stuff like title and favicon, but I don't think those really belong here.

[0] https://developer.android.com/reference/android/webkit/WebHistoryItem

In my opinion, GeckoSession.goto() is a pretty vague name; WebView uses goBackOrForward which is a bit verbose but a lot clearer.

Yeah. Maybe gotoHistoryIndex? I guess goBackOrForward works too.

Also, assuming that onHistoryListUpdate and goto use the same indexing scheme, this requires the app to store the current index in order for goto to be useful in most cases; having goto use relative indexing (so that the current history entry is index 0) makes goto a bit more useful on its own.

Maybe instead of a separate array/index pair we just have a List subclass that includes the current index? That way the app can just store a reference to that whole thing. For relative navigation I was thinking we would just add an overload for goForward and goBack that take an offset. So goBack(2) will go back two spots in the list. goBack() would chain to goBack(1), etc.

Also, I know it's come up before, but I think this is a good time to reconsider having canGoBack(), canGoForward(), and maybe something like gotoRange() (which would give the range of valid input for goto) so the app doesn't have the overhead of maintaining these and the possible bugs associated with getting them out of sync. I suppose you could add something like currentHistoryIndex() in here as well to address the indexing issue above.

I think that's part of a larger philosophical discussion of "should we save state in GeckoSession". I still don't think we need that, but we should instead fire initial events for things like onCanGoBack.

Assignee: nobody → snorp

Some further thoughts on this:

I think the "history stack" object is very similar to a saved state in practice, and possibly we should be using the same underlying representation for both (albeit perhaps with different exposed interfaces?).

Likewise, from an implementation standpoint, onHistoryListUpdate is essentially just a subset of the state saved callback (let's tentatively say onStateSaved); onHistoryListUpdate should be called whenever onStateSaved is called due to navigation (but not for scrolling or form data changes).

Those things considered, I think this bug should be viewed as pretty closely related to bug 1463878

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #7)

Also, assuming that onHistoryListUpdate and goto use the same indexing scheme, this requires the app to store the current index in order for goto to be useful in most cases; having goto use relative indexing (so that the current history entry is index 0) makes goto a bit more useful on its own.

Maybe instead of a separate array/index pair we just have a List subclass that includes the current index? That way the app can just store a reference to that whole thing. For relative navigation I was thinking we would just add an overload for goForward and goBack that take an offset. So goBack(2) will go back two spots in the list. goBack() would chain to goBack(1), etc.

I would suggest we expand onCanGoBack and onCanGoForward to supply ints indicating how far back/forward you can navigate, in that case. Also, if we have relative navigation with goBack and goForward, do we actually need goto at all? The case for absolute navigation seems pretty thin imo.

Also, I know it's come up before, but I think this is a good time to reconsider having canGoBack(), canGoForward(), and maybe something like gotoRange() (which would give the range of valid input for goto) so the app doesn't have the overhead of maintaining these and the possible bugs associated with getting them out of sync. I suppose you could add something like currentHistoryIndex() in here as well to address the indexing issue above.

I think that's part of a larger philosophical discussion of "should we save state in GeckoSession". I still don't think we need that, but we should instead fire initial events for things like onCanGoBack.

In what circumstances would we fire initial events? I thought we already did when loading a saved state, are there other cases?

I think the "history stack" object is very similar to a saved state in practice, and possibly we should be using the same underlying representation for both (albeit perhaps with different exposed interfaces?).

I think I'd agree with that line of thinking.

Also, if we have relative navigation with goBack and goForward, do we actually need goto at all? The case for absolute navigation seems pretty thin imo.

I'd think that for things like the session history popup it would feel much more natural to ideally just call goto with the index of the item that was selected instead of having to calculate a relative offset to the current history entry and then deciding whether that needs to be passed to goBack or to goForward.

P1 and [geckoview:fenix:m2] because this history feature is a must-have for Fenix M2:

https://github.com/mozilla-mobile/fenix/issues/351

Priority: P2 → P1
Whiteboard: [geckoview:fenix:p2] [gvtv] → [geckoview:fenix:p1] [gvtv] [geckoview:fenix:m2]

Dylan said he would look at this bug as part of his work on the new saveState API.

Assignee: snorp → droeh
Depends on: 1463878

[geckoview:fenix:m2] not [geckoview:fenix:p1]

Whiteboard: [geckoview:fenix:p1] [gvtv] [geckoview:fenix:m2] → [gvtv] [geckoview:fenix:m2]
Whiteboard: [gvtv] [geckoview:fenix:m2] → [gvtv:p1] [geckoview:fenix:m2]
Duplicate of this bug: 1537979
Attachment #9052731 - Attachment description: Bug 1509110 - Add HistoryState interface (implemented by SessionState), HistoryDelegate.onHistoryChange callback, and GeckoSession.gotoHistoryIndex. r=snorp! → Bug 1509110 - Add HistoryItem and HistoryList classes, HistoryDelegate.onHistoryStateChange callback, and GeckoSession.gotoHistoryIndex. r=snorp!
Flags: needinfo?(s.kaspari)

67=wontfix. Fenix MVP will use GeckoView 68, so we don't need to uplift this fix to 67 Beta.

Attachment #9052731 - Attachment description: Bug 1509110 - Add HistoryItem and HistoryList classes, HistoryDelegate.onHistoryStateChange callback, and GeckoSession.gotoHistoryIndex. r=snorp! → Bug 1509110 - Add HistoryItem and HistoryList classes, HistoryDelegate.onHistoryStateChange callback, and GeckoSession.gotoHistoryIndex. r=esawin!,#geckoview-reviewers!
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f3338782b169
Add HistoryItem and HistoryList classes, HistoryDelegate.onHistoryStateChange callback, and GeckoSession.gotoHistoryIndex. r=esawin,geckoview-reviewers,snorp
https://hg.mozilla.org/integration/autoland/rev/81253689d39c
Add testing for onHistoryStateChange and gotoHistoryIndex. r=geckoview-reviewers,snorp
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9ed5bb5e0cfc
Add HistoryItem and HistoryList classes, HistoryDelegate.onHistoryStateChange callback, and GeckoSession.gotoHistoryIndex. r=esawin,geckoview-reviewers,snorp
https://hg.mozilla.org/integration/autoland/rev/1bc29703a90f
Add testing for onHistoryStateChange and gotoHistoryIndex. r=geckoview-reviewers,snorp
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9a8350404d6c
Add HistoryItem and HistoryList classes, HistoryDelegate.onHistoryStateChange callback, and GeckoSession.gotoHistoryIndex. r=esawin,geckoview-reviewers,snorp
https://hg.mozilla.org/integration/autoland/rev/bc0600ed24ce
Add testing for onHistoryStateChange and gotoHistoryIndex. r=geckoview-reviewers,snorp
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Flags: needinfo?(droeh)
You need to log in before you can comment on or make changes to this bug.