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

OpenNewTab: Support Vivaldi #89

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

OpenNewTab: Support Vivaldi #89

wants to merge 11 commits into from

Conversation

sctice
Copy link

@sctice sctice commented Nov 14, 2023

Vivaldi is a Chromium-based browser that has decided to present itself as Chrome but that differs from Chrome in a number of ways. In particular, Vivaldi uses shift+click to open a link in a new foreground tab and ctrl+shift+click to open a link in a new foreground window.

This adds support to detect Vivaldi and change the simulated click that the "OpenNewTab" command executes accordingly. Per this SO answer, we must use the extension tabs API to identify Vivaldi, and this requires some extra work that we'd prefer to do just once, so we do it as part of the options setup on the background Program. Additionally, the check requires accessing a property that the WebExtension definition of browser.tabs.Tab doesn't know about, so we have to extend that interface in @types.globals.d.ts for this special case.

Closes #39

@sctice
OpenNewTab: Support Vivaldi
896cb25 
Vivaldi is a Chromium-based browser that has decided to present itself
as Chrome but that differs from Chrome in a number of ways. In
particular, Vivaldi uses shift+click to open a link in a new foreground
tab and ctrl+shift+click to open a link in a new foreground *window*.

This adds support to detect Vivaldi and change the simulated click that
the "OpenNewTab" command executes accordingly. Per this SO answer[1], we
must use the extension tabs API to identify Vivaldi, and this requires
some extra work that we'd prefer to do just once, so we do it as part of
the options setup on the background Program. Additionally, the check
requires accessing a property that the WebExtension definition of
`browser.tabs.Tab` doesn't know about, so we have to extend that
interface in @types.globals.d.ts for this special case.

[1]: https://stackoverflow.com/a/77047611

Closes lydell#39
@sctice
Copy link
Author

sctice commented Nov 14, 2023

I've successfully run npm test and tested on Vivaldi. I haven't had time yet to test on Chrome/FF. I think it's unlikely this change will affect FF, but there's a little more risk with Chrome because it would be pretty easy for the Vivaldi check to mistakenly identify Chrome or other Chromium variants as Vivaldi.

lydell
lydell reviewed Nov 14, 2023
View reviewed changes
Copy link
Owner

@lydell lydell left a comment

Choose a reason for hiding this comment

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

Nice, I like it! I left some comments.

src/background/Program.ts Outdated Show resolved Hide resolved
src/background/Program.ts Outdated Show resolved Hide resolved
@sctice
Vivaldi: Make getChromiumVariant a function
7e6e856 
There's no need for it to be a method.

Also, move the initialization of `chromiumVariant` a bit to avoid
breaking up a sequence of related assignments.
@sctice
Vivaldi: Restrict type of chromiumVariant
dc98b4b 
We know the valid values are "", "chrome", and "vivaldi".
@sctice
Vivaldi: Check browser just before message dispatch
64bda04 
We'd like to avoid adding new state until we need it. In the meantime,
we're willing to live with a little more complexity where we dispatch
the OpenNewTab message in order to handle the asynchronous
`browser.tabs.query()` API.
@sctice
Copy link
Author

sctice commented Nov 15, 2023

I've pushed some changes to address comments. I went with the approach of detecting Vivaldi "online", right before dispatching the OpenNewTab message. It is a more compact change set. I haven't had time to try testing it out with the various ways that openNewTab() ends up getting called.

@sctice
Vivaldi: Handle background opening
d0ef413 
Vivaldi uses ctrl+click to open a tab in the background. So when we
detect Vivaldi, we _do_ want to ctrl-click if we're asked to open a tab
in the background rather than the foreground.
@sctice
Copy link
Author

sctice commented Nov 15, 2023

Fixed a bug I introduced while focusing on opening tabs in the foreground. We do want to ctrl-click in Vivaldi when opening a tab in the background (we just don't want shift in that case). In short, ctrl-click and shift-click are mutually exclusive in Vivaldi except in the case that you want to open a new window, which we never want to do in the context of handling the OpenNewTab message.

@sctice
Copy link
Author

sctice commented Nov 15, 2023

Thanks for working on this with me! I see now what you meant about using fireAndForget(). And I like dropping the empty string now that we're only checking in a chrome context.

@lydell
Copy link
Owner

lydell commented Nov 20, 2023

Status: Hoping to get some time to merge and make a release soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lydell lydell lydell left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Alt-L opens a new window instead of tab on Vivaldi / Vivaldi support in general
2 participants
@sctice @lydell