regression - Clicking inside Extension Popup Dismisses Popup

[jonathansampson]

• Did you search for similar issues before submitting this one?
  Yes

• Describe the issue you encountered:
  Extension popup windows are closing unexpectedly.

• Platform (Win7, 8, 10? macOS? Linux distro?):
  Windows 10 / Desktop

• Brave Version (revision SHA):
  dbc3c4d

• Steps to reproduce:

  1. Enable LastPass
  2. Click the browserAction icon
  3. Attempt to focus/click anything in the window

• Actual result:
  The popup window closes

• Expected result:
  Focus is given to the input field.

• Will the steps above reproduce in a fresh profile? If not what other info can be added?
  N/A

• Is this an issue in the currently released version?
  No

• Can this issue be consistently reproduced?
  Yes

• Screenshot if needed:

[srirambv]

Also happens when you enable the extension for the first time and try to login.

[srirambv]

Same issue for Dashlane on Windows. 1Password works fine

[srirambv]

+2 from community https://community.brave.com/t/lastpass-broken-with-latest-brave-update/4916/3

[jonathansampson]

+1 from Twitter: https://twitter.com/waddlesplash/status/888410508730224640

[eljuno]

+3 from community

[i7u]

Seeing this as well on a fresh install on Win10 x64. Please let me know if there's any useful debugging information I can add.

[jonathansampson]

This does not repro for me on the master branch.

[jordan31bit]

+1 happening to me as well.

Brave: 0.17.19
rev: 4e46480
Muon: 4.1.9
libchromiumcontent: 59.0.3071.115
V8: 5.9.211.38
Node.js: 7.9.0
Update Channel: dev
os.platform: linux
os.release: 4.11.0-1-amd64
os.arch: x64 // debian testing

[jonathansampson]

This issue was introduced after 17.16 2d2ee2f

[jabowery]

Workaround until fixed: CTRL-ALT-h brings up the LastPass Vault

[jonathansampson]

The bug repros in our release build (left), but not when I checkout the same commit (right).

[mistertempy]

I have the same problem. Any ETA on a fix? I will have to use a different browser until this is fixed, since I rely on Lastpass for all of my logins.

[asdfractal]

Also had this problem after updating. For me I can still use my passwords because it autofills, but I cannot access any of the menu and I think it is not always saving new passwords. A massive hassle for anyone who uses LastPass exclusively for password management. Hope a fix is implemented ASAP!

[DivineOmega]

It's worth noting once you are automatically logged out of LastPass (as has just happened to me), this bug also prevents use of the login form in the popup, making it impossible to login and thus use LastPass at all.

[jordan31bit]

Some posts up someone suggested crtl alt h to load last pass vault. It works been my only workaround today. You can login that way

[DivineOmega]

@Trumpy Thanks, but unfortunately this does not seem to work on Linux. Perhaps because the ALT key is used by the desktop environment for window movement.

However, I have noticed if you click a LastPass autocomplete icon on a website, while logged out, it will open a LastPass login form in a new tab. This didn't seem to log me in correctly, however that might just be my mistake.

[bsclifton]

This is really unusual... when I go and do packaged builds (on Windows), this commit works:
2d2ee2f

While this commit (the very next commit) fails:
ce7e897

All I'm doing to package the builds is run:

git reset --hard <SHA HERE>
rm -rf node_modules
npm install
CHANNEL=dev npm run build-package

This drops the executable at Brave-win32-x64/Brave.exe which I am then running.

[bsclifton]

Disregard my last post- after testing again, it doesn't work in either case.

Is it a problem with the extension itself? Unfortunately, unable to debug using:
chrome-extension://hdokiejnpimakedhajhdlcegeplioahd/_generated_background_page.html

[jonathansampson]

Because LastPass ships their own background page, the proper debugging URL is: chrome-extension://hdokiejnpimakedhajhdlcegeplioahd/background.html

I also don't think this is a LastPass issue, since #10029 (comment) suggests Dashlane too. 1Password wouldn't be affected since they open their own Native Window over the top of Brave.

[jonathansampson]

This issue is not limited to LastPass. It applies to all extensions using standard popups, as far as I can tell. Here's a GIF of the moarTLS extension doing the same thing:

[jonathansampson]

After speaking with @kevinlawler, we came upon some rather key information. The core bit of logic in question is found here:

  onMouseDown (e) {
    // TODO(bsclifton): update this to use eventUtil.eventElHasAncestorWithClasses
    let node = e.target
    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'))) {
        // 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) {
          e.preventDefault()
        }
        // Click event is handled downstream
        return
      }
      node = node.parentNode
    }

    // Hide context menus, popup menus, and menu selections
    windowActions.resetMenuState()
  }

At times. e.target is a <webview> that houses the extension popup. This element has no classes, and therefore doesn't allow us to fall into the return path. Other times, e.target is the <div> that holds the webview, which has a className that starts with "popupWindow". This takes us down the return path.

On the Release build of Brave, the popupWindow suffix is not being added. The resulting DIV looks like this:

<div class="_yfkq6x-o_O-_142fl4c" style="height: 459px; top: 42px; width: 352px; right: 1em;">
    <webview tabindex="-1" src="chrome-extension://hdokiejnpimakedhajhdlcegeplioahd/extensionLogin.html" style="height: 457px; width: 350px;"></webview>
</div>

This suggests to me that it may be something to do with Aphrodite; perhaps an optimization to cut-down on generated markup. @cezaraugusto @bsclifton?

[jonathansampson]

I think we're minifying classNames for release. https://github.com/Khan/aphrodite#minify-class-names