Only update tabPageIndex if user is not hovering other tab #9809
Conversation
| // Otherwise each time the active tab is updated the tabPage | ||
| // will be updated as well. While we want that behavior not necessarily | ||
| // the user will be on the same tabPage as the active tab. See bug #9779 | ||
| const isPreviewing = state.get('frames').some(frame => frame.get('hoverState')) |
There was a problem hiding this comment.
I think this check should be in updateTabPageIndex to make the functionality consistent wherever it is called, but there are also a few issues here I think. The first one is that we still want to updated the tab page index, but without clearing the previewTabPageIndex, right? If the check is inside updateTabPageIndex that is easily accomplished. The second is that I think we want to check hover state for the tab pages and tabs? The last is that we want to avoid iterating over frames and I can't see any reason to have a per-frame hoverState prop. Can we change it to ['ui', 'tabs, 'hoverTabIndex'] to match hoverTabPageIndex?
There was a problem hiding this comment.
actually is it even necessary to clearn previewTabPageIndex inside updateTabPageIndex? I think it's covered by hoverTabPageIndex now. Maybe we can just remove it? The same probably applies to previewFrameKey above
There was a problem hiding this comment.
per-frame hoverState is needed to check whether or not the close tab icon should be shown -- a workaround I did for Aphrodite that can't handle children classes of pseudo-state. Needs to be per-frame.
Are you suggesting to add a new state that checks whether or not a tab is being hovered, like a global clone of hoverState to avoid iterating over frames? I think so but I'm often confused by state names regarding tabs.
There was a problem hiding this comment.
When I say we don't need a per-frame value, I'm suggesting that we can have a single key to keep track of which frame is hovered.
frames.1.hoverstate = false
frames.2.hoverstate = true
frames.3.hoverstate = false
changes to ui.tabs.hoverTabIndex = 2
cezaraugusto
force-pushed
the
tabsbar--9779
branch
from
b3170ad
to
51b876d
Compare
|
ok feedback addressed, ready for re-review |
| state = state.deleteIn(['ui', 'tabs', 'previewTabPageIndex']) | ||
|
|
||
| return state | ||
| // Do not update tabPageIndex if user is in hover mode |
There was a problem hiding this comment.
Do you know why it even attempts to do this if in hover state mode?
There was a problem hiding this comment.
The concern here is that updateTabPageIndex should just do what it says and not ignore for isTabInHoverState.
Instead we should just call updateTabPageIndex when the active state of a tab changes.
Because otherwise you could be on tab page 2 with an active tab on tab page 3. And as you're looking at tab page 2, audio could start playing on tab page 3 and it would change the active tab.
What you did mostly fixes it, but there are some edge cases that are missed.
I posted this issue for tracking the work, I'll merge this in the meantime:
#9911
You can do that task out of line with lower priority.
Only update tabPageIndex if user is not hovering other tab
Only update tabPageIndex if user is not hovering other tab
Only update tabPageIndex if user is not hovering other tab
Auditors: @bridiver, @NejcZdovc, @bsclifton, @bbondy
Fix #9809
Fix #9895
Reviewer note:
This PR includes work of PR #9896 (first commit). If this PR goes merged without it, please do not squash and I'll close the other. Thanks!
STR: