Refactor siteInfo.js with Aphrodite

Closes #7949

Also modify messageBox.js, which was introduced with #7107

- Introduced globalStyles.spacing.dialogInsideMargin
  As the marginTop of title and marginBottom of buttons on messageBox.js had been set to 0.5em, I removed and added them to padding of flyoutDialog. The vaule was calculated this way: calc(10px + 0.5rem) = 18px. Also I applied it to the elements inside messageBox.js

- Aligned the buttons to the right (same as messageBox.js)

- Introduced siteInfo__wrapperLarge
  As the buttons on mixed content info dialog are so huge that sometimes they are wrapped, epecially on foreign language like Japanese. This is a temporary workaround to avoid it.

- Moved 'denyRunInsecureContent' button to the right to improve UX

- Updated automated test code

Auditors:

Test Plan:
1. Open https://apple.com
2. Click the lock icon on the URL bar
3. Make sure the title and description are aligned
4. Make sure the button is aligned to the right
5. Open http://apple.com
6. Click the unlock icon on the URL bar
7. Make sure the title and description are aligned
8. Open https://mixed-script.badssl.com
9. Click the lock icon
10. Make sure the buttons are aligned to the right and they are not wrapped
11. Click "Load Unsafe Script"
12. Click the unlock icon on the URL bar
13. Click "Stop Loading Unsafe Script"
14. Visit http://jsbin.com/fiyojusahu/edit?html,output
15. Make sure the elements are aligned well
16. Visit http://jsbin.com/sadunogefu/edit?html,output
17. Make sure the switch next to "Prevent this page from creating additional dialogs" is aligned with the body text

app/renderer/components/messageBox.js

@@ -11,8 +11,10 @@ const appActions = require('../../../js/actions/appActions')
 const KeyCodes = require('../../common/constants/keyCodes')
 const config = require('../../../js/constants/config')
 const {makeImmutable} = require('../../common/state/immutableUtil')
+
 const {StyleSheet, css} = require('aphrodite')
 const commonStyles = require('./styles/commonStyles')
+const globalStyles = require('./styles/global')
 
 class MessageBox extends ImmutableComponent {
   constructor () {
@@ -119,6 +121,8 @@ class MessageBox extends ImmutableComponent {
         {
           this.showSuppress
             ? <SwitchControl
+              // TODO: refactor SwitchControl
+              className={css(commonStyles.noPaddingLeft)}
               rightl10nId='preventMoreAlerts'
               checkedOn={this.suppress}
               onClick={this.onSuppressChanged} />
@@ -143,21 +147,19 @@ const styles = StyleSheet.create({
   title: {
     fontWeight: 'bold',
     fontSize: '12pt',
-    marginBottom: '1em',
-    marginTop: '0.5em',
+    marginBottom: globalStyles.spacing.dialogInsideMargin,
     '-webkit-user-select': 'text'
   },
   body: {
-    marginTop: '1.5em',
+    marginTop: globalStyles.spacing.dialogInsideMargin,
     minWidth: '425px',
-    marginBottom: '1.5em',
+    marginBottom: globalStyles.spacing.dialogInsideMargin,
     '-webkit-user-select': 'text'
   },
   buttons: {
     display: 'flex',
     justifyContent: 'flex-end',
-    marginTop: '1.5em',
-    marginBottom: '0.5em'
+    marginTop: globalStyles.spacing.dialogInsideMargin
   }
 })
 

app/renderer/components/styles/commonStyles.js

@@ -20,13 +20,16 @@ const styles = StyleSheet.create({
     padding: '0.4em',
     width: '100%'
   },
+
+  // Dialogs
   flyoutDialog: {
     background: globalStyles.color.toolbarBackground,
     borderRadius: globalStyles.radius.borderRadius,
     boxShadow: '2px 2px 8px #3b3b3b',
     color: '#000',
     fontSize: '13px',
-    padding: '10px 30px',
+    // Issue #7949
+    padding: `${globalStyles.spacing.dialogInsideMargin} 30px`,
     position: 'absolute',
     top: globalStyles.spacing.dialogTopOffset
   },

js/components/siteInfo.js

@@ -15,6 +15,10 @@ const messages = require('../constants/messages')
 const siteUtil = require('../state/siteUtil')
 const platformUtil = require('../../app/common/lib/platformUtil')
 
+const {StyleSheet, css} = require('aphrodite/no-important')
+const globalStyles = require('../../app/renderer/components/styles/global')
+const commonStyles = require('../../app/renderer/components/styles/commonStyles')
+
 class SiteInfo extends ImmutableComponent {
   constructor () {
     super()
@@ -67,81 +71,150 @@ class SiteInfo extends ImmutableComponent {
     let l10nArgs = {
       partitionNumber: this.partitionNumber
     }
+
     let secureIcon
     if (this.isSecure === true && !this.runInsecureContent) {
       // fully secure
-      secureIcon = <li><span
-        className={cx({
-          fa: true,
-          'fa-lock': true,
-          extendedValidation: this.isExtendedValidation
-        })} /><span data-l10n-id='secureConnection' /></li>
+      secureIcon =
+        <div className={css(styles.secureIcon__wrapper)}>
+          <span className={cx({
+            fa: true,
+            'fa-lock': true,
+            [css(styles.secureIcon__fa)]: true,
+            [css(styles.secureIcon__extendedValidation)]: this.isExtendedValidation
+          })} />
+          <span className={css(styles.secureIcon__label)}
+            data-test-id='secureConnection'
+            data-l10n-id='secureConnection'
+          />
+        </div>
     } else if (this.isSecure === 1 && !this.runInsecureContent) {
       // passive mixed content loaded
-      secureIcon = <li><span className='fa fa-unlock' /><span data-l10n-id='partiallySecureConnection' /></li>
+      secureIcon =
+        <div className={css(styles.secureIcon__wrapper)}>
+          <span className={cx({
+            fa: true,
+            'fa-unlock': true,
+            [css(styles.secureIcon__fa)]: true
+          })} />
+          <span className={css(styles.secureIcon__label)}
+            data-test-id='partiallySecureConnection'
+            data-l10n-id='partiallySecureConnection'
+          />
+        </div>
     } else {
       // insecure
-      secureIcon = <li><span className='fa fa-unlock' /><span data-l10n-id='insecureConnection' /></li>
+      secureIcon =
+        <div className={css(styles.secureIcon__wrapper)}>
+          <span className={cx({
+            fa: true,
+            'fa-unlock': true,
+            [css(styles.secureIcon__fa)]: true
+          })} />
+          <span className={css(styles.secureIcon__label)}
+            data-test-id='insecureConnection'
+            data-l10n-id='insecureConnection'
+          />
+        </div>
     }
 
     let partitionInfo
     if (this.partitionNumber) {
-      partitionInfo = <li><span className='fa fa-user' />
-        <span data-l10n-args={JSON.stringify(l10nArgs)} data-l10n-id='sessionInfo' /></li>
+      partitionInfo =
+        <div className={css(styles.secureIcon__wrapper)}>
+          <span className={cx({
+            fa: true,
+            'fa-user': true,
+            [css(styles.secureIcon__fa)]: true
+          })} />
+          <span className={css(styles.secureIcon__label)}
+            data-test-id='partitionNumber'
+            data-l10n-args={JSON.stringify(l10nArgs)}
+            data-l10n-id='sessionInfo'
+          />
+        </div>
     }
 
     let connectionInfo = null
     let viewCertificateButton = null
+
     // TODO(Anthony): Hide it until muon support linux
     if (!platformUtil.isLinux()) {
       viewCertificateButton =
-        <Button l10nId='viewCertificate' className='primaryButton viewCertificate' onClick={this.onViewCertificate} />
+        <div className={css(styles.flexJustifyEnd, styles.viewCertificateButton__wrapper)}>
+          <Button l10nId='viewCertificate'
+            className='primaryButton'
+            testId='viewCertificate'
+            onClick={this.onViewCertificate}
+          />
+        </div>
     }
 
     if (this.maybePhishingLocation) {
       connectionInfo =
-        <div className='connectionInfo' data-l10n-id='phishingConnectionInfo' />
+        <div className={css(styles.connectionInfo__wrapper)}>
+          <div data-l10n-id='phishingConnectionInfo' data-test-id='phishingConnectionInfo' />
+        </div>
     } else if (this.isBlockedRunInsecureContent) {
       connectionInfo =
-        <li>
-          <ul>
-            <li><span className='runInsecureContentWarning' data-l10n-id='runInsecureContentWarning' /></li>
-            <li>
-              <Button l10nId='allowRunInsecureContent' className='whiteButton allowRunInsecureContentButton' onClick={this.onAllowRunInsecureContent} />
-              <Button l10nId='dismissAllowRunInsecureContent' className='primaryButton dismissAllowRunInsecureContentButton' onClick={this.props.onHide} />
-            </li>
-          </ul>
-        </li>
+        <div className={css(styles.connectionInfo__wrapper)}>
+          <div data-test-id='runInsecureContentWarning' data-l10n-id='runInsecureContentWarning' />
+          <div className={css(styles.flexJustifyEnd, styles.viewCertificateButton__wrapper)}>
+            <Button l10nId='allowRunInsecureContent'
+              className='whiteButton'
+              testId='allowRunInsecureContentButton'
+              onClick={this.onAllowRunInsecureContent}
+            />
+            <Button l10nId='dismissAllowRunInsecureContent'
+              className='primaryButton'
+              testId='dismissAllowRunInsecureContentButton'
+              onClick={this.props.onHide}
+            />
+          </div>
+        </div>
     } else if (this.runInsecureContent) {
       connectionInfo =
-        <li>
-          <ul>
-            <li><span className='denyRunInsecureContentWarning' data-l10n-id='denyRunInsecureContentWarning' /></li>
-            <li>
-              <Button l10nId='denyRunInsecureContent' className='primaryButton denyRunInsecureContentButton' onClick={this.onDenyRunInsecureContent} />
-              <Button l10nId='dismissDenyRunInsecureContent' className='whiteButton dismissDenyRunInsecureContentButton' onClick={this.props.onHide} />
-            </li>
-          </ul>
-        </li>
+        <div className={css(styles.connectionInfo__wrapper)}>
+          <div data-test-id='denyRunInsecureContentWarning' data-l10n-id='denyRunInsecureContentWarning' />
+          <div className={css(styles.flexJustifyEnd, styles.viewCertificateButton__wrapper)}>
+            <Button l10nId='dismissDenyRunInsecureContent'
+              className='whiteButton'
+              testId='dismissDenyRunInsecureContentButton'
+              onClick={this.props.onHide}
+            />
+            <Button l10nId='denyRunInsecureContent'
+              className='primaryButton'
+              testId='denyRunInsecureContentButton'
+              onClick={this.onDenyRunInsecureContent}
+            />
+          </div>
+        </div>
     } else if (this.isSecure === true) {
       connectionInfo =
-        <div>
-          <div className='connectionInfo' data-l10n-id='secureConnectionInfo' />
+        <div className={css(styles.connectionInfo__wrapper)}>
+          <div data-l10n-id='secureConnectionInfo' />
           {viewCertificateButton}
         </div>
     } else if (this.isSecure === 1) {
       connectionInfo =
-        <div>
-          <div className='connectionInfo' data-l10n-id='partiallySecureConnectionInfo' />
+        <div className={css(styles.connectionInfo__wrapper)}>
+          <div data-l10n-id='partiallySecureConnectionInfo' />
           {viewCertificateButton}
         </div>
     } else {
       connectionInfo =
-        <div className='connectionInfo' data-l10n-id='insecureConnectionInfo' />
+        <div className={css(styles.connectionInfo__wrapper)}>
+          <div data-l10n-id='insecureConnectionInfo' />
+        </div>
     }
 
     return <Dialog onHide={this.props.onHide} className='siteInfo' isClickDismiss>
-      <ul onClick={(e) => e.stopPropagation()}>
+      <div onClick={(e) => e.stopPropagation()}
+        className={cx({
+          [css(commonStyles.flyoutDialog)]: true,
+          [css(styles.siteInfo__wrapper)]: true,
+          [css(styles.siteInfo__wrapper__large)]: (this.isBlockedRunInsecureContent || this.runInsecureContent)
+        })}>
         {
           secureIcon
         }
@@ -151,7 +224,7 @@ class SiteInfo extends ImmutableComponent {
         {
           connectionInfo
         }
-      </ul>
+      </div>
     </Dialog>
   }
 }
@@ -161,4 +234,46 @@ SiteInfo.propTypes = {
   onHide: React.PropTypes.func
 }
 
+const styles = StyleSheet.create({
+  flexJustifyEnd: {
+    display: 'flex',
+    justifyContent: 'flex-end'
+  },
+
+  secureIcon__wrapper: {
+    display: 'flex',
+    alignItems: 'center'
+  },
+  secureIcon__fa: {
+    position: 'absolute'
+  },
+  secureIcon__extendedValidation: {
+    color: '#008000'
+  },
+  secureIcon__label: {
+    position: 'relative',
+    left: globalStyles.spacing.dialogInsideMargin
+  },
+
+  viewCertificateButton__wrapper: {
+    marginTop: globalStyles.spacing.dialogInsideMargin
+  },
+
+  connectionInfo__wrapper: {
+    display: 'flex',
+    flexFlow: 'column nowrap',
+    margin: `${globalStyles.spacing.dialogInsideMargin} 0 0 ${globalStyles.spacing.dialogInsideMargin}`
+  },
+
+  siteInfo__wrapper: {
+    maxHeight: '300px',
+    maxWidth: '400px',
+    width: 'auto'
+  },
+  siteInfo__wrapper__large: {
+    // temporary workaround
+    maxWidth: '500px'
+  }
+})
+
 module.exports = SiteInfo

less/dialogs.less

@@ -83,7 +83,3 @@
     }
   }
 }
-.connectionInfo {
-  max-width: 400px;
-  padding: 10px;
-}

less/forms.less

@@ -18,34 +18,6 @@ select {
   top: @dialogTopOffset;
 }
 
-.siteInfo {
-  >ul {
-    .flyoutDialog;
-    text-align: left;
-    width: auto;
-
-    li {
-      list-style-type: none;
-      padding: 10px 0;
-
-      .fa {
-        padding-right: 10px;
-      }
-      &.extendedValidation {
-        color: green;
-      }
-
-      ul {
-        max-height: 300px;
-        overflow-y: auto;
-        margin-top: -20px;
-        padding: 0 10px;
-        max-width: 400px;
-      }
-    }
-  }
-}
-
 .clearBrowsingDataPanel {
   .clearBrowsingData {
     .flyoutDialog;

test/lib/selectors.js

@@ -91,10 +91,10 @@ module.exports = {
   paymentsWelcomePage: '[data-test-id="paymentsMessage"]',
   autofillAddressPanel: '.autofillAddressPanel',
   autofillCreditCardPanel: '.autofillCreditCardPanel',
-  allowRunInsecureContentButton: '.allowRunInsecureContentButton',
-  dismissAllowRunInsecureContentButton: '.dismissAllowRunInsecureContentButton',
-  denyRunInsecureContentButton: '.denyRunInsecureContentButton',
-  dismissDenyRunInsecureContentButton: '.dismissDenyRunInsecureContentButton',
+  allowRunInsecureContentButton: '[data-test-id="allowRunInsecureContentButton"]',
+  dismissAllowRunInsecureContentButton: '[data-test-id="dismissAllowRunInsecureContentButton"]',
+  denyRunInsecureContentButton: '[data-test-id="denyRunInsecureContentButton"]',
+  dismissDenyRunInsecureContentButton: '[data-test-id="dismissDenyRunInsecureContentButton"]',
   tabsToolbar: '.tabsToolbar',
   hamburgerMenu: '.menuButton',
   contextMenu: '.contextMenu',