Skip to content

Open links with the external browser #374

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
Merged
stephanebisson merged 1 commit into from
Aug 10, 2021
Merged

Open links with the external browser #374

stephanebisson merged 1 commit into from
Aug 10, 2021

Conversation

@stephanebisson
Copy link
Collaborator

@stephanebisson stephanebisson commented Aug 9, 2021 •
edited
Loading

Phabricator Link: N/A

Problem Statement

The app needs to open external pages in some cases like the privacy policy and media pages. We do so with window.open(). It works well but the memory used by that window is reported as part of the application.

Solution

Open external links with a WebActivity so that the memory is reported as the browser application.

Note

Sim Link: https://wikimedia.github.io/wikipedia-kaios/open-external/sim.html

@medied
Copy link
Contributor

medied commented Aug 9, 2021 •
edited
Loading

Working well from my end both on desktop sim and actual device. The only differences in behavior I noticed are:

  • when you open a new window and then press the back key,
    • with openExternal it asks you to quit the browser and lands you back on the phone app menu (and if you then open Wikipedia app again, it lands you where you left off)
    • with open.window it lowers/closes the browser wrapper and lands you where you left off from Wikipedia app.
  • also, something that could be argued as positive:
    • with openExternal a user can now land on an external page and then click on another link and keep on browsing normally, and pressing the back key would take you back to the previous browser page as expected.
    • with open.window pressing the back key always takes you back to Wikipedia app without warning

These are definitely not a blocker for me, specially if this helps with memory consumption.

hueitan
hueitan approved these changes Aug 10, 2021
View reviewed changes
Copy link
Member

@hueitan hueitan left a comment

Choose a reason for hiding this comment

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

it makes sense.

@stephanebisson
Copy link
Collaborator Author

stephanebisson commented Aug 10, 2021

@medied you're right, the experience is different because with the WebActivity we shell out to a different app and the interaction between apps is not perfect, especially with regards to the back button.

For the sake of a significant improvement in perceived memory usage, I'll go ahead and merge this.

@stephanebisson stephanebisson merged commit 5f4b2b5 into main Aug 10, 2021
@stephanebisson stephanebisson deleted the open-external branch August 10, 2021 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hueitan hueitan hueitan approved these changes

@medied medied Awaiting requested review from medied

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@stephanebisson @medied @hueitan