Merge pull request #10165 from brave/10133

do not rely on classList for event target checks

Author: bsclifton

app/renderer/components/bookmarks/addEditBookmarkHanger.js

@@ -193,7 +193,7 @@ class AddEditBookmarkHanger extends React.Component {
   }
 
   render () {
-    return <Dialog className={cx({
+    return <Dialog bookmarkHanger className={cx({
       bookmarkDialog: this.props.isModal,
       bookmarkHanger: !this.props.isModal,
       [css(styles.bookmarkHanger)]: !this.props.isModal

app/renderer/components/common/browserButton.js

@@ -59,6 +59,7 @@ class BrowserButton extends ImmutableComponent {
   render () {
     return <button
       disabled={this.props.disabled}
+      data-extension-button={this.props.extensionButton}
       data-l10n-id={this.props.l10nId}
       data-test-id={this.props.testId}
       data-test2-id={this.props.test2Id}

app/renderer/components/common/contextMenu.js

@@ -474,6 +474,7 @@ class ContextMenu extends ImmutableComponent {
       styles.maxHeight = this.props.contextMenuDetail.get('maxHeight')
     }
     return <div
+      data-context-menu
       ref={(node) => { this.node = node }}
       className={cx({
         contextMenu: true,

app/renderer/components/common/dialog.js

@@ -41,11 +41,13 @@ class Dialog extends ImmutableComponent {
   }
 
   render () {
-    return <div className={cx({
-      [css(styles.dialog)]: true,
-      [css(styles.dialog_isNotClickDismiss)]: !this.props.isClickDismiss,
-      [this.props.className]: !!this.props.className
-    })}
+    return <div
+      data-bookmark-hanger={this.props.bookmarkHanger}
+      className={cx({
+        [css(styles.dialog)]: true,
+        [css(styles.dialog_isNotClickDismiss)]: !this.props.isClickDismiss,
+        [this.props.className]: !!this.props.className
+      })}
       data-test-id={this.props.testId}
       data-test2-id={this.props.test2Id}
       tabIndex='-1'

app/renderer/components/main/main.js

@@ -572,18 +572,19 @@ class Main extends ImmutableComponent {
   }
 
   onMouseDown (e) {
-    // TODO(bsclifton): update this to use eventUtil.eventElHasAncestorWithClasses
     let node = e.target
+    const datasets = [
+      'popupWindow',
+      'contextMenu',
+      'extensionButton',
+      'menubarItem',
+      'bookmarkHanger'
+    ]
     while (node) {
-      if (node.classList &&
-          (node.matches('[class^="popupWindow"]') ||
-            node.classList.contains('contextMenu') ||
-            node.matches('[class*="extensionButton_"]') ||
-            node.classList.contains('menubarItem') ||
-            node.classList.contains('bookmarkHanger'))) {
+      if (eventUtil.elementHasDataset(node, datasets)) {
         // Middle click (on context menu) needs to fire the click event.
         // We need to prevent the default "Auto-Scrolling" behavior.
-        if (node.classList.contains('contextMenu') && e.button === 1) {
+        if (eventUtil.elementHasDataset(node, 'contextMenu') && e.button === 1) {
           e.preventDefault()
         }
         // Click event is handled downstream

app/renderer/components/main/popupWindow.js

@@ -106,6 +106,7 @@ class PopupWindow extends ImmutableComponent {
     }
 
     return <div
+      data-popup-window
       className={css(
         styles.popupWindow,
         style.right !== undefined && styles.reverseExpand

app/renderer/components/navigation/browserAction.js

@@ -80,6 +80,7 @@ class BrowserAction extends React.Component {
     // TODO(bridiver) should have some visual notification of hover/press
     return <div className={css(styles.browserActionButton)}>
       <BrowserButton
+        extensionButton
         extensionItem
         l10nId='browserActionButton'
         testId='extensionBrowserAction'

app/renderer/components/navigation/menuBarItem.js

@@ -65,6 +65,7 @@ class MenuBarItem extends React.Component {
 
   render () {
     return <span
+      data-menubar-item
       className={'menubarItem' + (this.props.selected ? ' selected' : '')}
       onClick={this.onClick}
       onMouseOver={this.onMouseOver}

js/lib/eventUtil.js

@@ -10,6 +10,12 @@ module.exports.isForSecondaryAction = (e) =>
   e.button === 1
 
 module.exports.eventElHasAncestorWithClasses = (e, classesToCheck) => {
+  // DO NOT ADD NEW CHECKS USING THIS METHOD
+  // classNames are changed from dev to prod by Aphrodite est. v1.2.3
+  // and new code will not work. Consider using dataset attribute instead.
+  // See issue #10029 for a breaking example.
+  // ....
+  // TODO deprecate this method.
   let node = e.target
 
   while (node) {
@@ -28,3 +34,20 @@ module.exports.eventElHasAncestorWithClasses = (e, classesToCheck) => {
 
   return false
 }
+
+/**
+ * Checks if a given node dataset matches a given dataset or array of datasets
+ * @param {Object} The node to check if dataset is included
+ * @param {Array|String} the dataset value to check against
+ * @returns {Boolean} Whether or not the given dataset is included in the check
+ */
+module.exports.elementHasDataset = (node, datasetArray) => {
+  if (!node.dataset) {
+    return false
+  }
+
+  const datasetToMatch = Array.isArray(datasetArray) ? datasetArray : [datasetArray]
+  const elementDataset = Object.keys(node.dataset)
+
+  return elementDataset.some(dataset => datasetToMatch.includes(dataset))
+}

test/unit/js/lib/eventUtilTest.js

@@ -0,0 +1,38 @@
+/* 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/. */
+/* global describe, it */
+
+const eventUtil = require('../../../../js/lib/eventUtil')
+const assert = require('assert')
+
+require('../../braveUnit')
+
+describe('eventUtil', function () {
+  describe('elementHasDataset', function () {
+    let datasetToTest
+    let node = { dataset: { nespresso: true } }
+
+    it('returns false if node dataset do not match', function () {
+      datasetToTest = ['starbucks', 'keurig']
+      assert.equal(eventUtil.elementHasDataset(node, datasetToTest), false)
+    })
+    it('returns true if node dataset match the provided dataset array', function () {
+      datasetToTest = ['wow', 'such', 'nespresso', 'very', 'amazing']
+      assert.equal(eventUtil.elementHasDataset(node, datasetToTest), true)
+    })
+    it('can accept strings for the dataset to match against', function () {
+      datasetToTest = 'nespresso'
+      assert.equal(eventUtil.elementHasDataset(node, datasetToTest), true)
+    })
+    it('can not accept partial string match', function () {
+      datasetToTest = ['nespressomnibox']
+      assert.equal(eventUtil.elementHasDataset(node, datasetToTest), false)
+    })
+    it('returns false if node do not provide a dataset', function () {
+      node = delete node.dataset
+      datasetToTest = ['nespresso']
+      assert.equal(eventUtil.elementHasDataset(node, datasetToTest), false)
+    })
+  })
+})