Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Click Event Triggers on Complex Buttons are ignored in some environments #10366

Open
ericdrobinson opened this issue Aug 7, 2019 · 31 comments · May be fixed by #10459 or #10517
Open

Click Event Triggers on Complex Buttons are ignored in some environments #10366

ericdrobinson opened this issue Aug 7, 2019 · 31 comments · May be fixed by #10459 or #10517

Comments

@ericdrobinson
Copy link

ericdrobinson commented Aug 7, 2019

Version

2.6.10

Reproduction link

https://jsfiddle.net/s7hyqk13/2/

Steps to reproduce

  1. Configure one of the Adobe CEP Sample Panels. The PProPanel is a good starting point as it has very clear documentation on how to set up the environment for testing.
  2. Replace the HTML/JavaScript/CSS contents of the panel project with the contents of the linked JSFiddle.
  3. Open the panel.
  4. Attach a debugger (with the default PProPanel setup this would mean browsing to localhost:7777 in Chrome).
  5. Set the "Mouse > click" Event Listener Breakpoint in the "Sources" tab of the Chrome Debugger.
  6. Click the Vue icon in the center of the silver div.

What is expected?

Method bound to the @click handler is triggered when the image embedded in the parent div is clicked.

What is actually happening?

The method bound to the @click handler is only triggered when clicking outside of the parent div.


This is a non-trivial bug to reproduce as the only place I've experienced it is in Adobe CEP extensions (which run NW.js under the hood). That said, it does reproduce 100% of the time there.

The debugger (CEP context) seems to show several funny things at around this line in the Vue events.js file. Specifically:

  1. The e.timeStamp value does not change between callbacks for different buttons/elements.
  2. The attachedTimestamp is significantly larger than the e.timeStamp value.
  3. The attachedTimestamp value does change when the component is updated (the e.timeStamp remains identical).

I should note that this affects at least CEP 8.0 and CEP 9.0 (tested in Premiere Pro).

Vue Versions Note: This broke somewhere between versions 2.5.17 and 2.6.x. If we run a version of 2.5 (2.5.17 and some earlier versions verified), then this issue does not occur. In testing 2.6.x, we've found that this same issue occurs from 2.6.5 to 2.6.10 (latest). Versions of 2.6 prior to 2.6.5 are actually worse in that the buttons basically don't work at all.

Important Note: I should further note that apparently right-clicking to open the basic CEF [not CEP] context menu will cause the e.timeStamp values to begin reporting as expected. Once this occurs, the buttons will also work as expected. That said, we shouldn't have to instruct users to right-click prior to interfacing with the extension.

@posva
Copy link
Member

posva commented Aug 8, 2019

I don't own Photoshop so I won't be able to test nor debug this problem. Can it be on nw.js as well? Maybe that could be tested

@ericdrobinson
Copy link
Author

ericdrobinson commented Aug 8, 2019

@posva As a quick note, this issue isn't restricted to Photoshop. Just about every major Creative Cloud application supports CEP extensions - and is affected by this problem. 😕

@ericdrobinson
Copy link
Author

ericdrobinson commented Aug 8, 2019

right-clicking to open the basic CEF [not CEP] context menu will cause the e.timeStamp values to begin reporting as expected.

It appears that this isn't quite right. The e.timeStamp value is updated when a right-click occurs. However, subsequent callbacks will then report the same updated value. This is, at least, what the following extremely simple test case shows:

window.addEventListener('click', (evt)=>{console.log(evt.timeStamp);});

This means that a post right-click event will work as expected as long as the attachedTimestamp isn't reset! It appears that once a "flush" occurs and the event listener is re-attached, the issue reappears (until the next right-click).

@ericdrobinson
Copy link
Author

I should point out that it is possible to distinguish a CEP environment and version with standard JavaScript calls. This may be helpful for adding an environment flag.

Specifically, the following snippet is designed to be inserted into the existing environment flag setup:

const  isAdobeCEP = inBrowser && typeof window.__adobe_cep__ !== undefined;

The __adobe_cep__ object also has the getCurrentApiVersion native function, which returns the CEP version encoded as a JSON string. See:

// In Premiere Pro 13.1.3, outputs: "{"minor":3,"micro":1,"major":9}"
window.__adobe_cep__.getCurrentApiVersion();

The version JSON returned by this function may be easily parsed and accessed:

const cepVersion = JSON.parse(window.__adobe_cep_.getCurrentApiVersion());

cepVersion.major; // = 9
cepVersion.minor; // = 3
cepVersion.micro; // = 1

Hopefully this will prove helpful in some way.

@Inventsable
Copy link

Inventsable commented Aug 13, 2019

I made a standalone panel to demonstrate this but don't get the same behavior on Windows 10 and Vue 2.6.10, but I don't have a Mac to test:

Any Mac user should be able to run a single terminal command: defaults write com.adobe.CSXS.9 PlayerDebugMode 1, then clone the repo into the specified directory in the README and open it in Illustrator, After Effects, Photoshop or Premiere Pro via Windows > Extensions > clicktest, then access the CEF debugger easily via the right-click context menu on the panel.


EDIT: Eric corrected me on the version, this is 2.6.10 (not 2.6.1)

@ericdrobinson
Copy link
Author

ericdrobinson commented Aug 14, 2019

[Update: Please see this comment for more on the struck-through text below.]

Thanks to @Inventsable I was able to narrow down the test case a bit more. In order to experience the issue I described, a click event handler must be installed on the contained element (in this case, an <img> element) as well as the container. I have updated the JSFiddle link in the issue description.

[For the record, this was the previous version and this is the adjusted version.]

Also, @Inventsable was able to verify that the timeStamp value appears to update correctly when run in Windows applications. I've tested on macOS-based Premiere Pro and can verify that the timeStamp issue does occur there (even in the graciously provided bug report panel).

@derrickb
Copy link

This is affecting me as well.

  • Vue 2.6.10
  • Photoshop CC 2019
  • macOS 10.14.6

Thank you @ericdrobinson for filing this issue!

@ericdrobinson
Copy link
Author

Update: My previous comment about requiring a contained element to also have an event handler was 100% a red herring. This is entirely unnecessary to get the bug to reproduce.

It appears that small, fast-loading scripts result in the reported event timeStamp being 0. This is unfortunate because it obscures the bug by passing as expected here.

On macOS, the event timeStamp value only increments when a right-click occurs to open a native context menu (provided it's not been disabled, of course). Great. The problem now is that the "attachedTimestamp" is properly less than the currently-constant timeStamp. We need a way to ensure that the following occurs:

  1. The Event.timeStamp value is greater than 0.
  2. The attachedTimestamp is greater than the Event.timeStamp.

That's easy: have the user right-click anywhere and then trigger some code that attaches a new listener!

I've adjusted the JSFiddle example to account for the above to ensure a 100% reproduction rate in Adobe CEP applications (e.g. Photoshop, Premiere Pro, Illustrator, InDesign, After Effects, etc.) running on macOS.

@ericdrobinson
Copy link
Author

ericdrobinson commented Aug 27, 2019

I don't own Photoshop so I won't be able to test nor debug this problem. Can it be on nw.js as well? Maybe that could be tested

@posva We've narrowed the repro for this down to Adobe CEP contexts running on macOS. Adobe is aware of the problem but a fix from them will not address released applications.

From what I can gather by reading this comment from @yyx990803 on #6566, there is an issue here in event dispatch/processing with respect to the micro/macro task order.

Further, it appears that the code has subsequently been updated with a few workarounds "for environments that have buggy event.timeStamp implementations".

It appears that the workaround for the event ordering issue that was implemented in ba0ebd4 to address #6566 ended up breaking things for CEP-on-macOS (as it did with a few other "broken environments").

I would then propose that a solution would be to follow the examples set in 0bad7e2 and 7591b9d and simply bail out of this special-case processing when we detect that we're in the "broken environment" that is CEP-on-macOS. This can be done like so:

  1. Add the following to env.js
    export const isCEP = inBrowser && window.__adobe_cep__;
  2. Add the following to events.js
    // #10366 Adobe CEP bug: event.timeStamp is not reliable
    isCEP ||

You will note that the above solution does not distinguish between macOS and Windows hosts (the latter of which does not have this issue). This would be for cross-platform consistency. It could easily be amended to check for macOS with something akin to the following:

/mac/.test(window.navigator.platform.toLowerCase()); // True on macOS; false otherwise.

I have tested this locally and the above does appear to resolve the issue. Thoughts? Concerns?

@posva
Copy link
Member

posva commented Aug 27, 2019

I imagined it would be that, that's why I was asking about a nw.js version that we could target
It is weird that it doesn't appear on windows though, it would be worth seeing the differences in terms of user agent between them

@ericdrobinson
Copy link
Author

ericdrobinson commented Aug 27, 2019

I have no idea about whether nw.js would be similarly affected. Someone internal at Adobe would need to chime in there. @bbb999 perhaps?

The User Agent reported for Premiere Pro 13.1.4 on macOS 10.14.6 is as follows:

Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.91 Safari/537.36

I've asked others if they can provide the Windows equivalent. Given that the only difference between CEP's User Agent and the Chrome browsers' is the Chrome version, I'd be willing to bet it looks identical to a standard Windows Chrome User Agent for Chrome 61.0.3163.91...

@ericdrobinson
Copy link
Author

@posva Here are the two User Agent versions:

From After Effects 16.1.2 on Windows 10:

Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.91 Safari/537.36

From Premiere Pro 13.1.4 on macOS 10.14.6:

Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.91 Safari/537.36

The CEP version determines the User Agent. See this table for a rough outline.

@posva
Copy link
Member

posva commented Aug 28, 2019

Okay, thanks a lot for the research. In that case I think the best option we have here is to use the check you showed at #10366 (comment).
You could integrate the code modification (there is no need for a test in this scenario) and create a PR. I won't be able to check it since I don't have the adobe suite but hopefully other people could give feedback and it shouldn't break anything for others

@ericdrobinson
Copy link
Author

ericdrobinson commented Aug 28, 2019

@posva Sounds good to me. Do you have any input on whether or not I should add the macOS platform check to restrict the fix to macOS? If yes, do you have a suggestion on detection approach? Either of these should work:

  1. /mac/.test(window.navigator.platform.toLowerCase())
  2. UA && /mac os x/.test(UA)

Also, I just heard that the issue is fixed in the upcoming versions of the Adobe apps. I'm getting version information from them right now. This will allow me to further restrict the workaround to affected versions (the version check can be based on the version code I pointed to at the end of this comment).

@ericdrobinson
Copy link
Author

ericdrobinson commented Aug 28, 2019

Here is a proposal for a snippet of code to add to event.js (somewhere around here):

// #10366: CEP <= 9.3.x on macOS has a buggy Event.timeStamp implementation.
const isMacCEP93orEarlier = isCEPMac && ((maxBadMajor, maxBadMinor) => {
  const version = JSON.parse(window.__adobe_cep__.getCurrentApiVersion())
  return version.major <= maxBadMajor && version.minor <= maxBadMinor
})(9, 3)

This isMacCEP93orEarlier flag would restrict the standard flow elision to only affected CEP versions on macOS.

In the above example, isCEPMac would be defined as follows in env.js:

export const isCEP = inBrowser && window.__adobe_cep__ !== 'undefined'
export const isCEPMac = isCEP && UA && /mac os x/.test(UA)

Does this look reasonable? Are the variable names okay?

(The above code works in my environment.)

@posva
Copy link
Member

posva commented Aug 29, 2019

@posva Sounds good to me. Do you have any input on whether or not I should add the macOS platform check to restrict the fix to macOS?

Ideally, the program should behave the same on CEP osx vs CEP windows

Other than that the checks you wrote look good

@ericdrobinson ericdrobinson linked a pull request Aug 30, 2019 that will close this issue
13 tasks
@derrickb
Copy link

derrickb commented Sep 6, 2019

I can confirm that @ericdrobinson's PR fixes the issue for me on Photoshop CC 2019 macOS

@ericdrobinson
Copy link
Author

Awesome! Thanks for giving it a shot @derrickb! For the record, I've tested the fix on Premiere Pro CC 2019 on macOS and everything looks good!

ericdrobinson added a commit to ericdrobinson/vue that referenced this issue Sep 6, 2019
Some Adobe CEP environments have a broken Event.timeStamp implementation that breaks event blocking
logic. The issue specifically affects host applications running on macOS with CEP version 9.3 and
below. This workaround is OS-agnostic so as to maintain identical behavior across platforms. More
details can be found in issue vuejs#10366.

fix vuejs#10366
ericdrobinson added a commit to ericdrobinson/vue that referenced this issue Sep 10, 2019
Some Adobe CEP environments have a broken Event.timeStamp implementation that breaks event blocking
logic. The issue specifically affects host applications running on macOS with CEP version 9.3 and
below. This workaround is OS-agnostic so as to maintain identical behavior across platforms. More
details can be found in issue vuejs#10366.

fix vuejs#10366
@atwhiteley
Copy link

atwhiteley commented Sep 15, 2019

Hi all, huge thank you for finding and fixing this bug. I spent the last couple days going crazy over debugging this, and I'm so happy to see this here.

This bug is causing our panel to be practically unusable in production in mac environments, and I wanted to ask how I could get this bugfix into our system? I really rather not wait for this to be merged & published. Is there a way to patch a bugfix such as this?

Again, huge kudos to finding and fixing this bug.

EDIT: I decided to go with patch-package.

EDIT 2 : I really can't seem to make it work. I am having a lot of trouble building vue from source code. I hope this gets accepted as a fix ASAP.

@ericdrobinson
Copy link
Author

EDIT: I decided to go with patch-package.

Probably wouldn't work. This PR only contains fixes for the source code - no dist modifications were committed. You'd have to grab this PR and run the correct build command to output the version of Vue that you need.

I wanted to ask how I could get this bugfix into our system?

@atwhiteley Provided you only need the ESM build, you can use the branch I prepared on my own fork of the vue repository. Here's how you would install it with NPM:

npm uninstall --save vue
npm install --save ericdrobinson/vue#v2.6.10-with-cep-fix

If you need a different build, you can do the following:

  1. Fork my vue repository on GitHub.
  2. Create your own branch off of my fix-adobe-cep-mouse-events branch.
  3. Build the version of Vue that your setup needs and commit it to your branch.
  4. Run the following in your project:
    npm uninstall --save vue
    npm install --save atwhiteley/vue#[your-branch-name]
    

This approach works perfectly for us.

@atwhiteley
Copy link

atwhiteley commented Sep 15, 2019

@ericdrobinson Thanks a lot - I'm actually not a 100% sure which build we use but I'll check that now, though I think it's not the ESM one.

I did fork vue and already did create a separate branch - but I ran into trouble trying to build the source code.

Your response tells me I'm on the right path so I'll keep trying, thanks!

UPDATE: It was my own stupid mistake that I couldn't build it, resolved it, built the files - and now my project is in good shape again in adobe premiere!

@posva posva added the has PR label Sep 17, 2019
ericdrobinson added a commit to ericdrobinson/vue that referenced this issue Dec 19, 2019
Some Adobe CEP environments have a broken Event.timeStamp implementation that breaks event blocking
logic. The issue specifically affects host applications running on macOS with CEP version 9.3 and
below. This workaround is OS-agnostic so as to maintain identical behavior across platforms. More
details can be found in issue vuejs#10366.

fix vuejs#10366
@boardend
Copy link

boardend commented Jan 15, 2020

I've just observed the same behavior in CEF 79.1.28 on Windows 10. The problem can be reproduced with the following conditions:

  • Vue >2.5.17
    • Click Events work fine when downgrading to Vue <= 2.5.17
  • CEF started with --off-screen-rendering-enabled
    • Click Events work fine when CEF is started without off screen rendering
    • I assume that CEP is using the off screen rendering inside the Adobe products
  • Windows "Fast startup" is enabled and the computer was actually started after a real shutdown

There is an issue reported on the CEF repo: https://bitbucket.org/chromiumembedded/cef/issues/2749/osr-results-in-weird-eventtimestamp-values

Maybe someone could check if the problem can be reproduced on their Windows 10 machine. CEF builds can be downloaded via http://opensource.spotify.com/cefbuilds/index.html (I've used cef_binary_79.1.28+gf272726+chromium-79.0.3945.117_windows64_client.tar.bz2)

If this is the case, then I would adapt the PR #10459 from @ericdrobinson to detect CEP and CEF.

@joaomlemos
Copy link

Hey there!
This PR #11031 looks good to fix the issue. Similar to the PR mentioned #10459 but more agnostic of the platform involved

@ericdrobinson
Copy link
Author

ericdrobinson commented Feb 14, 2020

@joaomlemos #11031 does not mention CEP applications at all. Have you tested that PR in a CEP context? If so, which application and which version?

I'm also not sure if the new logic in #11031 isn't so broad that it would negate the purpose of the check to begin with... Someone on the Vue team would need to weigh in on that. @posva perhaps?

@joaomlemos
Copy link

@ericdrobinson, I didn’t tested it with CEP applications. The issue that I’m facing is in CEF applications. The same issue was mentioned by @boardend in the comment above #10366 (comment)

@ericdrobinson
Copy link
Author

ericdrobinson commented Feb 14, 2020

@joaomlemos Then, to be clear, you can only confirm that the proposed fix in #11031 handles the CEF case that @boardend mentioned, yes?

This is important because, if so, you cannot globally declare that #11031 fixes the issue for CEP as well, which your initial comment seemed to somewhat ambiguously imply.

Also, if this is the case, then it may be a good idea to comment on #11031 with a link to @boardend's comment above and mention on that PR that in your testing it seems to address the CEF problem.


I would still suggest that someone more familiar with the code in question take a good look at the proposed solution in #11031 as it seems that it might be a little too "lenient".

@joaomlemos
Copy link

Right, thank you for your suggestion @ericdrobinson !

@rx-837
Copy link

rx-837 commented Feb 17, 2020

@ericdrobinson

Solution coudn't be so "lenient" because of first check is excludes browsers with normal timestamp behaviour.

The solution only extends a first crutch(e.currentTarget === e.target)

@John0King
Copy link

I think the problem is actually happend with CEF (Chrome Embeded Framework)
I hit this issue https://stackoverflow.com/questions/60473957/cef-vuejs-performance-issue-on-listening-mouse-click-event-after-4-5-times-ren#

note , this question is not asked by me, that mean other people hit it too

@ericdrobinson
Copy link
Author

@John0King That may very well be a separate core issue with a similar result. While Adobe CEP does indeed build on CEF, the issue described here explicitly occurred only on macOS hosts. The CEF version in Adobe CEP is also relatively ancient, whereas the issue you posted describes far more recent versions of CEF.

This issue has an associated PR that focuses the solution to as narrow a scope as possible. I would recommend opening a new issue and describe exactly the kind of values you're seeing for the event timeStamp that are causing the problem you're experiencing.

@posva posva mentioned this issue Sep 2, 2020
ericdrobinson added a commit to ericdrobinson/vue that referenced this issue May 10, 2022
Some Adobe CEP environments have a broken Event.timeStamp implementation that breaks event blocking
logic. The issue specifically affects host applications running on macOS with CEP version 9.3 and
below. This workaround is OS-agnostic so as to maintain identical behavior across platforms. More
details can be found in issue vuejs#10366.

fix vuejs#10366
@mrspartak
Copy link

2022 and its still a thing...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants