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

iPad: can not trigger context menu outside of Editor #120956

Closed
isidorn opened this issue Apr 9, 2021 · 12 comments
Closed

iPad: can not trigger context menu outside of Editor #120956

isidorn opened this issue Apr 9, 2021 · 12 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug insiders-released Patch has been released in VS Code Insiders ios-ipados menus Menu items and widget issues on-release-notes Issue/pull request mentioned in release notes touch/pointer Touch and Pointer Device support related issues verified Verification succeeded
Milestone

Comments

@isidorn
Copy link
Contributor

isidorn commented Apr 9, 2021

iPad

Currently you can trigger a context menu on the editor by holding a click.
However you can not trigger a context menu anywhere else in the workbench.

@rebornix is there something special that we do for this in the editor?
Might we need to do that in all the places?

fyi @joaomoreno for tree

@rebornix
Copy link
Member

rebornix commented Apr 9, 2021

@bpasero bpasero assigned rebornix and unassigned bpasero Apr 10, 2021
@bpasero bpasero added touch/pointer Touch and Pointer Device support related issues menus Menu items and widget issues labels Apr 10, 2021
@isidorn
Copy link
Contributor Author

isidorn commented Apr 12, 2021

@rebornix oh that's great!
@joaomoreno could you adopt this in the List? I can also look into this if you prefer
@bpasero do we have one central place in the workbench where we trigger a context menu on right click? So I can look into changing that to listen on touch/context as @rebornix suggests

@bpasero
Copy link
Member

bpasero commented Apr 12, 2021

@isidorn please sync with @sbatten who owns our custom menus

@isidorn
Copy link
Contributor Author

isidorn commented Apr 23, 2021

Ok, so a bunch of components listen to contextmenu event. They will now have to listen to the touch context menu event as well.
I can do a pass and adopt it across the workbench. Assigning to May to do it then.

fyi @joaomoreno for list / tree since I plan to change it there as well.

@isidorn isidorn added this to the May 2021 milestone Apr 23, 2021
@isidorn
Copy link
Contributor Author

isidorn commented Apr 23, 2021

Might be a bit of bigger task since the list does not seem to recognize from the touch event on what element it is being triggered.
Apart from that transforming a touch event to a BrowserMouseEvent might not work

@isidorn
Copy link
Contributor Author

isidorn commented Jun 2, 2021

fyi @joaomoreno just in case you are in an iPad bug destroying mode...

joaomoreno added a commit that referenced this issue Jun 2, 2021
@joaomoreno
Copy link
Member

Pushed a fix for all list/tree/tables. 🚀

@joaomoreno joaomoreno added the bug Issue identified by VS Code Team member as probable bug label Jun 2, 2021
@isidorn
Copy link
Contributor Author

isidorn commented Jun 2, 2021

Awesome 👏
We are actually now covered for most of the workbench.
Let's assign to June so I go through all the workbench and just make a list of places where it still does not work.

@isidorn isidorn modified the milestones: May 2021, June 2021 Jun 2, 2021
@isidorn
Copy link
Contributor Author

isidorn commented Jun 9, 2021

Here's are some places where the long press still does not trigger a context menu:

@sandy081 could you maybe look into some of the above and just do the same fix as @joaomoreno did for the list / tree. Or what I just did for the status bar. That is to listen to TouchEventType.Contextmenu, and add the HTMLElement to the Gesture targets.
If you prefer I can also do it. Let me know what works best

Most of the other context menu usages are inside the list and tree and I think we are already good there.

@isidorn
Copy link
Contributor Author

isidorn commented Jun 9, 2021

@sandy081 I actually started looking into the other areas. So you can hopefully just review my work once I push 😊

@isidorn isidorn closed this as completed in a12af58 Jun 9, 2021
@isidorn
Copy link
Contributor Author

isidorn commented Jun 9, 2021

I think I captured most of the cases in my list above. I just fixed them and this seems to work fine.
There are probably still some cases where we are not properly listening on touch context menu event but for now I am very happy with what we have and we will tackle the other issues as they come.

@isidorn
Copy link
Contributor Author

isidorn commented Jun 9, 2021

fyi @Tyriar @meganrogge if they would like to do the same thing for the terminal (even though we do not support the terminal for web cases it would be nice to have this for touch laptops). So the fix is just to add you element to Gesture targets and listen on Gesture context menu. Let me know if you would prefer that I create an issue for you for this.

@rebornix rebornix added the verified Verification succeeded label Jul 1, 2021
@isidorn isidorn added the on-release-notes Issue/pull request mentioned in release notes label Jul 2, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Jul 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug insiders-released Patch has been released in VS Code Insiders ios-ipados menus Menu items and widget issues on-release-notes Issue/pull request mentioned in release notes touch/pointer Touch and Pointer Device support related issues verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

7 participants
@joaomoreno @rebornix @bpasero @isidorn @sbatten @sandy081 and others