Feat/enable threefinger swipe, closes #3299 #7786

lucidNTR · 2017-03-19T23:28:27Z

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.

Test Plan:

On macOS:
- set system settings > trackpad > more gestures > swipe between pages to "OFF"
- start brave > open any website > click a few links
- try swiping with two or three fingers > nothing should happen

- quit brave > change same setting to "ON" and "swipe with two fingers"
- start brave > open any website > click a few links
- try swiping with two or three fingers > only two finger 'overscrolling' of horizontal scroll should trigger navigation

- quit brave > change same setting to "ON" and "swipe with three fingers"
- start brave > open any website > click a few links
- try swiping with two or three fingers > swiping with three fingers  horizontally should trigger navigation

- quit brave > change same setting to "ON" and "swipe with three or two fingers"
- start brave > open any website > click a few links
- try swiping with two or three fingers > swiping with three fingers  horizontally should trigger navigation or overscrolling with two fingers should trigger the navigation

lucidNTR · 2017-03-20T13:28:43Z

unsure why standard fails on travis but not locally, will fix soon, but of course PR feedback is welcome asap.

lucidNTR · 2017-03-21T13:54:59Z

@luixxiul the standard errors on travis are from code i did not even touch, is travis configured correctly?

/home/travis/build/brave/browser-laptop/app/renderer/components/clipboardButton.js:40:1: Expected indentation of 8 space characters but found 0.
/home/travis/build/brave/browser-laptop/app/renderer/components/preferences/payment/bitcoinDashboard.js:210:1: Expected indentation of 12 space characters but found 0.
/home/travis/build/brave/browser-laptop/app/renderer/components/preferences/payment/disabledContent.js:32:1: Expected indentation of 10 space characters but found 0.
/home/travis/build/brave/browser-laptop/app/renderer/components/preferences/payment/disabledContent.js:33:1: Expected indentation of 10 space characters but found 0.
/home/travis/build/brave/browser-laptop/app/renderer/components/preferences/payment/ledgerTable.js:110:11: Expected indentation of 4 space characters but found 10.

NejcZdovc · 2017-03-21T14:10:55Z

@lucidNTR #7794, there is already a fix ready for this problem

luixxiul · 2017-03-21T14:25:06Z

@lucidNTR The travis error issue is known to the team and they are working on it lately. You do not have to be too worried on the issue. Please run test locally and if it is fine, then it is ok.

cndouglas · 2017-03-26T18:47:21Z

ecd391c fixed two finger swipe and swipe disabled. #7905 is now tracking three finger swipe. Maybe we can integrate this to implement three finger swipe.

cndouglas · 2017-03-26T18:49:03Z

Tagged as work in progress since ecd391c has caused some conflicts.

lucidNTR · 2017-03-29T13:52:23Z

@liunkae i rebased my pr and also fixed a new issue where the system setting 'two AND three fingers' was not respected

cndouglas · 2017-03-30T15:16:28Z

@lucidNTR Great! I'll review in a little while.

cndouglas

I found one problem: the two finger swipe also works when only the three finger swipe is enabled.
Repro steps:

Set the preference to "Swipe with three fingers" (see screenshot)
Try to swipe left or right with two fingers.
The page navigates (but it should not).

Otherwise, looks good!

lucidNTR · 2017-03-30T16:23:31Z

@liunkae are you sure you restarted brave between changing the settings? I looked at the code and i cannot find a way to explain the wrong behaviour you described. :-/ Maybe you just closed the window, not full quit?

cndouglas · 2017-03-30T16:37:10Z

In one of my tests, I did not restart. Restarting seemed to fix the problem. Besides requiring a restart, the behavior now matches Firefox.

In my opinion, we probably should either make a note somewhere that it requires a restart or find a way to not require a restart.

lucidNTR · 2017-03-30T17:27:44Z

i thought about this for a while, the problem is that preventing restarts requires checking either on every scroll, or polling the system settings, both seem overkill for a setting that 99% of users change twice in 4 years. a perfect place to show the info would be if braves setting had a checkbox to disable swipe navigation or enable. but currently i think it would be overengineering, maybe we can add a ticket to future? - yes, all errors are my iPhone's fault

…

Am 30.03.2017 um 18:37 schrieb Liunkae ***@***.***>: In one of my tests, I did not restart. Restarting seemed to fix the problem. The behavior now matches Firefox. In my opinion, we probably should either make a note somewhere that it requires a restart or find a way to not require a restart. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub, or mute the thread.

cndouglas · 2017-03-30T19:39:41Z

Good points. Regardless, this PR is a step in the right direction.

bsclifton · 2017-04-03T08:00:44Z

@darkdh is this one you could also review? 😄

darkdh · 2017-04-03T14:48:45Z

Np

darkdh

Good job @lucidNTR.

darkdh · 2017-04-03T16:29:30Z

js/components/main.js

+
+    // isSwipeTrackingFromScrollEventsEnabled is only true if "two finger scroll to swipe" is enabled
+    // the swipe gesture handler will only fire if the three finger swipe setting is on, so the complete off setting and three and two finger together is also taken care of
+    if (systemPreferences.isSwipeTrackingFromScrollEventsEnabled()) {


Could you put this check in scroll-touch-begin? Because we don't want user to restart for every system setting change. Other browsers do not require restart and we should have the same behavior.

darkdh · 2017-04-03T16:31:46Z

Please squash all the commits after you make the change, thanks.

lucidNTR · 2017-04-05T01:46:43Z

@darkdh Ok i tried my best to keep the impact as minimal as possible, yet check on touchstart. Let me know if there is still issues, but my feeling is this is mergeable now...
Only issue is that there are non standard.js changes in master now and i could not commit without disabling checks, though i made sure my changes pass, any idea how this could have happened? probably someone has an outdated standard, we should hard pin the standard version used in the pre commit hook

lucidNTR · 2017-04-05T01:49:45Z

argh i committed my yarn file. is this ok? i strongly think yarn is far supperiour and we should have this anyways, ok or should i remove?

luixxiul · 2017-04-05T02:29:08Z

The PR which added yarn support was reverted once here: #6437 (comment)

I don't think any progress is done so I think you should remove the file from this PR.

darkdh · 2017-04-05T03:37:17Z

What @luixxiul said. Please remove it thanks.

lucidNTR · 2017-04-05T08:44:34Z

@darkdh ok removed, will look into that again at later point

darkdh

�lgtm now thanks

niclasek · 2017-09-08T18:25:24Z

Hi, I experience this bug with Brave version 0.18.29 on macOS 10.12.6
Expected result: as above
Actual result:

When setting 'swipe between pages' to 'swipe with three fingers' navigation does not trigger on swipe with three fingers (neither with two)
When setting 'swipe between pages' to 'swipe with two or three fingers' navigation triggers on swipe with three fingers but not on swipe with two fingers.

I have restarted brave after changing the setting.

cndouglas · 2017-09-08T19:03:28Z

@niclasek We have an issue for that: #9845

luixxiul added OS/macOS PR/needs-QA-attention ☕ labels Mar 20, 2017

cndouglas added PR/work-in-progress ⚒ and removed PR/needs-QA-attention ☕ labels Mar 26, 2017

lucidNTR force-pushed the feat/enable_threefinger_swipe branch from 3c06699 to 1d5138a Compare March 29, 2017 13:50

cndouglas self-requested a review March 30, 2017 15:16

cndouglas assigned lucidNTR Mar 30, 2017

cndouglas added PR/needs-QA-attention ☕ and removed PR/work-in-progress ⚒ labels Mar 30, 2017

cndouglas added this to the 0.14.1 milestone Mar 30, 2017

cndouglas suggested changes Mar 30, 2017

View reviewed changes

cndouglas approved these changes Mar 30, 2017

View reviewed changes

darkdh self-requested a review April 3, 2017 14:48

darkdh suggested changes Apr 3, 2017

View reviewed changes

lucidNTR force-pushed the feat/enable_threefinger_swipe branch from 1d5138a to 6546acc Compare April 5, 2017 01:41

feat: support three finger swipe navigation

f217ff9

lucidNTR force-pushed the feat/enable_threefinger_swipe branch from 6546acc to f217ff9 Compare April 5, 2017 08:43

darkdh approved these changes Apr 5, 2017

View reviewed changes

darkdh merged commit 8a97b0c into brave:master Apr 5, 2017

darkdh mentioned this pull request Apr 5, 2017

Added support for Back/forward gesture with three finger swipe #7905

Closed

luixxiul removed the PR/needs-QA-attention ☕ label Apr 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/enable threefinger swipe, closes #3299 #7786

Feat/enable threefinger swipe, closes #3299 #7786

lucidNTR commented Mar 19, 2017 •

edited by luixxiul

Loading

lucidNTR commented Mar 20, 2017

lucidNTR commented Mar 21, 2017

NejcZdovc commented Mar 21, 2017

luixxiul commented Mar 21, 2017

cndouglas commented Mar 26, 2017

cndouglas commented Mar 26, 2017

lucidNTR commented Mar 29, 2017

cndouglas commented Mar 30, 2017

cndouglas left a comment •

edited

Loading

lucidNTR commented Mar 30, 2017 •

edited

Loading

cndouglas commented Mar 30, 2017 •

edited

Loading

lucidNTR commented Mar 30, 2017 via email

cndouglas commented Mar 30, 2017 •

edited

Loading

bsclifton commented Apr 3, 2017

darkdh commented Apr 3, 2017

darkdh left a comment

darkdh Apr 3, 2017 •

edited

Loading

darkdh commented Apr 3, 2017

lucidNTR commented Apr 5, 2017

lucidNTR commented Apr 5, 2017

luixxiul commented Apr 5, 2017

darkdh commented Apr 5, 2017

lucidNTR commented Apr 5, 2017

darkdh left a comment

niclasek commented Sep 8, 2017

cndouglas commented Sep 8, 2017

Feat/enable threefinger swipe, closes #3299 #7786

Feat/enable threefinger swipe, closes #3299 #7786

Conversation

lucidNTR commented Mar 19, 2017 • edited by luixxiul Loading

lucidNTR commented Mar 20, 2017

lucidNTR commented Mar 21, 2017

NejcZdovc commented Mar 21, 2017

luixxiul commented Mar 21, 2017

cndouglas commented Mar 26, 2017

cndouglas commented Mar 26, 2017

lucidNTR commented Mar 29, 2017

cndouglas commented Mar 30, 2017

cndouglas left a comment • edited Loading

Choose a reason for hiding this comment

lucidNTR commented Mar 30, 2017 • edited Loading

cndouglas commented Mar 30, 2017 • edited Loading

lucidNTR commented Mar 30, 2017 via email

cndouglas commented Mar 30, 2017 • edited Loading

bsclifton commented Apr 3, 2017

darkdh commented Apr 3, 2017

darkdh left a comment

Choose a reason for hiding this comment

darkdh Apr 3, 2017 • edited Loading

Choose a reason for hiding this comment

darkdh commented Apr 3, 2017

lucidNTR commented Apr 5, 2017

lucidNTR commented Apr 5, 2017

luixxiul commented Apr 5, 2017

darkdh commented Apr 5, 2017

lucidNTR commented Apr 5, 2017

darkdh left a comment

Choose a reason for hiding this comment

niclasek commented Sep 8, 2017

cndouglas commented Sep 8, 2017

lucidNTR commented Mar 19, 2017 •

edited by luixxiul

Loading

cndouglas left a comment •

edited

Loading

lucidNTR commented Mar 30, 2017 •

edited

Loading

cndouglas commented Mar 30, 2017 •

edited

Loading

cndouglas commented Mar 30, 2017 •

edited

Loading

darkdh Apr 3, 2017 •

edited

Loading