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

[HOLD for payment 2024-01-31] [HOLD for payment 2024-01-24] [$500] Feature Request: Three-finger swipe to go back and forward doesn't work on desktop #33435

Closed
1 of 6 tasks
m-natarajan opened this issue Dec 21, 2023 · 64 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item. Weekly KSv2

Comments

@m-natarajan
Copy link

m-natarajan commented Dec 21, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number:
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @rafecolton
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1703008037476039

Action Performed:

  1. Swipe on the trackpad with three fingers (or whatever you have configured in your system settings - could be two).

Expected Result:

Navigates back and forward like in the browser or when using cmd + [ or cmd + ]

Actual Result:

Does nothing

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01fce9f35e954f949c
  • Upwork Job ID: 1742359014682963968
  • Last Price Increase: 2024-01-09
  • Automatic offers:
    • shubham1206agra | Reviewer | 28092901
    • gijoe0295 | Contributor | 28092902
@m-natarajan m-natarajan added Weekly KSv2 NewFeature Something to build that is a new item. labels Dec 21, 2023
Copy link

melvin-bot bot commented Dec 21, 2023

@gijoe0295
Copy link
Contributor

gijoe0295 commented Dec 21, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

Feature Request: Three-finger swipe to go back and forward doesn't work on desktop

What is the root cause of that problem?

Feature request.

What changes do you think we should make in order to solve the problem?

Electron now supports swipe event here. We could implement it in desktop/main.js just like we already did here.

browserWindow.on('swipe', (e, direction) => {
    if (direction === 'right') {
        browserWindow.webContents.goBack();
    }
    if (direction === 'left') {
        browserWindow.webContents.goForward();
    }
});

However, as already highlighted in the documentation, in order for swipe to emit:

Screenshot 2023-12-22 at 01 39 01
  • The 'Swipe between pages' preference in System Preferences > Trackpad > More Gestures must be set to 'Swipe with two or three fingers'

  • User must swipe with three fingers

What alternative solutions did you explore? (Optional)

NA

@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels Dec 21, 2023
Copy link

melvin-bot bot commented Dec 25, 2023

@mallenexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

Copy link

melvin-bot bot commented Dec 27, 2023

@mallenexpensify Eep! 4 days overdue now. Issues have feelings too...

Copy link

melvin-bot bot commented Dec 29, 2023

@mallenexpensify 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

Copy link

melvin-bot bot commented Jan 2, 2024

@mallenexpensify 10 days overdue. Is anyone even seeing these? Hello?

@mallenexpensify mallenexpensify added the Internal Requires API changes or must be handled by Expensify staff label Jan 3, 2024
Copy link

melvin-bot bot commented Jan 3, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01fce9f35e954f949c

Copy link

melvin-bot bot commented Jan 3, 2024

Triggered auto assignment to Contributor Plus for review of internal employee PR - @shubham1206agra (Internal)

@mallenexpensify
Copy link
Contributor

A lil unsure what to do here. Our concept of navigation is weird. We don't have back/forward buttons on Desktop and command+[ and ] work for shortcut keys but we don't currently support other keyboard shortcuts.

@shubham1206agra , what do you think we should do?

@mallenexpensify mallenexpensify added Weekly KSv2 and removed Daily KSv2 labels Jan 3, 2024
@shubham1206agra
Copy link
Contributor

I think we should not do anything for swipe gesture as this is browser specific feature (only safari supports this).
And for navigation part, at best, we should show forward / back button for navigation (not sure), or add the navigation shortcut in keyboard shortcut screen so the user can be informed.

@gijoe0295
Copy link
Contributor

this is browser specific feature (only safari supports this)

This works on all browsers. I've tried and it worked fine on Desktop.

@shubham1206agra
Copy link
Contributor

Most macOS trackpads are not configured to allow this kind of swiping anymore

@gijoe0295
Copy link
Contributor

gijoe0295 commented Jan 3, 2024

That depends on user settings. You can read more:

the 'Swipe between pages' preference in System Preferences > Trackpad > More Gestures must be set to 'Swipe with two or three fingers'

By default, it is "Scroll":

Screenshot 2024-01-03 at 12 04 35

So if user can use swiping to navigate in browsers, they could definitely used it in other apps.

@shubham1206agra
Copy link
Contributor

Tbh I don't think anybody will use swiping (that too 3 finger).

@gijoe0295
Copy link
Contributor

I myself use it everyday (2-finger swipe) 😂 @rafecolton reported this so I think he uses it regularly as well.

@shubham1206agra
Copy link
Contributor

I myself use it everyday (2-finger swipe) 😂 @rafecolton reported this so I think he uses it regularly as well.

Yes, 2 finger-swipe, i.e., the default setting.

@rafecolton
Copy link
Member

I use it every day. It is supported by default in every browser I've used and on other desktop apps like Slack.

@mallenexpensify
Copy link
Contributor

Thanks all for the feedback! For Desktop, it the fix as simple as
Mac - For forward/back navigation, default to MacOS settings

and, for PC, is there anything we want to do now, so that if/when we build a PC Desktop app, we also default to any forward/back navigation in settings there?

@mallenexpensify
Copy link
Contributor

Was just testing swiping on new.expensify.com in Chrome and wasn't able to use my default 'swipe two fingers' to go forward/back, so the issue might not only be related to Desktop

@mallenexpensify
Copy link
Contributor

Desktop, Version 1.4.28-0 , isn't navigating all all, back or forward, when i use two fingers to swipe left or right
image

@gijoe0295
Copy link
Contributor

@mallenexpensify You should set the Swipe between pages to Swipe with three fingers. Electron only support that kind of swipe and it's also the OP.

@rafecolton
Copy link
Member

I don't think that's right @gijoe0295 - it should work with whatever gesture is set up in the system settings

@shubham1206agra
Copy link
Contributor

@rafecolton Slack doesn't allow the same either.

@rafecolton
Copy link
Member

Hm, interesting. I tried changing the settings to two or three fingers, and three fingers still works in slack, but two fingers does not. However, after changing it, two finger swipe does work in browsers.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Jan 24, 2024
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2024-01-24] [$500] Feature Request: Three-finger swipe to go back and forward doesn't work on desktop [HOLD for payment 2024-01-31] [HOLD for payment 2024-01-24] [$500] Feature Request: Three-finger swipe to go back and forward doesn't work on desktop Jan 24, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jan 24, 2024
Copy link

melvin-bot bot commented Jan 24, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Jan 24, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.30-1 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-01-31. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Jan 24, 2024

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@shubham1206agra] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
  • [@mallenexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

@mallenexpensify
Copy link
Contributor

TIL that 2 fingers doesn't work (ever if that's your mac setting) and 3 fingers does, both for Slack and now NewDot.
@rafecolton can you confirm that 3 fingers is working for you for NewDot? Thx

@rafecolton
Copy link
Member

Yep, seems to be working as expected now

@mallenexpensify
Copy link
Contributor

@shubham1206agra can you please accept the job and reply here once you have?
https://www.upwork.com/jobs/~01fce9f35e954f949c

@gijoe0295 , please confirm your name in Upwork so I can ensure I'm paying the right person?
Thx

@shubham1206agra
Copy link
Contributor

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

Steps on Desktop only

  1. Swipe on the trackpad with three fingers to navigate back and forward like in the browser or when using cmd + [ or cmd + ]
  • [@mallenexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

@shubham1206agra
Copy link
Contributor

Offer accepted.

@rafecolton
Copy link
Member

Noticed a possible regression, though I'm not 100% sure it's related - https://expensify.slack.com/archives/C049HHMV9SM/p1706724719541289

@shubham1206agra
Copy link
Contributor

Not sure how this is related to our change. But will look into this.
@rafecolton Can you just in case list all ways to navigate in desktop?

@rafecolton
Copy link
Member

Not sure how this is related to our change. But will look into this.

Not sure if it's related mechanically, only flagged it because it's related conceptually. So appreciate you checking it out.

Can you just in case list all ways to navigate in desktop?

I cannot, I don't know what they all are 😄 Off the top of my head, there should be three-finger swipe, cmd + left/right arrow, and cmd + square bracket.

@shubham1206agra
Copy link
Contributor

I confirmed this. And this is not related to desktop only. This is also a problem with web too. And github and slack does the navigation on cmd + left/right arrow on web too.

So this is not a regression from this issue.

cc @rafecolton

@gijoe0295
Copy link
Contributor

i do not have access to the channel. Can you describe the issue?

@rafecolton
Copy link
Member

@gijoe0295 not sure what you are referring to, there is no channel in question here. Are you sure this is the issue you meant to comment on?

@shubham1206agra thanks for checking. I don't know enough to say if it's a regression from this issue. Did you identify where the issue occurred?

You are not quite correct about this not working on web. It works IF you are not focused on an input field. If you're in an input field, it goes to the beginning and end of the line. But if you click out of the input field, it still works in the browser because the browser supports back and forward using cmd + arrows. On desktop, it does not work at all.

@gijoe0295
Copy link
Contributor

@rafecolton You linked the issue description here which is an internal Slack channel that contributors do not have access. So I did not know the issue and thus cannot investigate the root cause.

@rafecolton
Copy link
Member

Ah, that's just where I posted the concern in our bug-tracking channel, there's no new information there that does not appear in this GH

@amyevans
Copy link
Contributor

amyevans commented Feb 1, 2024

Just confirming that I tested locally as well, and removing the code added in the PRs here did not fix cmd + arrows navigation.

@rafecolton
Copy link
Member

Thanks @amyevans, I updated the Slack thread to let everybody know it's a separate issue

@gijoe0295
Copy link
Contributor

@mallenexpensify we're good for payment.

@mallenexpensify
Copy link
Contributor

Contributor: @gijoe0295 paid $500 via Upwork
Contributor+: @shubham1206agra paid $500 via Upwork.

@gijoe0295 if you'd like to join the Slack channel, details are here
https://github.com/Expensify/App/blob/main/contributingGuides/CONTRIBUTING.md#slack-channels

I created the TestRail update steps
https://github.com/Expensify/Expensify/issues/366364

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item. Weekly KSv2
Projects
None yet
Development

No branches or pull requests

6 participants