Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

do not rely on classList for event target checks #10165

Merged
merged 2 commits into from
Aug 1, 2017
Merged

do not rely on classList for event target checks #10165

merged 2 commits into from
Aug 1, 2017

Conversation

cezaraugusto
Copy link
Contributor

@cezaraugusto cezaraugusto commented Jul 27, 2017

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.

Auditors: @bsclifton, @jonathansampson
Close #10133

Test Plan:

npm run test -- --grep="eventUtil"

Related: #10062

Manual test plan (cc @luixxiul):

  1. Update Aphrodite to 1.2.3 in package.json
  2. Open Devtool with Shift+F8.
  3. Check that styles for tabs should show the style concatenated
  4. You should be able to click extension button and open extensions popup, which should be persistent;
  5. Bookmark hanger should work as expected;
  6. Right click over any of the above should not close the dialog.

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

@codecov-io
Copy link

codecov-io commented Jul 27, 2017

Codecov Report

Merging #10165 into master will increase coverage by 0.02%.
The diff coverage is 81.25%.

@@            Coverage Diff             @@
##           master   #10165      +/-   ##
==========================================
+ Coverage   52.93%   52.95%   +0.02%     
==========================================
  Files         228      228              
  Lines       20295    20306      +11     
  Branches     3251     3253       +2     
==========================================
+ Hits        10744    10754      +10     
- Misses       9551     9552       +1
Flag Coverage Δ
#unittest 52.95% <81.25%> (+0.02%) ⬆️
Impacted Files Coverage Δ
app/renderer/components/navigation/menuBarItem.js 20% <ø> (ø) ⬆️
app/renderer/components/common/browserButton.js 69.76% <ø> (ø) ⬆️
...erer/components/bookmarks/addEditBookmarkHanger.js 30.33% <0%> (ø) ⬆️
...pp/renderer/components/navigation/browserAction.js 30.98% <0%> (ø) ⬆️
app/renderer/components/common/dialog.js 84.21% <100%> (ø) ⬆️
js/lib/eventUtil.js 57.14% <90.9%> (+21.84%) ⬆️

@bsclifton bsclifton changed the title do not rely on classLIst for event target checks do not rely on classList for event target checks Jul 27, 2017
@luixxiul
Copy link
Contributor

@cezaraugusto to test the effectiveness of this PR, Aphrodite should be updated to 1.2.3, where we had the issue on the packaged build (because on that version Aphrodite started shrinking the classnames when NODE_ENV = production is set).

@@ -10,6 +10,11 @@ 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cezaraugusto mind specifying Aphrodite version (1.2.3) and adding the number of the issue (#10029)?

@luixxiul
Copy link
Contributor

this should fix #10104 as well

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes work great 😄

Tests I tried

  • Enabling LastPass and making sure the popup menu works
  • Adding bookmark using star icon and dismissing by clicking outside modal
  • Editing bookmark (opens hanger) and dismissing by clicking outside modal
  • Launching on Windows and using alt to select a menu, then dismiss by clicking outside menu
  • Launching on Windows and using alt to open a menu, then dismiss by clicking outside menu

++


require('../../braveUnit')

describe('eventUtil', function () {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cezaraugusto can you add a test for a partial match? like nespressoPods and/or love_nespresso?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added can not accept partial string match

@bsclifton
Copy link
Member

@luixxiul @cezaraugusto can you confirm this also closes #10104?

@luixxiul
Copy link
Contributor

luixxiul commented Aug 1, 2017

could I know how to test the binary build with NODE_ENV=production for manual QA? See: #10165 (comment)

@cezaraugusto
Copy link
Contributor Author

@luixxiul pls run NODE_ENV=production npm run watch when running the app. Notice you'll need to shift+f8 to open devtools. Checking styles for tabs should show the style concatenated

@luixxiul
Copy link
Contributor

luixxiul commented Aug 1, 2017

@cezaraugusto I tried NODE_ENV=production npm run watch and it did not work for me. I modified watch and start inside package.json by adding cross-env NODE_ENV=production and that did not work either :-( do you have any tip?

@luixxiul
Copy link
Contributor

luixxiul commented Aug 1, 2017

NODE_ENV=production npm run watch does not seem to work because of this error:

    ERROR in webtorrentPage.entry.js from UglifyJs
    SyntaxError: Name expected [./app/renderer/components/styles/commonStyles.js:5,6]
webpack: Failed to compile.

@bsclifton bsclifton merged commit a562475 into master Aug 1, 2017
@bsclifton bsclifton deleted the 10133 branch August 1, 2017 17:12
bsclifton added a commit that referenced this pull request Aug 1, 2017
do not rely on classList for event target checks
bsclifton added a commit that referenced this pull request Aug 1, 2017
do not rely on classList for event target checks
bsclifton added a commit that referenced this pull request Aug 1, 2017
do not rely on classList for event target checks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rework code to not use class names
4 participants