Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add navigationHistory.getEntryAtIndex(int index) method #41577

Merged
merged 18 commits into from
Mar 21, 2024
Merged

feat: add navigationHistory.getEntryAtIndex(int index) method #41577

merged 18 commits into from
Mar 21, 2024

Conversation

alicelovescake
Copy link
Member

@alicelovescake alicelovescake commented Mar 13, 2024 •
edited
Loading

Description of Change

Problem

Currently, Electron does not expose APIs that provide a list of navigation history. As a result, developers have no way to implement features such as:

  • Cmd/Ctrl click on the back/forward button to open the last or next entry in a new tab
  • Restoring tab history when tabs are restored

Proposed solution

In Chromium, a Navigation Controller manages session history. There are Chromium APIs that expose NavigationEntry, a data structure that captures all the information required to recreate a browsing state. In particular, NavigationEntry* GetEntryAtIndex(int index) allows us to fetch the navigation entry at a particular index.

NavigationEntry and NavigationController are both currently being used in the Electron codebase so there is no additional upkeep to expose new APIs that involve the same data structures.

Advantages of exposing a new API:

  • Developers don’t need to manually bookkeep navigation states, which has the risk of going out of sync with Chromium’s navigation history entries.
  • Enhance functionality of Electron for long term usability and DX.

This PR:

  1. Developers can get current navigation index with .getActiveIndex() and entire length of navigation history with .length() These APIs exist but are not included in the documentation. This PR adds them to public docs.
  2. With current indexes available, developers can fetch url and title of any navigation entry to save/ recreate history or cmd click back button to open in a new tab
  • Developers can't get an entire list of the navigation entries in a single API, but the available APIs should provide enough support to recreate the entire navigation history for every tab.
  • Updates documentation and adds tests

Checklist

Release Notes

Notes: Added a new instance property navigationHistory on webContents API with navigationHistory.getEntryAtIndex method, enabling applications to retrieve the URL and title of any navigation entry within the browsing history.

@welcome Welcome
Copy link

welcome bot commented Mar 13, 2024

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

  • fix: don't overwrite prevent_default if default wasn't prevented
  • feat: add app.isPackaged() method
  • docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

  • Follow the JavaScript, C++, and Python coding style.
  • Run npm run lint locally to catch formatting errors earlier.
  • Document any user-facing changes you've made following the documentation styleguide.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Mar 13, 2024
@alicelovescake alicelovescake marked this pull request as ready for review March 13, 2024 18:29
@alicelovescake alicelovescake changed the title feat: add getNavigationEntryFromIndex endpoint to web contents API feat: add getNavigationEntryFromIndex(int index) endpoint to web contents API Mar 13, 2024
@alicelovescake alicelovescake changed the title feat: add getNavigationEntryFromIndex(int index) endpoint to web contents API feat: add getNavigationEntryFromIndex(int index) to web contents API Mar 13, 2024
@alicelovescake
Copy link
Member Author

FYI, the failing tests (returns a Promise with a NativeImage in the rendere) in CI are not from my tests, and pass locally on my machine. I don't have the permission to retry the failed tests. So let me know if this is a blocker!

@codebytere
Copy link
Member

@alicelovescake if you rebase the tests have since been disabled!

@codebytere codebytere added the semver/minor backwards-compatible functionality label Mar 13, 2024
erickzhao
erickzhao reviewed Mar 13, 2024
View reviewed changes
Copy link
Member

@erickzhao erickzhao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @alicelovescake, can you add API documentation to the PR? We generally do API review via the Markdown API documentation (which also is used to generate the TypeScript types).

codebytere
codebytere reviewed Mar 13, 2024
View reviewed changes
spec/webview-spec.ts Outdated Show resolved Hide resolved
@alicelovescake
Copy link
Member Author

Hey @alicelovescake, can you add API documentation to the PR? We generally do API review via the Markdown API documentation (which also is used to generate the TypeScript types).

Will do! If it's ok with everyone, I'm going to include API documentation for some existing APIs that were not exposed before as well as my new one:

  • "getActiveIndex", &WebContents::GetActiveIndex
  • "length", &WebContents::GetHistoryLength
  • "getNavigationEntryForIndex", &WebContents::GetNavigationEntryForIndex

@alicelovescake alicelovescake changed the title feat: add getNavigationEntryFromIndex(int index) to web contents API feat: add getNavigationEntryFromIndex(int index) method Mar 13, 2024
@alicelovescake alicelovescake changed the title feat: add getNavigationEntryFromIndex(int index) method feat: add contents.getNavigationEntryFromIndex(int index) method Mar 13, 2024
@alicelovescake alicelovescake changed the title feat: add contents.getNavigationEntryFromIndex(int index) method feat: add contents.getNavigationEntryForIndex(int index) method Mar 14, 2024
@alicelovescake alicelovescake changed the title feat: add contents.getNavigationEntryForIndex(int index) method feat: add contents.getNavigationEntryAtIndex(int index) method Mar 14, 2024
@felixrieseberg felixrieseberg added target/29-x-y PR should also be added to the "29-x-y" branch. target/30-x-y PR should also be added to the "30-x-y" branch. labels Mar 14, 2024
@MarshallOfSound
Copy link
Member

MarshallOfSound commented Mar 14, 2024 •
edited
Loading

I don't love the overloading of the webContents namespace, I think especially if we're documenting a few internal APIs too this might make sense to live on it's own webContents.navigationHistory property or something.

So it would be webContents.navigationHistory.length() etc.

Would make naming make more sense too, like "length" of a webContents doesn't make sense independent of more context, where as the length of a "history" thing does make sense

+1 on the concept
-0.5 on the API location

@alicelovescake
Copy link
Member Author

I don't love the overloading of the webContents namespace, I think especially if we're documenting a few internal APIs too this might make sense to live on it's own webContents.navigationHistory property or something.

So it would be webContents.navigationHistory.length() etc.

Would make naming make more sense too, like "length" of a webContents doesn't make sense independent of more context, where as the length of a "history" thing does make sense

+1 on the concept -0.5 on the API location

Great point Sam! I agree that we can better organize these endpoints, especially the ones all related to navigationHistory.

What I can do is:

  1. In this PR, create new navigationHistory namespace to store getActiveIndex, length, and GetEntryAtIndex.
  2. In a separate PR, duplicate and move other navigationHistory related endpoints like CanGoBack, GoBack, CanGoForward, GoForward, CanGoToOffset, GoToOffset, CanGoToIndex, GoToIndex to the new namespace after the first PR merges.
  3. Deprecate the old endpoints later and highlight the change in release notes.

Let me know what you think and if you have other ideas! Appreciate the feedback!

@MarshallOfSound
Copy link
Member

Let me know what you think and if you have other ideas! Appreciate the feedback!

  1. Sounds great 💯
  2. Sounds great too 👍 We already did this from BrowserWindow -> WebContents many years ago, moving things another layer deep seems fine as long as we maintain the existing API surface as aliases to avoid the breaking change
  3. Other folks can comment on this, but maintaining the old aliases isn't that big of a maintenance burden. Happy leaving those in for a while. Deprecating and noting is fine, just like, there's no actual timeline to remove what is effectively webContents.foo = () => webContents.nav.foo() 😄

I'll take a look at the actual code / docs here next week (or at least try too 😅)

@alicelovescake alicelovescake changed the title feat: add contents.getNavigationEntryAtIndex(int index) method feat: add navigationHistory.getEntryAtIndex(int index) method Mar 16, 2024
@trop
Copy link
Contributor

trop bot commented Mar 21, 2024

I was unable to backport this PR to "29-x-y" cleanly;
you will need to perform this backport manually.

@trop trop bot added needs-manual-bp/30-x-y needs-manual-bp/29-x-y and removed target/29-x-y PR should also be added to the "29-x-y" branch. labels Mar 21, 2024
@alicelovescake alicelovescake deleted the getNavigationEntryFromIndex branch March 21, 2024 22:06
alicelovescake added a commit to alicelovescake/electron that referenced this pull request Mar 21, 2024
@alicelovescake
feat: add navigationHistory.getEntryAtIndex(int index) method (elec…
4ee75be 
…tron#41577)
@alicelovescake alicelovescake mentioned this pull request Mar 22, 2024
Merged
@trop
Copy link
Contributor

trop bot commented Mar 22, 2024

@alicelovescake has manually backported this PR to "29-x-y", please check out #41661

alicelovescake added a commit to alicelovescake/electron that referenced this pull request Mar 22, 2024
@alicelovescake
feat: add navigationHistory.getEntryAtIndex(int index) method (elec…
cf0f92b 
…tron#41577)
@alicelovescake alicelovescake mentioned this pull request Mar 22, 2024
Merged
@trop
Copy link
Contributor

trop bot commented Mar 22, 2024

@alicelovescake has manually backported this PR to "30-x-y", please check out #41662

@alicelovescake alicelovescake mentioned this pull request Mar 31, 2024
Merged
5 tasks
jkleinsc pushed a commit that referenced this pull request Apr 4, 2024
@alicelovescake
feat: add navigationHistory.getEntryAtIndex(int index) method (#41661)
97eee46 
* feat: add `navigationHistory.getEntryAtIndex(int index)` method (#41577)

* chore: remove code not related to this pr:

* test: fix flaky tests by replacing real urls with data urls

* test: remove hardcoded url
@trop trop bot added merged/29-x-y PR was merged to the "29-x-y" branch. and removed in-flight/29-x-y labels Apr 4, 2024
alicelovescake added a commit to alicelovescake/electron that referenced this pull request Apr 4, 2024
@alicelovescake
feat: add navigationHistory.getEntryAtIndex(int index) method (elec…
3ada70b 
…tron#41577)
jkleinsc pushed a commit that referenced this pull request Apr 5, 2024
@alicelovescake
feat: add navigationHistory.getEntryAtIndex(int index) method (#41662)
9056dd7 
* feat: add `navigationHistory.getEntryAtIndex(int index)` method (#41577)

* test: fix flaky tests by replacing real urls with data urls

* test: remove hardcoded url
@trop trop bot added merged/30-x-y PR was merged to the "30-x-y" branch. and removed in-flight/30-x-y labels Apr 5, 2024
@panther7
Copy link

These is missing in changelog (v29.3.0 and 30.x)

@alicelovescake
Copy link
Member Author

These is missing in changelog (v29.3.0 and 30.x)

Thanks for the callout! This is being updated!

@vitekcerny vitekcerny mentioned this pull request May 1, 2024
Merged
5 tasks
@codebytere codebytere mentioned this pull request May 13, 2024
Merged
4 tasks
@kommunarr kommunarr mentioned this pull request Jun 5, 2024
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixrieseberg felixrieseberg felixrieseberg approved these changes

@codebytere codebytere codebytere approved these changes

@MarshallOfSound MarshallOfSound MarshallOfSound approved these changes

@erickzhao erickzhao erickzhao approved these changes

Assignees
No one assigned
Labels
api-review/approved ✅ in-flight/29-x-y in-flight/30-x-y merged/29-x-y PR was merged to the "29-x-y" branch. merged/30-x-y PR was merged to the "30-x-y" branch. semver/minor backwards-compatible functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature Request]: View and modify navigation history of webcontents (and by extension webviews)
6 participants
@alicelovescake @codebytere @MarshallOfSound @panther7 @felixrieseberg @erickzhao