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.

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

Will-navigate event should forward to opener service #90360

Closed
wants to merge 2 commits into from

Conversation

joaomoreno
Copy link
Member

@joaomoreno joaomoreno commented Feb 10, 2020

Related to #89215

This makes it that attempts to navigate the page in VS Code just get forwarded to OpenerService.open calls. Eg. clicking something like <a href="https://github.com">this</a> will eventually call openerService.open('https://github.com').

This is useful in notifications, where we currently listen to click events, and empty view contents #89215.

@bpasero @jrieken @mjbvz Let me know your thoughts.

@joaomoreno joaomoreno self-assigned this Feb 10, 2020
@joaomoreno joaomoreno requested review from bpasero, mjbvz and jrieken and removed request for bpasero and mjbvz February 10, 2020 12:00
@joaomoreno joaomoreno added this to the February 2020 milestone Feb 10, 2020
@jrieken
Copy link
Member

jrieken commented Feb 10, 2020

idk - we are using "normal" link with a click listener in many other places and that seems to work just fine. I see two issues with this:

  • a half baked adoption as other callers, e.g users of actionHandler?: IContentActionHandler; aren't updated by this change
  • a dependency onto the main-process, meaning notification links won't work in the web companion

I'd suggest to stick to what we have been doing before or have "global" listener inside the renderer so that this isn't desktop-only

@joaomoreno
Copy link
Member Author

joaomoreno commented Feb 10, 2020

The problem with the current approach is that we also need to handle keyboard Enter for keyboard accessibility. And we must handle clicks and Enters everywhere links might appear, over and over again. 😢

@bpasero
Copy link
Member

bpasero commented Feb 10, 2020

Yeah for web we need a different solution, good point. I think finding consumers should be relatively easier going for the IOpenerService usage.

@joaomoreno
Copy link
Member Author

I'm not going to hunt all consumers of opener service. If you think this isn't good enough, I can just use click and key down handlers and let this go, like everyone else.

@jrieken
Copy link
Member

jrieken commented Feb 10, 2020

The problem with the current approach is that we also need to handle keyboard Enter for keyboard accessibility. And we must handle clicks and Enters everywhere links might appear, over and over again. 😢

Doesn't that make a case for a Link-widget which does all of that, e.g using the opener service and having a command registered and so

@joaomoreno
Copy link
Member Author

OK, closing it up!

@joaomoreno joaomoreno closed this Feb 10, 2020
@joaomoreno joaomoreno deleted the joao/will-navigate-opener-service branch February 10, 2020 14:00
@joaomoreno
Copy link
Member Author

@bpasero In case you'd like to adopt in Notifications: ac834c0#diff-9f5d19be21bb00b33fa612b5e0d42066R19

@bpasero
Copy link
Member

bpasero commented Feb 10, 2020

@joaomoreno I can look into it. Can we move this helper to the opener service into a browser layer? I find its location in workbench odd, especially since we may want to use it from the editor too.

@joaomoreno
Copy link
Member Author

@bpasero Something like this? 1ceefd1

@bpasero
Copy link
Member

bpasero commented Feb 10, 2020

Yes, thanks!

@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants