make tab previews based on mouse idle time #9887

cezaraugusto · 2017-07-06T07:23:07Z

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.

Auditors: @bridiver
/cc @luixxiul for QA

Test Plan:

Previews are now happening based on mouse idle time. Which means that it will only be fired if you stay the mouse idle over a tab for a given amount of time -- which is defined in prefs->tabs.

The general test plan is to hover over a tab up to a point that the tab previews.

Switching to another tab should preview the tab;
Moving out of a tab should cancel preview;
After each tab leave you will have to wait for the timeout to happen again, so you can preview

For further manual tests please refer to #8852 (comment)

Automated tests:

npm run test -- --grep="webview previews the next tab when current hovered tab is closed"
npm run test -- --grep="webview previews when tab is hovered"
npm run test -- --grep="TabsTab component"
npm run test -- --grep="tabUtil hasTabAsRelatedTarget"

Reviewer Checklist:

Tests

Adequate test coverage exists to prevent regressions
Tests should be independent and work correctly when run individually or as a suite ref
New files have MPL2 license header

cezaraugusto · 2017-07-06T07:29:03Z

app/renderer/components/tabs/tab.js


 class Tab extends React.Component {
  constructor (props) {
    super(props)
+    this.onMouseMove = this.onMouseMove.bind(this)


@bridiver before sending this I debounced mouse move but even with 10ms it was leading to bad behavior and tabs were previewing without user action, given they were debounced. The event triggered inside onmousemove is canceled with both mouseenter and mouseleave, which I guess cover the event's cost.

cezaraugusto · 2017-07-06T07:38:45Z

app/renderer/components/tabs/tab.js

+    // previewMode is only triggered if mouse is idle over a tab
+    // for a given amount of time based on timing defined in prefs->tabs
+    clearTimeout(this.mouseTimeout)
+    this.mouseTimeout = setTimeout(() => {


@bridiver I had a hard time moving the delayed state update to frameStateUtil while trying to keep using setTabHoverState (without adding extra actions).

afaict there's no way to delay state updates without calling an action inside the utils file? Even if so I guess per our convo we are trying to avoid that. If this is not unacceptable I would prefer to leave it as-is.

we definitely don't want to add a timeout here because we handle that in frameStateUtil

I think the best thing to do here is add a onTabMouseMoved action

cezaraugusto · 2017-07-06T07:40:56Z

js/actions/windowActions.js

-   *
-   * @param {Object} frameKey - the frame key for the webview in question.
-   */
-  setPreviewFrame: function (frameKey) {


we don't use it anymore. frameStateUtilsetPreviewFrameKey(..args) now checks for preview mode and returns if there is none. I guess the only use case for that was to delay tab previews for one use case, which we don't have anymore

bridiver · 2017-07-10T23:56:15Z

app/renderer/lib/tabUtil.js

@@ -36,3 +36,8 @@ module.exports.hasBreakpoint = (breakpoint, arr) => {
  arr = Array.isArray(arr) ? arr : [arr]
  return arr.includes(breakpoint)
 }
+
+module.exports.hasTabAsRelatedTarget = (event) => {
+  const tabComponents = /tab(?=_|area|title|id)|closetab|icon/i


this seems fragile to me, we should probably be checking for children with something like https://stackoverflow.com/questions/4697758/prevent-onmouseout-when-hovering-child-element-of-the-parent-absolute-div-withou

I ended up checking for parentNode instead + nulling pointer events for elements I don't want to be triggered. That method is now tested too.

bbondy · 2017-07-13T23:49:25Z

needs fixes and rebase so I'm going to move to 0.19.x

cezaraugusto · 2017-07-25T03:53:49Z

ok rebased + more tests added, ready for re review

NejcZdovc · 2017-07-25T08:25:36Z

docs/state.md

@@ -667,6 +667,8 @@ WindowStore
      hoverTabIndex: number, // index of the current hovered tab
      tabPageIndex: number, // index of the current tab page
      previewTabPageIndex: number // index of the tab being previewed
+      previewTabPageIndex: number, // index of the tab being previewed
+      previewMode: boolean // whether or not tab preview should be fired based on mouse idle time


can you please sort this alphabetically?

@bridiver

- Auditors: @bridiver, @bsclifton - Fix #8860 - Fix #3271

cezaraugusto · 2017-08-08T20:38:20Z

moving to 0.20.x per triage meeting

bsclifton

Works great! 😄

@bradleyrichter this PR is defaulting tab preview timing to long (which is very different than today's experience). Right now, we're closer to the Short value. Do you think these timings are OK? (long=2 seconds, normal=1 second, short=1/2 second)

cc: @alexwykoff @bbondy

bradleyrichter · 2017-08-14T17:16:16Z

@bsclifton @cezaraugusto We should use Normal as the default.

cezaraugusto · 2017-08-14T17:47:41Z

ok changed default to NORMAL

bsclifton

Nice polish on this one 😄 👌

make tab previews based on mouse idle time

Jacalz · 2017-08-17T13:48:37Z

@cezaraugusto could you add an even shorter timing? The shortest time feel too slow, I want to have it as fast as it was before this pr 👎 Great job otherwise 👍

Jacalz · 2017-08-17T13:49:58Z

It was instant before and I liked that and I think many users liked that, maby a setting called "no delay"?

cezaraugusto · 2017-08-17T16:12:09Z

@Jacalz I think that's something we should consider

cc @bradleyrichter

Jacalz · 2018-01-24T13:46:26Z

Any news about the no timing mode? I really hate having to wait for tab previews 👎
Should I open an issue for it? 🙂

bradleyrichter · 2018-01-24T14:32:23Z

Agree! Let’s include an instant setting to retain the original feel. We might want to consider using that setting for existing users who have previews enabled.

…

On Jan 24, 2018, at 5:46 AM, Jacob ***@***.***> wrote: Any news about the no timing mode? I really hate having to wait for tab previews 👎 Should I open an issue for it? 🙂 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

cezaraugusto · 2018-01-24T14:32:37Z

by now you can change to SHORT under prefs->tabs->time to wait before previewing a tab

cezaraugusto · 2018-01-24T14:33:19Z

@Jacalz do you mind opening an issue for this? pls cc me in there let's do it

Jacalz · 2018-01-24T15:41:37Z

Will do 👍

Jacalz · 2018-01-24T15:42:14Z

The short time is too long for me 😅

Jacalz · 2018-01-24T16:02:43Z

Opened the issue for you @cezaraugusto 👍

cezaraugusto added the feature/tabsbar label Jul 6, 2017

cezaraugusto added this to the 0.18.x (Beta Channel) milestone Jul 6, 2017

cezaraugusto self-assigned this Jul 6, 2017

cezaraugusto requested review from bridiver and luixxiul July 6, 2017 07:23

cezaraugusto commented Jul 6, 2017

View reviewed changes

bridiver reviewed Jul 10, 2017

View reviewed changes

bbondy modified the milestones: 0.19.x (Beta Channel), 0.18.x (Release Channel) Jul 13, 2017

luixxiul added the PR/needs-rebase ۞ label Jul 24, 2017

cezaraugusto requested review from bridiver and removed request for luixxiul and bridiver July 24, 2017 22:38

cezaraugusto added PR/work-in-progress ⚒ and removed PR/needs-rebase ۞ labels Jul 24, 2017

cezaraugusto force-pushed the tabsbar--preview-timing branch 2 times, most recently from 4e49938 to 95d61a2 Compare July 25, 2017 03:42

brave deleted a comment from codecov-io Jul 25, 2017

cezaraugusto removed the PR/work-in-progress ⚒ label Jul 25, 2017

cezaraugusto requested review from bridiver, bsclifton and luixxiul July 25, 2017 03:54

cezaraugusto added PR/needs-QA-attention ☕ release-notes/include labels Jul 25, 2017

cezaraugusto force-pushed the tabsbar--preview-timing branch from 95d61a2 to a7e9925 Compare July 25, 2017 03:56

NejcZdovc reviewed Jul 25, 2017

View reviewed changes

make tab previews based on mouse idle time

764d310

- Auditors: @bridiver, @bsclifton - Fix #8860 - Fix #3271

cezaraugusto force-pushed the tabsbar--preview-timing branch from 14f7c04 to 764d310 Compare August 8, 2017 20:13

cezaraugusto removed the PR/needs-rebase ۞ label Aug 8, 2017

cezaraugusto modified the milestones: 0.20.x (Developer Channel), 0.21.x (Nightly Channel) Aug 8, 2017

bsclifton reviewed Aug 14, 2017

View reviewed changes

set tab preview default to NORMAL

2e00e35

bsclifton approved these changes Aug 15, 2017

View reviewed changes

bsclifton merged commit 1deb581 into master Aug 15, 2017

bsclifton deleted the tabsbar--preview-timing branch August 15, 2017 06:21

bsclifton added a commit that referenced this pull request Aug 15, 2017

Merge pull request #9887 from brave/tabsbar--preview-timing

33c7dab

make tab previews based on mouse idle time

This was referenced Aug 19, 2017

closing a tab with right-click throws a console error #10590

Closed

avoid empty node when closing a tab with right click #10591

Merged

This was referenced Aug 28, 2017

onTabMouseMove is called too many times #10689

Closed

Reduce actions call when doing previews #10690

Merged

Jacalz mentioned this pull request Jan 24, 2018

Add option for no tab preview delay #12823

Closed

cezaraugusto mentioned this pull request Jan 24, 2018

Add an option for "no delay" in tab preview timing #12832

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make tab previews based on mouse idle time #9887

make tab previews based on mouse idle time #9887

cezaraugusto commented Jul 6, 2017 •

edited by bsclifton

Loading

cezaraugusto Jul 6, 2017

cezaraugusto Jul 6, 2017

bridiver Jul 10, 2017

bridiver Jul 10, 2017

cezaraugusto Jul 6, 2017

bridiver Jul 10, 2017

cezaraugusto Jul 25, 2017

bbondy commented Jul 13, 2017

cezaraugusto commented Jul 25, 2017

NejcZdovc Jul 25, 2017

cezaraugusto commented Aug 8, 2017

bsclifton left a comment

bradleyrichter commented Aug 14, 2017

cezaraugusto commented Aug 14, 2017

bsclifton left a comment

Jacalz commented Aug 17, 2017

Jacalz commented Aug 17, 2017 •

edited

Loading

cezaraugusto commented Aug 17, 2017

Jacalz commented Jan 24, 2018

bradleyrichter commented Jan 24, 2018 via email

cezaraugusto commented Jan 24, 2018 •

edited

Loading

cezaraugusto commented Jan 24, 2018

Jacalz commented Jan 24, 2018

Jacalz commented Jan 24, 2018

Jacalz commented Jan 24, 2018

make tab previews based on mouse idle time #9887

make tab previews based on mouse idle time #9887

Conversation

cezaraugusto commented Jul 6, 2017 • edited by bsclifton Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbondy commented Jul 13, 2017

cezaraugusto commented Jul 25, 2017

Choose a reason for hiding this comment

cezaraugusto commented Aug 8, 2017

bsclifton left a comment

Choose a reason for hiding this comment

bradleyrichter commented Aug 14, 2017

cezaraugusto commented Aug 14, 2017

bsclifton left a comment

Choose a reason for hiding this comment

Jacalz commented Aug 17, 2017

Jacalz commented Aug 17, 2017 • edited Loading

cezaraugusto commented Aug 17, 2017

Jacalz commented Jan 24, 2018

bradleyrichter commented Jan 24, 2018 via email

cezaraugusto commented Jan 24, 2018 • edited Loading

cezaraugusto commented Jan 24, 2018

Jacalz commented Jan 24, 2018

Jacalz commented Jan 24, 2018

Jacalz commented Jan 24, 2018

cezaraugusto commented Jul 6, 2017 •

edited by bsclifton

Loading

Jacalz commented Aug 17, 2017 •

edited

Loading

cezaraugusto commented Jan 24, 2018 •

edited

Loading