From a90027f7f10cb612599a2bc12b4b6f5a5c962bb8 Mon Sep 17 00:00:00 2001
From: Cezar Augusto <cezaraugusto@outlook.it>
Date: Tue, 21 Mar 2017 04:21:44 -0300
Subject: [PATCH] Adjust min-threshold for closeTab

Auditors: @bbondy, @bsclifton

Fix #5431
---
 app/renderer/components/styles/global.js |  2 +-
 app/renderer/components/tabContent.js    | 40 +++++++-----------
 app/renderer/lib/tabUtil.js              | 30 ++++++++++++++
 js/components/tab.js                     |  8 +++-
 test/unit/app/renderer/tabContentTest.js | 52 +++++++++++++++++++++++-
 5 files changed, 103 insertions(+), 29 deletions(-)

diff --git a/app/renderer/components/styles/global.js b/app/renderer/components/styles/global.js
index 1e942a3f25a..9fea2f140c0 100644
--- a/app/renderer/components/styles/global.js
+++ b/app/renderer/components/styles/global.js
@@ -11,7 +11,7 @@ const globalStyles = {
       medium: '66px',
       mediumSmall: '53px',
       small: '46px',
-      extraSmall: '33px',
+      extraSmall: '40px',
       smallest: '19px'
     }
   },
diff --git a/app/renderer/components/tabContent.js b/app/renderer/components/tabContent.js
index cd67f5bbd6f..2a8e059edd9 100644
--- a/app/renderer/components/tabContent.js
+++ b/app/renderer/components/tabContent.js
@@ -9,6 +9,7 @@ const globalStyles = require('./styles/global')
 const {isWindows} = require('../../common/lib/platformUtil')
 const {getTextColorForBackground} = require('../../../js/lib/color')
 const {tabs} = require('../../../js/constants/config')
+const {hasBreakpoint, hasRelativeCloseIcon, hasFixedCloseIcon} = require('../lib/tabUtil')
 
 const newSessionSvg = require('../../extensions/brave/img/tabs/new_session.svg')
 
@@ -69,11 +70,16 @@ class Favicon extends ImmutableComponent {
     return this.props.tab.get('breakpoint') === 'smallest'
   }
 
+  get shouldHideFavicon () {
+    return (hasBreakpoint(this.props, 'extraSmall') && this.props.isActive) ||
+    this.props.tab.get('location') === 'about:newtab'
+  }
+
   render () {
     const iconStyles = StyleSheet.create({
       favicon: {backgroundImage: `url(${this.favicon})`}
     })
-    return this.props.tab.get('location') !== 'about:newtab'
+    return !this.shouldHideFavicon
       ? <TabIcon
         data-test-favicon={this.favicon}
         data-test-id={this.loadingIcon ? 'loading' : 'defaultIcon'}
@@ -197,15 +203,10 @@ class TabTitle extends ImmutableComponent {
     return !!this.props.tab.get('pinnedLocation')
   }
 
-  get hoveredOnNarrowView () {
-    const sizes = ['mediumSmall', 'small', 'extraSmall', 'smallest']
-    return this.props.tab.get('hoverState') && sizes.includes(this.props.tab.get('breakpoint'))
-  }
-
   get shouldHideTitle () {
     return (this.props.tab.get('breakpoint') === 'mediumSmall' && this.locationHasSecondaryIcon) ||
-      this.props.tab.get('breakpoint') === 'extraSmall' || this.props.tab.get('breakpoint') === 'smallest' ||
-      this.hoveredOnNarrowView
+      hasBreakpoint(this.props, ['extraSmall', 'smallest']) ||
+      hasFixedCloseIcon(this.props)
   }
 
   get themeColor () {
@@ -220,12 +221,6 @@ class TabTitle extends ImmutableComponent {
 
   render () {
     const titleStyles = StyleSheet.create({
-      reduceTitleSize: {
-        // include a margin gutter with same size
-        // as closeTabIcon to avoid title overflow
-        // when hovering over a tab
-        marginRight: `calc(${globalStyles.spacing.iconSize} + ${globalStyles.spacing.defaultIconPadding})`
-      },
       gradientText: {
         backgroundImage: `-webkit-linear-gradient(left,
         ${this.themeColor} 90%, ${globalStyles.color.almostInvisible} 100%)`
@@ -237,7 +232,6 @@ class TabTitle extends ImmutableComponent {
       className={css(
       styles.tabTitle,
       titleStyles.gradientText,
-      this.props.tab.get('hoverState') && titleStyles.reduceTitleSize,
       // Windows specific style
       isWindows() && styles.tabTitleForWindows
     )}>
@@ -252,13 +246,9 @@ class CloseTabIcon extends ImmutableComponent {
     return !!this.props.tab.get('pinnedLocation')
   }
 
-  get narrowView () {
-    const sizes = ['extraSmall', 'smallest']
-    return sizes.includes(this.props.tab.get('breakpoint'))
-  }
-
   render () {
-    return this.props.tab.get('hoverState') && !this.narrowView && !this.isPinned
+    return !this.isPinned &&
+    (hasRelativeCloseIcon(this.props) || hasFixedCloseIcon(this.props))
       ? <TabIcon
         data-test-id='closeTabIcon'
         className={css(styles.closeTab)}
@@ -311,17 +301,17 @@ const styles = StyleSheet.create({
 
   closeTab: {
     opacity: '0.7',
-    position: 'absolute',
+    position: 'relative',
     top: '0',
     right: '0',
-    padding: '0 4px',
-    borderTopRightRadius: globalStyles.radius.borderRadius,
+    padding: '0',
+    borderRadius: globalStyles.radius.borderRadius,
     display: 'flex',
     alignItems: 'center',
     justifyContent: 'center',
     fontSize: globalStyles.spacing.iconSize,
     width: globalStyles.spacing.iconSize,
-    height: '100%',
+    height: globalStyles.spacing.iconSize,
     border: '0',
     zIndex: globalStyles.zindex.zindexTabs,
 
diff --git a/app/renderer/lib/tabUtil.js b/app/renderer/lib/tabUtil.js
index 5de587a5eab..c29521476f5 100644
--- a/app/renderer/lib/tabUtil.js
+++ b/app/renderer/lib/tabUtil.js
@@ -25,3 +25,33 @@ module.exports.getTabBreakpoint = (tabWidth) => {
 
 // Execute resize handler at a rate of 15fps
 module.exports.tabUpdateFrameRate = 66
+
+/**
+ * Check whether or not current breakpoint match defined criteria
+ * @param {Object} props - Object that hosts the tab breakpoint
+ * @param {Array} arr - Array of Strings including breakpoint names to check against
+ * @returns {Boolean} Whether or not the sizing criteria was match
+ */
+module.exports.hasBreakpoint = (props, arr) => {
+  arr = Array.isArray(arr) ? arr : [arr]
+  return arr.includes(props.tab.get('breakpoint'))
+}
+
+/**
+ * Check whether or not closeTab icon is relative to hover state
+ * @param {Object} props - Object that hosts the tab props
+ * @returns {Boolean} Whether or not the tab has a relative closeTab icon
+ */
+module.exports.hasRelativeCloseIcon = (props) => {
+  return props.tab.get('hoverState') &&
+    !module.exports.hasBreakpoint(props, ['small', 'extraSmall', 'smallest'])
+}
+
+/**
+ * Check whether or not closeTab icon is always visible (fixed) in tab
+ * @param {Object} props - Object that hosts the tab props
+ * @returns {Boolean} Whether or not the close icon is always visible (fixed)
+ */
+module.exports.hasFixedCloseIcon = (props) => {
+  return props.isActive && module.exports.hasBreakpoint(props, ['small', 'extraSmall'])
+}
diff --git a/js/components/tab.js b/js/components/tab.js
index 8bbe30886b4..d6cb2c72857 100644
--- a/js/components/tab.js
+++ b/js/components/tab.js
@@ -289,7 +289,12 @@ class Tab extends ImmutableComponent {
           this.narrowView && styles.tabIdNarrowView,
           this.props.tab.get('breakpoint') === 'smallest' && styles.tabIdMinAllowedSize
           )}>
-          <Favicon tab={this.props.tab} isLoading={this.loading} isPinned={this.isPinned} />
+          <Favicon
+            isActive={this.props.isActive}
+            tab={this.props.tab}
+            isLoading={this.loading}
+            isPinned={this.isPinned}
+          />
           <AudioTabIcon
             tab={this.props.tab}
             onClick={this.onMuteFrame.bind(this, !this.props.tab.get('audioMuted'))}
@@ -310,6 +315,7 @@ class Tab extends ImmutableComponent {
           l10nId='sessionInfoTab'
         />
         <CloseTabIcon
+          isActive={this.props.isActive}
           tab={this.props.tab}
           onClick={this.onTabClosedWithMouse.bind(this)}
           l10nId='closeTabButton'
diff --git a/test/unit/app/renderer/tabContentTest.js b/test/unit/app/renderer/tabContentTest.js
index c67c30607bb..b6afd5f33c4 100644
--- a/test/unit/app/renderer/tabContentTest.js
+++ b/test/unit/app/renderer/tabContentTest.js
@@ -376,14 +376,62 @@ describe('tabContent components', function () {
         <CloseTabIcon
           tab={
             Immutable.Map({
-              hoverState: false,
+              hoverState: true,
               pinnedLocation: true
             })}
         />
       )
       assert.notEqual(wrapper.props().symbol, globalStyles.appIcons.closeTab)
     })
-    it('should not show closeTab icon if tab size is too small', function () {
+    it('should show closeTab icon if tab size is small and tab is active', function () {
+      const wrapper = shallow(
+        <CloseTabIcon isActive
+          tab={
+            Immutable.Map({
+              hoverState: false,
+              breakpoint: 'small'
+            })}
+        />
+      )
+      assert.equal(wrapper.props().symbol, globalStyles.appIcons.closeTab)
+    })
+    it('should not show closeTab icon if tab size is small and tab is not active', function () {
+      const wrapper = shallow(
+        <CloseTabIcon isActive={false}
+          tab={
+            Immutable.Map({
+              hoverState: true,
+              breakpoint: 'small'
+            })}
+        />
+      )
+      assert.notEqual(wrapper.props().symbol, globalStyles.appIcons.closeTab)
+    })
+    it('should show closeTab icon if tab size is extraSmall and tab is active', function () {
+      const wrapper = shallow(
+        <CloseTabIcon isActive
+          tab={
+            Immutable.Map({
+              hoverState: false,
+              breakpoint: 'extraSmall'
+            })}
+        />
+      )
+      assert.equal(wrapper.props().symbol, globalStyles.appIcons.closeTab)
+    })
+    it('should not show closeTab icon if tab size is extraSmall and tab is not active', function () {
+      const wrapper = shallow(
+        <CloseTabIcon isActive={false}
+          tab={
+            Immutable.Map({
+              hoverState: true,
+              breakpoint: 'extraSmall'
+            })}
+        />
+      )
+      assert.notEqual(wrapper.props().symbol, globalStyles.appIcons.closeTab)
+    })
+    it('should not show closeTab icon if tab size is the smallest size', function () {
       const wrapper = shallow(
         <CloseTabIcon
           tab={