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

fix(menu): allow change events to be direct #3689

Merged
merged 1 commit into from Oct 2, 2023
Merged

Conversation

Westbrook
Copy link
Collaborator

Description

Remove an asynchronous break in the call stack between click and change so that navigator.clipboard.copyText() can be safely called during the change event.

Test this.

  • permissions can be added to Chrome
  • flags can be set in Firefox
  • Safari does not seem to have testing support in the version of Playwright we're leveragng

Demonstrate it in a handful of stories.

Related issue(s)

How has this been tested?

  • Test case 1
    1. Go here
    2. Select a Menu Item
    3. See that it's value (or text in most cases) has been copied to the clipboard
  • Test case 2
    1. Go here
    2. Open the Menu
    3. Select a Menu Item
    4. See that it's value (or text in most cases) has been copied to the clipboard

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • If my change required a change to the documentation, I have updated the documentation in this pull request.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@github-actions
Copy link

github-actions bot commented Oct 2, 2023

Tachometer results

Chrome

action-menu permalink

Version Bytes Avg Time vs remote vs branch
npm latest 630 kB 250.22ms - 253.72ms - unsure 🔍
-0% - +1%
-0.95ms - +3.55ms
branch 625 kB 249.25ms - 252.09ms unsure 🔍
-1% - +0%
-3.55ms - +0.95ms
-

menu permalink

Version Bytes Avg Time vs remote vs branch
npm latest 452 kB 365.31ms - 369.76ms - unsure 🔍
-1% - +1%
-3.48ms - +4.04ms
branch 447 kB 364.22ms - 370.28ms unsure 🔍
-1% - +1%
-4.04ms - +3.48ms
-

picker permalink

Version Bytes Avg Time vs remote vs branch
npm latest 496 kB 858.67ms - 865.59ms - unsure 🔍
-1% - +0%
-6.67ms - +4.14ms
branch 491 kB 859.24ms - 867.54ms unsure 🔍
-0% - +1%
-4.14ms - +6.67ms
-

split-button permalink

Version Bytes Avg Time vs remote vs branch
npm latest 701 kB 1888.96ms - 1892.44ms - unsure 🔍
-0% - +0%
-4.16ms - +0.36ms
branch 695 kB 1891.16ms - 1894.04ms unsure 🔍
-0% - +0%
-0.36ms - +4.16ms
-
Firefox

action-menu permalink

Version Bytes Avg Time vs remote vs branch
npm latest 630 kB 484.56ms - 517.68ms - unsure 🔍
-5% - +5%
-24.64ms - +24.04ms
branch 625 kB 483.58ms - 519.26ms unsure 🔍
-5% - +5%
-24.04ms - +24.64ms
-

menu permalink

Version Bytes Avg Time vs remote vs branch
npm latest 452 kB 714.72ms - 758.52ms - unsure 🔍
-3% - +5%
-18.71ms - +35.31ms
branch 447 kB 712.50ms - 744.14ms unsure 🔍
-5% - +3%
-35.31ms - +18.71ms
-

picker permalink

Version Bytes Avg Time vs remote vs branch
npm latest 496 kB 1301.29ms - 1348.39ms - unsure 🔍
-2% - +2%
-27.06ms - +32.86ms
branch 491 kB 1303.42ms - 1340.46ms unsure 🔍
-2% - +2%
-32.86ms - +27.06ms
-

split-button permalink

Version Bytes Avg Time vs remote vs branch
npm latest 701 kB 1625.09ms - 1633.51ms - unsure 🔍
-0% - +1%
-1.32ms - +12.96ms
branch 695 kB 1617.71ms - 1629.25ms unsure 🔍
-1% - +0%
-12.96ms - +1.32ms
-

Copy link
Collaborator

@najikahalsema najikahalsema left a comment

Choose a reason for hiding this comment

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

Is this functionality something that is expected or standard to a menu like this, or is this something we'd like to optionally toggle? I can imagine as an end user if I had some unexpected text copied in my clipboard from a menu item I messed with, I would be annoyed.

@Westbrook
Copy link
Collaborator Author

Yes, randomly copying stuff into people's clipboard would be weird! However, all of the copy workflows are part of the demos herein, not the custom elements that are distributed themselves. 😅

@najikahalsema
Copy link
Collaborator

Oh, duh. My brain didn't register this was only in the stories.

@najikahalsema najikahalsema merged commit b2cd3da into main Oct 2, 2023
47 checks passed
@najikahalsema najikahalsema deleted the direct-change branch October 2, 2023 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Cannot write to clipboard in Safari from change event of sp-action-menu
2 participants