UI bug fixing

- tabs can now respond beautifully to intersections - Also BEMify tabs Auditors: @bsclifton, @luixxiul fix brave#6716 fix brave#7301 fix brave#7730 fix brave#7765 fix brave#7925 fix brave#10123 fix brave#10509 fix brave#10544 fix brave#10582 fix brave#10611 fix brave#10838
syuan100 · Nov 9, 2017 · ac96632 · ac96632
1 parent 7b846b6
commit ac96632
Show file tree

Hide file tree

Showing 5 changed files with 110 additions and 89 deletions.
diff --git a/app/renderer/components/styles/commonStyles.js b/app/renderer/components/styles/commonStyles.js
@@ -161,7 +161,7 @@ const styles = StyleSheet.create({
   notificationBar__notificationItem: {
     backgroundColor: globalStyles.color.notificationItemColor,
     boxSizing: 'border-box',
-    borderTop: `1px solid ${globalStyles.color.tabsToolbarBorderColor}`,
+    boxShadow: `0 -1px 0 ${globalStyles.color.tabsToolbarBorderColor}`,
     lineHeight: '24px',
     padding: '8px 20px'
   },

diff --git a/app/renderer/components/styles/global.js b/app/renderer/components/styles/global.js
@@ -25,7 +25,7 @@ const globalStyles = {
     breakpointExtensionButtonPadding: '720px',
     breakpointSmallWin32: '650px',
     breakpointTinyWin32: '500px',
-    breakpointNewPrivateTab: '890px'
+    breakpointNewPrivateTab: '890px' // page's breakpoint for the private tab page
   },
   intersection: {
     // whereas 1 === 100%
@@ -162,7 +162,7 @@ const globalStyles = {
     aboutPageDetailsPageWidth: '704px',
     aboutPageSectionPadding: '24px',
     aboutPageSectionMargin: '10px',
-    defaultTabPadding: '0 4px',
+    defaultTabMargin: '6px',
     defaultIconPadding: '2px',
     iconSize: '16px',
     sessionIconSize: '15px',
@@ -214,6 +214,7 @@ const globalStyles = {
     zindexWindowIsPreview: '1100',
     zindexDownloadsBar: '1000',
     zindexTabs: '1000',
+    zindexTabsAudioTopBorder: '10001',
     zindexTabsThumbnail: '1100',
     zindexTabsDragIndicator: '1100',
     zindexNavigationBar: '2000',

diff --git a/app/renderer/components/tabs/tab.js b/app/renderer/components/tabs/tab.js
@@ -1,9 +1,9 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
- * License, v. 2.0. If a copy of the MPL was not distributed with this file,
- * You can obtain one at http://mozilla.org/MPL/2.0/. */
+* License, v. 2.0. If a copy of the MPL was not distributed with this file,
+* You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 const React = require('react')
-const {StyleSheet, css} = require('aphrodite')
+const {StyleSheet, css} = require('aphrodite/no-important')
 const Immutable = require('immutable')
 
 // Components
@@ -23,7 +23,8 @@ const windowActions = require('../../../../js/actions/windowActions')
 // Store
 const windowStore = require('../../../../js/stores/windowStore')
 
-// State
+// State helpers
+const privateState = require('../../../common/state/tabContentState/privateState')
 const audioState = require('../../../common/state/tabContentState/audioState')
 const tabUIState = require('../../../common/state/tabUIState')
 const tabState = require('../../../common/state/tabState')
@@ -33,6 +34,7 @@ const dragTypes = require('../../../../js/constants/dragTypes')
 
 // Styles
 const globalStyles = require('../styles/global')
+const {theme} = require('../styles/theme')
 
 // Utils
 const cx = require('../../../../js/lib/classSet')
@@ -119,10 +121,17 @@ class Tab extends React.Component {
   }
 
   onDragStart (e) {
+    // showing up the sentinel while dragging leads to show the shadow
+    // of the next tab. See 10691#issuecomment-329854096
+    // this is added back when dragEnd event happens
+    this.tabSentinel.style.display = 'none'
+
     dnd.onDragStart(dragTypes.TAB, this.frame, e)
   }
 
   onDragEnd (e) {
+    // re-enable the tabSentinel after drag ends
+    this.tabSentinel.style.display = 'block'
     dnd.onDragEnd(dragTypes.TAB, this.frame, e)
   }
 
@@ -186,9 +195,11 @@ class Tab extends React.Component {
   }
 
   componentDidMount () {
-    // do not observe pinned tabs
+    // unobserve tabs that we don't need. This will
+    // likely be made by onObserve method but added again as
+    // just to double-check
     if (this.props.isPinned) {
-      this.observer.unobserve(this.tabSentinel)
+      this.observer && this.observer.unobserve(this.tabSentinel)
     }
     const threshold = Object.values(globalStyles.intersection)
     // At this moment Chrome can't handle unitless zeroes for rootMargin
@@ -200,20 +211,11 @@ class Tab extends React.Component {
     this.tabNode.addEventListener('auxclick', this.onAuxClick.bind(this))
   }
 
-  componentDidUpdate () {
-    // if a tab was just pinned, unobserve its intersection
-    // this gives us some more performance boost
-    if (this.props.isPinnedTab) {
-      this.observer.unobserve(this.tabSentinel)
-    }
-  }
-
   componentWillUnmount () {
     this.observer.unobserve(this.tabSentinel)
   }
 
   onObserve (entries) {
-    // avoid observing pinned tabs
     if (this.props.isPinnedTab) {
       return
     }
@@ -234,27 +236,32 @@ class Tab extends React.Component {
   mergeProps (state, ownProps) {
     const currentWindow = state.get('currentWindow')
     const frame = frameStateUtil.getFrameByKey(currentWindow, ownProps.frameKey) || Immutable.Map()
+    const frameKey = ownProps.frameKey
+    const tabId = frame.get('tabId', tabState.TAB_ID_NONE)
+    const isPinned = tabState.isTabPinned(state, tabId)
+    const partOfFullPageSet = ownProps.partOfFullPageSet
+
+    // TODO: this should have its own method
     const notifications = state.get('notifications')
     const notificationOrigins = notifications ? notifications.map(bar => bar.get('frameOrigin')) : false
     const notificationBarActive = frame.get('location') && notificationOrigins &&
       notificationOrigins.includes(UrlUtil.getUrlOrigin(frame.get('location')))
-    const tabId = frame.get('tabId', tabState.TAB_ID_NONE)
 
     const props = {}
-    // used in renderer
-    props.frameKey = ownProps.frameKey
-    props.isPrivateTab = frame.get('isPrivate')
+    // TODO: this should have its own method
     props.notificationBarActive = notificationBarActive
-    props.isActive = frameStateUtil.isFrameKeyActive(currentWindow, props.frameKey)
+    props.frameKey = frameKey
+    props.isPinnedTab = isPinned
+    props.isPrivateTab = privateState.isPrivateTab(currentWindow, frameKey)
+    props.isActive = frameStateUtil.isFrameKeyActive(currentWindow, frameKey)
     props.tabWidth = currentWindow.getIn(['ui', 'tabs', 'fixTabWidth'])
-    props.isPinnedTab = tabState.isTabPinned(state, tabId)
-    props.canPlayAudio = audioState.canPlayAudio(currentWindow, props.frameKey)
-    props.themeColor = tabUIState.getThemeColor(currentWindow, props.frameKey)
+    props.themeColor = tabUIState.getThemeColor(currentWindow, frameKey)
     props.title = frame.get('title')
-    props.partOfFullPageSet = ownProps.partOfFullPageSet
+    props.partOfFullPageSet = partOfFullPageSet
+    props.showAudioTopBorder = audioState.showAudioTopBorder(currentWindow, frameKey, isPinned)
+    props.centralizeTabIcons = tabUIState.centralizeTabIcons(currentWindow, frameKey, isPinned)
 
     // used in other functions
-    props.totalTabs = state.get('tabs').size
     props.dragData = state.getIn(['dragData', 'type']) === dragTypes.TAB && state.get('dragData')
     props.tabId = tabId
     props.previewMode = currentWindow.getIn(['ui', 'tabs', 'previewMode'])
@@ -265,7 +272,7 @@ class Tab extends React.Component {
   render () {
     // we don't want themeColor if tab is private
     const perPageStyles = !this.props.isPrivateTab && StyleSheet.create({
-      themeColor: {
+      tab_themeColor: {
         color: this.props.themeColor ? getTextColorForBackground(this.props.themeColor) : 'inherit',
         background: this.props.themeColor ? this.props.themeColor : 'inherit',
         ':hover': {
@@ -301,13 +308,15 @@ class Tab extends React.Component {
         className={css(
           styles.tab,
           // Windows specific style
-          isWindows && styles.tabForWindows,
-          this.props.isPinnedTab && styles.isPinned,
-          this.props.isActive && styles.active,
-          this.props.isActive && this.props.themeColor && perPageStyles.themeColor,
+          isWindows && styles.tab_forWindows,
+          this.props.isPinnedTab && styles.tab_pinned,
+          this.props.isActive && styles.tab_active,
+          this.props.showAudioTopBorder && styles.tab_audioTopBorder,
+          this.props.isActive && this.props.themeColor && perPageStyles.tab_themeColor,
           // Private color should override themeColor
-          this.props.isPrivateTab && styles.private,
-          this.props.isActive && this.props.isPrivateTab && styles.activePrivateTab
+          this.props.isPrivateTab && styles.tab_private,
+          this.props.isActive && this.props.isPrivateTab && styles.tab_active_private,
+          this.props.centralizeTabIcons && styles.tab__content_centered
         )}
         data-test-id='tab'
         data-frame-key={this.props.frameKey}
@@ -323,7 +332,10 @@ class Tab extends React.Component {
           ref={(node) => { this.tabSentinel = node }}
           className={css(styles.tab__sentinel)}
         />
-        <div className={css(styles.tabId)}>
+        <div className={css(
+          styles.tab__identity,
+          this.props.centralizeTabIcons && styles.tab__content_centered
+        )}>
           <Favicon tabId={this.props.tabId} />
           <AudioTabIcon tabId={this.props.tabId} />
           <TabTitle tabId={this.props.tabId} />
@@ -337,34 +349,39 @@ class Tab extends React.Component {
 }
 
 const styles = StyleSheet.create({
-  // Windows specific style
-  tabForWindows: {
-    color: '#555'
-  },
-
   tab: {
     borderWidth: '0 1px 0 0',
     borderStyle: 'solid',
     borderColor: '#bbb',
     boxSizing: 'border-box',
-    color: '#5a5a5a',
+    color: theme.tab.color,
     display: 'flex',
     marginTop: '0',
-    transition: `transform 200ms ease, ${globalStyles.transition.tabBackgroundTransition}`,
+    transition: theme.tab.transition,
     left: '0',
     opacity: '1',
-    width: '100%',
+    height: '-webkit-fill-available',
+    width: '-webkit-fill-available',
     alignItems: 'center',
     justifyContent: 'space-between',
-    padding: globalStyles.spacing.defaultTabPadding,
+    padding: 0,
     position: 'relative',
 
-    // globalStyles.spacing.tabHeight has been set to globalStyles.spacing.tabsToolbarHeight,
-    // which is 1px extra due to the border-top of .tabsToolbar
-    height: '-webkit-fill-available',
+    ':hover': {
+      background: theme.tab.hover.background
+    }
+  },
+
+  // Windows specific style
+  tab_forWindows: {
+    color: theme.tab.forWindows.color
+  },
+
+  tab_active: {
+    background: theme.tab.active.background,
 
     ':hover': {
-      background: 'linear-gradient(to bottom, rgba(255, 255, 255, 0.8), rgba(250, 250, 250, 0.4))'
+      background: theme.tab.active.background
     }
   },
 
@@ -380,59 +397,56 @@ const styles = StyleSheet.create({
     width: globalStyles.spacing.sentinelSize
   },
 
-  tabId: {
-    justifyContent: 'center',
+  tab_audioTopBorder: {
+    '::before': {
+      content: `''`,
+      display: 'block',
+      position: 'absolute',
+      top: 0,
+      left: 0,
+      right: 0,
+      height: '2px',
+      background: 'lightskyblue'
+    }
+  },
+
+  tab__identity: {
+    justifyContent: 'flex-start',
     alignItems: 'center',
     display: 'flex',
     flex: '1',
-
-    // @see https://bugzilla.mozilla.org/show_bug.cgi?id=1108514#c5
-    minWidth: '0',
-
-    // prevent the icons wrapper from being the target of mouse events.
-    pointerEvents: 'none'
+    minWidth: '0', // @see https://bugzilla.mozilla.org/show_bug.cgi?id=1108514#c5
+    marginLeft: globalStyles.spacing.defaultTabMargin
   },
 
-  isPinned: {
+  tab__content_centered: {
+    justifyContent: 'center',
+    flex: 'auto',
     padding: 0,
-    width: `calc(${globalStyles.spacing.tabsToolbarHeight} * 1.1)`,
-    justifyContent: 'center'
+    margin: 0
   },
 
-  active: {
-    background: `rgba(255, 255, 255, 1.0)`,
-    marginTop: '0',
-    borderWidth: '0 1px 0 0',
-    borderStyle: 'solid',
-    borderColor: '#bbb',
-    color: '#000',
-
-    ':hover': {
-      background: `linear-gradient(to bottom, #fff, ${globalStyles.color.chromePrimary})`
-    }
+  tab_pinned: {
+    padding: 0,
+    width: '28px',
+    justifyContent: 'center'
   },
 
-  activePrivateTab: {
-    background: globalStyles.color.privateTabBackgroundActive,
+  tab_active_private: {
+    background: theme.tab.active.private.background,
+    color: theme.tab.active.private.color,
 
     ':hover': {
-      background: globalStyles.color.privateTabBackgroundActive
+      background: theme.tab.active.private.background
     }
   },
 
-  private: {
-    background: 'rgba(75, 60, 110, 0.2)',
+  tab_private: {
+    background: theme.tab.private.background,
 
     ':hover': {
-      background: globalStyles.color.privateTabBackgroundActive
-    }
-  },
-
-  dragging: {
-    ':hover': {
-      closeTab: {
-        opacity: '0'
-      }
+      color: theme.tab.active.private.color,
+      background: theme.tab.active.private.background
     }
   }
 })

diff --git a/less/tabs.less b/less/tabs.less
@@ -10,7 +10,7 @@
   flex: 1;
   overflow: auto;
   padding: 0;
-  height: @tabsToolbarHeight;
+  height: -webkit-fill-available;
   position: relative;
   white-space: nowrap;
   z-index: @zindexTabs;
@@ -59,6 +59,7 @@
   position: relative;
   vertical-align: top;
   overflow: hidden;
+  height: -webkit-fill-available;
   // There's a special case that tabs should span the full width
   // if there are a full set of them.
   &:not(.partOfFullPageSet) {
@@ -123,14 +124,17 @@
   box-sizing: border-box;
   background: @tabsBackground;
   display: flex;
-  height: @tabsToolbarHeight;
+  // This element is set as border-box so it does not
+  // take into account the borders as width gutter, so we
+  // increase its size by 1px to include the top border
+  height: calc(~'@{tabsToolbarHeight} + 1px');
   border-top: 1px solid @tabsToolbarBorderColor;
   user-select: none;
 }
 
 .tabsToolbarButtons {
   box-sizing: border-box;
-  height: @tabHeight;
+  height: -webkit-fill-available;
   padding-right: 5px;
 
   .browserButton {
@@ -222,6 +226,8 @@
 }
 
 .pinnedTabs {
+  height: -webkit-fill-available;
+  box-sizing: border-box;
   margin-left: 0;
   margin-top: 0;
   background: @tabsBackgroundInactive;