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

Bug: Fix Eyedropper Karma Test #8524

Closed
wants to merge 3 commits into from
Closed

Conversation

maxyinger
Copy link
Contributor

Context

Karma test for eye dropper is currently failing on main. This fixes the existing karma test up until the point of the eyedropper having a misalignment issue at it's core. once the eyedropper is fixed this karma test should work properly.

Summary

Relevant Technical Choices

To-do

User-facing changes

Testing Instructions

QA

  • This is a non-user-facing change and requires no QA

This PR can be tested by following these steps:

UAT

  • UAT should use the same steps as above.

This PR can be tested by following these steps:

Reviews

Does this PR have a security-related impact?

Does this PR change what data or activity we track or use?

Does this PR have a legal-related impact?

Checklist

  • This PR addresses an existing issue and I have linked this PR to it in ZenHub
  • I have tested this code to the best of my abilities
  • I have verified accessibility to the best of my abilities (docs)
  • I have verified i18n and l10n (translation, right-to-left layout) to the best of my abilities
  • This PR contains automated tests (unit, integration, and/or e2e) to verify the code works as intended (docs)
  • I have added documentation where necessary
  • I have added a matching Type: XYZ label to the PR

Fixes #

@maxyinger maxyinger requested a review from merapi July 30, 2021 20:29
@google-cla google-cla bot added the cla: yes label Jul 30, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Jul 30, 2021

Size Change: +5.23 kB (0%)

Total Size: 2.89 MB

Filename Size Change
assets/js/chunk-fonts-********************.js 46.3 kB +469 B (+1%)
assets/js/edit-story.js 1.1 MB +519 B (0%)
assets/js/stories-dashboard.js 1 MB +270 B (0%)
assets/js/web-stories-block.js 18.2 kB -179 B (-1%)
assets/js/imgareaselect.js 4.14 kB +4.14 kB (new file) 🆕
ℹ️ View Unchanged
Filename Size Change
assets/css/carousel-view-rtl.css 701 B 0 B
assets/css/carousel-view.css 701 B 0 B
assets/css/edit-story-rtl.css 1.21 kB 0 B
assets/css/edit-story.css 1.2 kB 0 B
assets/css/stories-dashboard-rtl.css 625 B 0 B
assets/css/stories-dashboard.css 626 B 0 B
assets/css/web-stories-block-rtl.css 4.43 kB 0 B
assets/css/web-stories-block.css 4.47 kB 0 B
assets/css/web-stories-embed-rtl.css 288 B 0 B
assets/css/web-stories-embed.css 288 B 0 B
assets/css/web-stories-list-styles-rtl.css 2.3 kB 0 B
assets/css/web-stories-list-styles.css 2.32 kB 0 B
assets/css/web-stories-theme-style-twentyeleven-rtl.css 102 B 0 B
assets/css/web-stories-theme-style-twentyeleven.css 102 B 0 B
assets/css/web-stories-theme-style-twentyfifteen-rtl.css 251 B 0 B
assets/css/web-stories-theme-style-twentyfifteen.css 251 B 0 B
assets/css/web-stories-theme-style-twentyfourteen-rtl.css 287 B 0 B
assets/css/web-stories-theme-style-twentyfourteen.css 287 B 0 B
assets/css/web-stories-theme-style-twentyseventeen-rtl.css 288 B 0 B
assets/css/web-stories-theme-style-twentyseventeen.css 288 B 0 B
assets/css/web-stories-theme-style-twentysixteen-rtl.css 224 B 0 B
assets/css/web-stories-theme-style-twentysixteen.css 224 B 0 B
assets/css/web-stories-theme-style-twentyten-rtl.css 143 B 0 B
assets/css/web-stories-theme-style-twentyten.css 143 B 0 B
assets/css/web-stories-theme-style-twentytwelve-rtl.css 256 B 0 B
assets/css/web-stories-theme-style-twentytwelve.css 256 B 0 B
assets/css/web-stories-theme-style-twentytwenty-rtl.css 86 B 0 B
assets/css/web-stories-theme-style-twentytwenty.css 86 B 0 B
assets/css/web-stories-theme-style-twentytwentyone-rtl.css 325 B 0 B
assets/css/web-stories-theme-style-twentytwentyone.css 326 B 0 B
assets/css/web-stories-widget-rtl.css 484 B 0 B
assets/css/web-stories-widget.css 484 B 0 B
assets/js/carousel-view.js 3.72 kB 0 B
assets/js/chunk-colorthief-********************.js 2.61 kB 0 B
assets/js/chunk-focus-visible-********************.js 999 B 0 B
assets/js/chunk-web-stories-template-0-********************.js 476 B +1 B (0%)
assets/js/chunk-web-stories-template-10-********************.js 8.77 kB 0 B
assets/js/chunk-web-stories-template-100-********************.js 8.15 kB 0 B
assets/js/chunk-web-stories-template-102-********************.js 505 B 0 B
assets/js/chunk-web-stories-template-106-********************.js 7.52 kB 0 B
assets/js/chunk-web-stories-template-108-********************.js 443 B 0 B
assets/js/chunk-web-stories-template-112-********************.js 9.19 kB 0 B
assets/js/chunk-web-stories-template-114-********************.js 480 B 0 B
assets/js/chunk-web-stories-template-118-********************.js 12 kB 0 B
assets/js/chunk-web-stories-template-12-********************.js 472 B -1 B (0%)
assets/js/chunk-web-stories-template-120-********************.js 462 B 0 B
assets/js/chunk-web-stories-template-124-********************.js 5.98 kB 0 B
assets/js/chunk-web-stories-template-126-********************.js 517 B 0 B
assets/js/chunk-web-stories-template-130-********************.js 7.14 kB 0 B
assets/js/chunk-web-stories-template-132-********************.js 489 B 0 B
assets/js/chunk-web-stories-template-136-********************.js 8.77 kB 0 B
assets/js/chunk-web-stories-template-138-********************.js 491 B +1 B (0%)
assets/js/chunk-web-stories-template-142-********************.js 7.46 kB 0 B
assets/js/chunk-web-stories-template-144-********************.js 482 B +1 B (0%)
assets/js/chunk-web-stories-template-148-********************.js 8 kB 0 B
assets/js/chunk-web-stories-template-150-********************.js 503 B 0 B
assets/js/chunk-web-stories-template-154-********************.js 9.88 kB 0 B
assets/js/chunk-web-stories-template-156-********************.js 490 B +1 B (0%)
assets/js/chunk-web-stories-template-16-********************.js 7.96 kB 0 B
assets/js/chunk-web-stories-template-160-********************.js 7.81 kB 0 B
assets/js/chunk-web-stories-template-162-********************.js 429 B 0 B
assets/js/chunk-web-stories-template-166-********************.js 8.15 kB 0 B
assets/js/chunk-web-stories-template-168-********************.js 471 B 0 B
assets/js/chunk-web-stories-template-172-********************.js 9.23 kB 0 B
assets/js/chunk-web-stories-template-174-********************.js 464 B -1 B (0%)
assets/js/chunk-web-stories-template-178-********************.js 13.8 kB 0 B
assets/js/chunk-web-stories-template-18-********************.js 492 B 0 B
assets/js/chunk-web-stories-template-180-********************.js 510 B +1 B (0%)
assets/js/chunk-web-stories-template-184-********************.js 8.32 kB 0 B
assets/js/chunk-web-stories-template-186-********************.js 445 B -1 B (0%)
assets/js/chunk-web-stories-template-190-********************.js 8.56 kB 0 B
assets/js/chunk-web-stories-template-22-********************.js 9.01 kB 0 B
assets/js/chunk-web-stories-template-24-********************.js 499 B 0 B
assets/js/chunk-web-stories-template-28-********************.js 7.97 kB 0 B
assets/js/chunk-web-stories-template-30-********************.js 515 B 0 B
assets/js/chunk-web-stories-template-34-********************.js 8.09 kB 0 B
assets/js/chunk-web-stories-template-36-********************.js 465 B 0 B
assets/js/chunk-web-stories-template-4-********************.js 10.4 kB 0 B
assets/js/chunk-web-stories-template-40-********************.js 6.64 kB 0 B
assets/js/chunk-web-stories-template-42-********************.js 506 B 0 B
assets/js/chunk-web-stories-template-46-********************.js 7.15 kB 0 B
assets/js/chunk-web-stories-template-48-********************.js 466 B 0 B
assets/js/chunk-web-stories-template-52-********************.js 7.75 kB 0 B
assets/js/chunk-web-stories-template-54-********************.js 478 B 0 B
assets/js/chunk-web-stories-template-58-********************.js 8.01 kB 0 B
assets/js/chunk-web-stories-template-6-********************.js 506 B 0 B
assets/js/chunk-web-stories-template-60-********************.js 425 B -1 B (0%)
assets/js/chunk-web-stories-template-64-********************.js 9.65 kB 0 B
assets/js/chunk-web-stories-template-66-********************.js 481 B 0 B
assets/js/chunk-web-stories-template-70-********************.js 8.4 kB 0 B
assets/js/chunk-web-stories-template-72-********************.js 458 B -1 B (0%)
assets/js/chunk-web-stories-template-76-********************.js 8.52 kB 0 B
assets/js/chunk-web-stories-template-78-********************.js 451 B 0 B
assets/js/chunk-web-stories-template-82-********************.js 6.96 kB 0 B
assets/js/chunk-web-stories-template-84-********************.js 476 B 0 B
assets/js/chunk-web-stories-template-88-********************.js 6.75 kB 0 B
assets/js/chunk-web-stories-template-90-********************.js 459 B 0 B
assets/js/chunk-web-stories-template-94-********************.js 8.16 kB 0 B
assets/js/chunk-web-stories-template-96-********************.js 490 B 0 B
assets/js/chunk-web-stories-textset-0-********************.js 5.29 kB 0 B
assets/js/chunk-web-stories-textset-1-********************.js 6.79 kB 0 B
assets/js/chunk-web-stories-textset-2-********************.js 7.91 kB 0 B
assets/js/chunk-web-stories-textset-3-********************.js 15.4 kB 0 B
assets/js/chunk-web-stories-textset-4-********************.js 4.4 kB 0 B
assets/js/chunk-web-stories-textset-5-********************.js 5.71 kB 0 B
assets/js/chunk-web-stories-textset-6-********************.js 5.52 kB 0 B
assets/js/chunk-web-stories-textset-7-********************.js 10.4 kB 0 B
assets/js/lightbox.js 991 B 0 B
assets/js/tinymce-button.js 3.49 kB 0 B
assets/js/vendors-chunk-ffmpeg-********************.js 5.6 kB 0 B
assets/js/vendors-chunk-html-to-image-********************.js 4.62 kB 0 B
assets/js/vendors-chunk-react-calendar-********************.js 11.7 kB 0 B
assets/js/vendors-chunk-react-color-********************.js 42.9 kB 0 B
assets/js/vendors-chunk-react-moveable-********************.js 53.4 kB 0 B
assets/js/vendors-chunk-resize-observer-polyfill-edit-story-********************.js 2.55 kB 0 B
assets/js/vendors-chunk-web-animations-js-********************.js 14.6 kB 0 B
assets/js/vendors-edit-story-stories-dashboard-********************.js 177 kB +8 B (0%)
assets/js/web-stories-activation-notice.js 23.7 kB 0 B
assets/js/web-stories-embed.js 493 B 0 B
assets/js/web-stories-widget.js 984 B 0 B

compressed-size-action

Comment on lines +55 to +57
// Only regestering image click by clicking twice for some reason
await fixture.events.click(image);
await fixture.events.click(image);
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this is the issue we've also recorded in #7481 and that we need to fix in a general manner.

const bgCanvasRect =
fixture.editor.canvas.framesLayer.frames[0].node.getBoundingClientRect();
// Make sure to click on part of bg element not covered by media element previously
// assed to canvas
Copy link
Contributor

Choose a reason for hiding this comment

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

😂 😂 😂 😂

Suggested change
// assed to canvas
// added to canvas

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🍑

const bgCanvasRect =
fixture.editor.canvas.framesLayer.frames[0].node.getBoundingClientRect();
// Make sure to click on part of bg element not covered by media element previously
// assed to canvas
Copy link
Contributor

Choose a reason for hiding this comment

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

😂 😂 😂 😂

Suggested change
// assed to canvas
// added to canvas

Comment on lines 64 to 66
await fixture.events.mouse.click(
bgCanvasRect.left + 10,
bgCanvasRect.bottom - 10
Copy link
Contributor

Choose a reason for hiding this comment

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

We actually have clickOn for that same purpose:

await fixture.events.mouse.clickOn(fixture.editor.canvas.framesLayer.frames[0].node, 10 ,10);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ooo very cool. thanks for sharing!

@swissspidy swissspidy mentioned this pull request Aug 2, 2021
9 tasks
Comment on lines +85 to +89
// await fixture.events.mouse.move(
// imageOnCanvasRect.right - 2,
// imageOnCanvasRect.top + 8
// );
// await fixture.events.sleep(10000);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@merapi @barklund if you uncomment this, and run npm run test:karma:edit-story:watch you can see the misalignment issue I mentioned.

Any thoughts on this? seems like an issue with the eyedropper and not the test, but maybe I'm misunderstanding something. Anyway, y'all have more context here. curious to see what you think

Copy link
Contributor

Choose a reason for hiding this comment

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

Found it - I can replicate the issue on one of my machines, will fix it today/tomorrow, right now I will work on a general fix for Moveable (double click/dnd issue).

@merapi
Copy link
Contributor

merapi commented Aug 4, 2021

The Moveable fix is already merged and the eyedropper fix will be in another PR, so I'm closing this.

@merapi merapi closed this Aug 4, 2021
@merapi merapi deleted the fix-eyedropper-karma-test branch August 4, 2021 14:21
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.

3 participants