-
Notifications
You must be signed in to change notification settings - Fork 973
improve tabs performance and UI with this one simple trick #10691
Conversation
4b20c2e
to
b058a21
Compare
b058a21
to
21005b1
Compare
less/main.less
Outdated
@@ -3,7 +3,7 @@ | |||
* You can obtain one at http://mozilla.org/MPL/2.0/. */ | |||
|
|||
@bodyBG: #000; | |||
@windowContainerFG: #f00; | |||
@windowContainerFG: #000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
main.less was removed on #10861, not being sure what those specifications have been required for. See: https://github.com/brave/browser-laptop/pull/10861/files#diff-eb0db025dc73e282655968f53c9bfbdfR171. We could discuss it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes I saw that just after pushing. Should be removed here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed main.less was removed here too 👍
const frame = getFrameByKey(state, frameKey) | ||
|
||
if (frame == null) { | ||
if (process.env.NODE_ENV !== 'test') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that you use this pattern a lot. I am curious why we wouldn't like to see this error in test
as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it always falls to that condition for unit tests even when the test pass for some async reason
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok will check out unit tests and try to fix it, so that we can remove this check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all state helper functions should be converted from format (currentWindow, frameKey)
to (currentWindow, tabId)
, because we are in the process of converting frameKey into tabId
return false | ||
} | ||
|
||
const isNewTabPage = frame.get('location') === 'about:newtab' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that we use this check in different places, can you please create function in utils for it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function should receive location as param, so that we can reuse this function in other places. We can create followup where we would fix other places, that are not included in this PR. This way we would have 'about:newtab'
defined only in one place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good
// until we know what we're loading. We should probably do this for | ||
// all about: pages that we already know the title for so we don't have | ||
// to wait for the title to be parsed. | ||
if (frame.get('location') === 'about:blank') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as for 'about:newtab'
we should do the same with about:blank
app/common/state/tabUIState.js
Outdated
// Constants | ||
const settings = require('../../../js/constants/settings') | ||
|
||
// state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/state/State/g
app/common/state/tabUIState.js
Outdated
const tabCloseState = require('../../common/state/tabContentState/tabCloseState') | ||
const partitionState = require('../../common/state/tabContentState/partitionState') | ||
|
||
// Utis |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/Utis/Utils/g
app/common/state/tabUIState.js
Outdated
const {getTextColorForBackground} = require('../../../js/lib/color') | ||
const {isEntryIntersected} = require('../../../app/renderer/lib/observerUtil') | ||
|
||
// settings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/settings/Settings/g
app/renderer/components/tabs/tab.js
Outdated
? <TabTitle frameKey={this.props.frameKey} /> | ||
: null | ||
} | ||
<Favicon frameKey={this.props.frameKey} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this component and all bellow we should be passing tabId into them
docs/state.md
Outdated
@@ -697,7 +696,8 @@ WindowStore | |||
hoverTabIndex: number, // index of the current hovered tab | |||
previewMode: boolean, // whether or not tab preview should be fired based on mouse idle time | |||
previewTabPageIndex: number, // index of the tab being previewed | |||
tabPageIndex: number // index of the current tab page | |||
tabPageIndex: number, // index of the current tab page | |||
intersectionRatio: number // at which tab size position the tab sentinel is being intersected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please sort this alphabetically
js/state/frameStateUtil.js
Outdated
const tabPageIndex = state.getIn(['ui', 'tabs', 'tabPageIndex'], 0) | ||
const previewTabPageIndex = state.getIn(['ui', 'tabs', 'previewTabPageIndex']) | ||
|
||
return previewTabPageIndex != null ? previewTabPageIndex : tabPageIndex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we simplify this to return state.getIn(['ui', 'tabs', 'previewTabPageIndex']) || tabPageIndex
js/state/frameStateUtil.js
Outdated
|
||
const hasFrame = (state, frameKey) => { | ||
const frame = getFrameByKey(state, frameKey) | ||
return !frame.isEmpty() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getFrameByKey
can return null as well so we need to add check here as well frame && !frame.isEmpty()
js/state/frameStateUtil.js
Outdated
|
||
const getTabIdByFrameKey = (state, frameKey) => { | ||
const frame = getFrameByKey(state, frameKey) | ||
return frame.get('tabId', tabState.TAB_ID_NONE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to add null check here as well
} | ||
// we only have one entry | ||
const entry = entries[0] | ||
windowActions.setTabIntersectionState(this.props.frameKey, entry.intersectionRatio) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is called quite a lot of time when resizing, can we doubnce it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
throttling tabs as we do now leads to #7925. I tried minimizing perf cost by observing only the first tab in a tab set. I'll share perf wins once I finish this monster
21005b1
to
019522e
Compare
@cezaraugusto just in case let me remind you that detaching and reattaching tabs do not work for now. Also you cannot drag tabs. |
@luixxiul @cezaraugusto should be fixed with #10937, so I will review it and if it's ok I will merge it very soon |
}, | ||
|
||
enforceFontVisibility: { | ||
fontWeight: '600' | ||
tab__title_bold: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if this would be a proper name as we have tab__title_windows
below which has bolder font-weight. How about renaming it to tab__title_isDarwin
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed this was addressed on https://github.com/brave/browser-laptop/pull/10691/files#diff-6c5463a6f60c16525fb939516ad54125R53
On the commit: d97a56b#diff-6c5463a6f60c16525fb939516ad54125R53
!this.props.showIconAtReducedSize && | ||
css(styles.icon__loading, iconStyles.icon__loading_color) | ||
) | ||
: !this.props.favicon && css(styles.icon__default, iconStyles.icon__default_props) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have icon__default*
for both styles and iconStyles. I'm wondering if it would be confusing a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed icon__default_props
to icon__default_sizeAndColor
. Not sure if best but more declarative ya
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed this was addressed on https://github.com/brave/browser-laptop/pull/10691/files#diff-af59f73d2e4394ed5cd93fbab7e1cdc7R92
On the commit: c6947c5#diff-af59f73d2e4394ed5cd93fbab7e1cdc7R92
background: '#ddd', | ||
borderColor: '#bbb', | ||
color: '#000', | ||
hover: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would you mind adding blank lines for readability?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
className={css(this.props.showCloseIcon && styles.closeTab)} | ||
className={css( | ||
styles.closeTab__icon, | ||
this.props.centralizeTabIcons && styles.closeTab__icon__centered |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be closeTab__icon_centered
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return <TabIcon | ||
className={css(styles.icon, styles.icon_audio)} | ||
data-test-id={this.audioIcon} | ||
className={css(styles.audio__tabIcon)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would you mind renaming this to styles.audioTab__icon
following d69c3f7#diff-08a3077ca08d52dfaad29de7ae4d904aR75 or renaming the other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed this was addressed on https://github.com/brave/browser-laptop/pull/10691/files#diff-1a5be0fc07ffeb6eda18603b57e9fce9R65
On the commit: 01092fe#diff-1a5be0fc07ffeb6eda18603b57e9fce9R65
|
||
const newSessionProps = StyleSheet.create({ | ||
newSession__indicator: { | ||
filter: this.props.isActive && this.props.textIsWhite ? 'invert(100%)' : 'none' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind formatting like this? f672622#diff-bf09ff10e2191a87584e58393f99b728R44
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed this was addressed on https://github.com/brave/browser-laptop/pull/10691/files#diff-d8880b1566bd03fcd4e0b29425a5d473R50
On the commit: 049f438#diff-d8880b1566bd03fcd4e0b29425a5d473R50
@@ -73,21 +64,17 @@ class NewSessionIcon extends React.Component { | |||
module.exports = ReduxComponent.connect(NewSessionIcon) | |||
|
|||
const styles = StyleSheet.create({ | |||
icon: { | |||
newSession__icon: { | |||
zIndex: 99, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind converting this into const and adding it to global.js so that we could see the relationship among z-index values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed this was addressed on https://github.com/brave/browser-laptop/pull/10691/files#diff-d8880b1566bd03fcd4e0b29425a5d473R70
On the commit: 049f438#diff-d8880b1566bd03fcd4e0b29425a5d473R70
@@ -214,6 +214,7 @@ const globalStyles = { | |||
zindexWindowIsPreview: '1100', | |||
zindexDownloadsBar: '1000', | |||
zindexTabs: '1000', | |||
zindexTabsAudioTopBorder: '10001', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it not 1001
instead of 10001
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed this was addressed on https://github.com/brave/browser-laptop/pull/10691/files#diff-f14732b3ea914a26c2215676e7208907R217
On the commit: f4780d3#diff-f14732b3ea914a26c2215676e7208907R217
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++ 👍
6517c74
to
33c5d10
Compare
@cezaraugusto please do not forget to merge this to |
improve tabs performance and UI with this one simple trick
By doing QA I must say the tabs area has been massively improved 🦁 Kudos to @cezaraugusto 🎖 |
Auditors: @luixxiul Address brave#10691 Follow-up of sha 33c5d10
TL;DR
Click to ignore it all and go to test plan
Performance tests
Performance while resizing window
Test plan was made as the following:
Metrics used:
current
main resize function cost
We relied on
get tabSize
for our calculations, which has some cost and we don't use it anymore.main call stack cost
Values varied while doing tests. Below shows that previous call stack was lighter than what this PR introduces so still room for improvement. Getting a tab by id seems to have a heavier cost too and other new methods also added some weight.
after this PR
main resize function cost
There's no need for this.tabSize getter anymore.
main call stack cost
Performance while opening tabs
Test plan was made as the following:
current
Again
get tabSize
taking its space which is no longer required.after this pr
Performance while closing tabs
Test plan was made as the following:
I couldn't see a significant change here, as tab icons are only updated when you mouse out of tabs, but I'm sharing anyway.
Tests were made several times, below a comparison between two consecutive tests.
Other than scripting, no gains were found. Given we only update tab size after mouse leave, that makes sense.
current
after this pr
Performance while resizing window -- stress test (multiple heavy tabs)
Tests were made several times, below a comparison between two consecutive tests.
current
after this pr
Bugs fixed | usability improved
Total: 16 issues
close #10883
fix #10838
fix #10611
fix #10582
fix #10544
fix #10464
fix #10509
fix #10123
fix #10103
fix #9398
fix #8414
fix #7925
fix #7765
fix #7730
fix #7301
fix #6716
Unrelated tasks covered with this PR
Test plan
For reviewers (QA please skip)
For everybody (cc QA team)
Overall test plan is to have a lot of tabs, being included pinned, partitioned, private, newtab (which has no icon), about:blank (which has a "default" icon) and other tabs.
As you resize the window, icons should be shown in a graceful state, without bad UI/UX
Running through each issue (from older to newer)
Issue #6716 (fading effect b/w tabs feels like flickering)
This is slightly different than #10838 as was reported by not the active tab. Test plan is similar.
Issue #7301 (Tabs display wrong after exiting HTML5 fullscreen)
Issue #7730 (Title on new tab flickers after setting the language to Russian)
Issue #7925 (Throttling between window resize cause tabContent to pop)
Issue #7765 (Tab title often isn't updated until the cursor is moved over it or tab focus is changed)
npm run add-simulated-payment-history
)Issue #8414 (Tabs No Longer Show Full-Color Emoji)
Issue #9398 (Tab Label Rendering Issue)
Not sure how to track this but issue is unreproducible. See #9398 for comments
issue #10103 (Tab preview does not work properly with DnD)
Note: Needs PR #10937 merged to be tested.
Issue #10123 (no tab close icon is shown for the active tab on Windows)
(you need Windows)
(Test plan gently borrowed from @bsclifton)
Issue #10464 (Description for Session Tab on Hover)
Issue #10509 (make use of intersectionObserver for tabs)
This is the main base of this PR and there's no test plan needed
Issue #10544 (Tabs cannot be muted via speaker icon)
Issue #10582 (Tabs do not automatically resize until mousing over them)
Issue #10611 (intermittent - audio indicator is not shown)
This issue happened because when breakpoints was a thing, "dynamic"
breakpoint wasn't set for the icon to be visible (see https://github.com/brave/browser-laptop/blob/master/app/renderer/components/tabs/tab.js#L263). We do not rely on breakpoints anymore, so:
Issue #10838 (Active tab flicker on hover)
Issue #10883 (create smooth transitions for tab components)