#8206: playwright tests for MS Edge sidebar links #8216

twschiller · 2024-04-10T19:53:20Z

What does this PR do?

Closes MS Edge: clicking a link in a sidebar mod crashes MS Edge #8206
Adds playwright tests for verifying sidebar links don't crash the browser
Adds a title the "Open Extension Console" button
I wrote a mod @pixies/test/sidebar-links for testing

Remaining Work

@fregante on main and 1.8.13-alpha.1 build the gear icon isn't doing anything for me in MS Edge. It doesn't change style on hover and isn't responding to clicks when I test manually (but seems like it might be working on CI?, but closing the sidepanel): seems to be fine for me on local builds on MS Edge. Seems like my extension got into a bad state
@fregante On MV3 it looks like clicking a link causes the sidebar to be closed on the current tab. So the tests I originally wrote aren't passing. Was that the intended behavior in your PR? 3e1a159#r140843795
- Hmm, testing manually on Chrome MV3, it doesn't look like the sidebar closes
- Looking at the MS Edge behavior, it looks like visiting a new tab that doesn't have the sidebar open closes it on the other tab. Can replicate that by opening the sidebar, opening a new tab, and then clicking back to the original tab. It looks like other people have noticed this too: Sidebar API for extensions is now live! microsoft/MicrosoftEdge-Extensions#95 (comment)
- I also opened a w3c ticket for the vendors to clarify expected behavior: Tab-specific sidePanel inconsistencies when opening new tabs/switching tabs w3c/webextensions#588
Fix MV3 tests on Chrome/Edge

Discussion

Given that MS Edge has a different sidepanel behavior, we'll need to adjust the test to ensure the sidepanel is open for each step. @fungairino @mnholtz - do we have a best practice for writing browser-specific tests or checks?
event.composedPath has different behavior for open/closed Shadow DOM. Is this a problem? I think it would be a source of seeing different results on CI vs. manual testing: cc @grahamlangford @fregante

Potential Improvements

Assert that a new page is opened when clicking the links

Future Work

@fregante MS Edge crashes if the user clicks a link in an iframe. This might be hard for us to address because we'd need to use a content script to rewrite the clicks. Do we have a response from the MS Edge team yet on the crashing? The fix for iframes might have to be a follow-up
- I opened a ticket here: MS Edge crashes when user clicks a target="_blank" link in the sidepanel microsoft/MicrosoftEdge-Extensions#145

Checklist

Add jest or playwright tests and/or storybook stories
Designate a primary reviewer: @fungairino

fungairino · 2024-04-10T20:01:33Z

end-to-end-tests/tests/sidebar/sidebarLinks.spec.ts

+
+  await sideBarPage.getByRole("link", { name: "Markdown Text Link" }).click();
+


You can also assert that the expected page was opened. See: https://playwright.dev/docs/pages#handling-popups

end-to-end-tests/tests/sidebar/sidebarLinks.spec.ts

twschiller · 2024-04-10T22:15:30Z

end-to-end-tests/tests/sidebar/sidebarLinks.spec.ts

+  // await expect(mainFrame.getByText("Alpha")).toBeVisible();
+  //
+  // const srcdocFrame = mainFrame.frameLocator("iframe").first();
+  // await srcdocFrame.getByRole("link", { name: "IFrame Link" }).click();


During manual testing in MS Edge on Mac the browser crashes when clicking link in the iframe. (Because our interception logic does not intercept it)

codecov · 2024-05-09T12:52:56Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.42%. Comparing base (6d95047) to head (a1b896a).
Report is 38 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8216      +/-   ##
==========================================
- Coverage   73.47%   73.42%   -0.05%     
==========================================
  Files        1334     1352      +18     
  Lines       41259    41523     +264     
  Branches     7686     7778      +92     
==========================================
+ Hits        30316    30490     +174     
- Misses      10943    11033      +90

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-05-09T13:01:31Z

Playwright test results - MV2

1 failed
41 passed
2 flaky
8 skipped

Details

Open report ↗︎
52 tests across 19 suites
11 minutes, 54 seconds
a1b896a

Failed tests

edge › tests/regressions/sidebarLinks.spec.ts › #8206: clicking links doesn't crash browser

Flaky tests

chrome › tests/extensionConsoleActivation.spec.ts › can activate a mod with a database
edge › tests/extensionConsoleActivation.spec.ts › can activate a mod with a database

Skipped tests

chrome › tests/extensionConsoleActivation.spec.ts › can activate a mod with built-in integration
edge › tests/extensionConsoleActivation.spec.ts › can activate a mod with built-in integration
chrome › tests/runtime/sidebarController.spec.ts › sidebar controller › shows focus dialog in top-level frame
edge › tests/runtime/sidebarController.spec.ts › sidebar controller › shows focus dialog in top-level frame
chrome › tests/runtime/sidebarNavigation.spec.ts › sidebar mod panels are persistent during navigation
chrome › tests/runtime/sidebarNavigation.spec.ts › sidebar form and temporary panels are unavailable after navigation
edge › tests/runtime/sidebarNavigation.spec.ts › sidebar mod panels are persistent during navigation
edge › tests/runtime/sidebarNavigation.spec.ts › sidebar form and temporary panels are unavailable after navigation

github-actions · 2024-05-09T13:06:33Z

Playwright test results - MV3

1 failed
49 passed
2 flaky

Details

Open report ↗︎
52 tests across 19 suites
17 minutes, 55 seconds
a1b896a

Failed tests

chrome › tests/regressions/sidebarLinks.spec.ts › #8206: clicking links doesn't crash browser

Flaky tests

chrome › tests/regressions/doNotCloseSidebarOnPageEditorSave.spec.ts › #8104: Do not automatically close the sidebar when saving in the Page Editor
edge › tests/regressions/doNotCloseSidebarOnPageEditorSave.spec.ts › #8104: Do not automatically close the sidebar when saving in the Page Editor

mnholtz · 2024-05-09T15:17:38Z

do we have a best practice for writing browser-specific tests or checks?

@twschiller You can use test.skip with a conditional, either at the top of the spec file or inside the individual test you want to denote. See an example here:
https://github.com/pixiebrix/pixiebrix-extension/pull/8276/files#diff-c39e8844cde0c8522636aedf8b8076926691d1f804b13ff1035445b45c065fe5R8-R11

In your case you'll want to compare the browser type, e.g. in Playwright's example:

import { test, expect } from '@playwright/test';

test('Safari-only test', async ({ page, browserName }) => {
  test.skip(browserName !== 'webkit', 'This feature is Safari-only');
  // ...
});

grahamlangford · 2024-05-10T14:07:34Z

src/sidebar/Header.tsx

+        aria-label="Open Extension Console"
+        title="Open Extension Console"


Why title and aria-label?

I've always been a bit confused as to the accessibility best practices for icon buttons. In a couple of places I've seen recommendations to not use title, or use both title and aria-label. It seems however that we should be able to just use title and getByRole("link", {name: "text"})

https://github.com/playwright-community/eslint-plugin-playwright/blob/main/docs/rules/no-get-by-title.md
https://dequeuniversity.com/rules/axe/4.1/button-name

#8206: playwright tests for links

e52a302

vercel bot deployed to Preview April 10, 2024 19:54 View deployment

Open the extension console

0fd07f4

twschiller requested a review from fungairino April 10, 2024 20:01

twschiller assigned fungairino and mnholtz Apr 10, 2024

twschiller requested a review from mnholtz April 10, 2024 20:01

twschiller assigned twschiller and unassigned mnholtz Apr 10, 2024

fungairino reviewed Apr 10, 2024

View reviewed changes

twschiller added the testing label Apr 10, 2024

vercel bot deployed to Preview April 10, 2024 20:01 View deployment

fungairino reviewed Apr 10, 2024

View reviewed changes

end-to-end-tests/tests/sidebar/sidebarLinks.spec.ts Outdated Show resolved Hide resolved

Fix jest snapshots

5be8e0e

vercel bot deployed to Preview April 10, 2024 20:31 View deployment

twschiller added this to the 1.8.13 milestone Apr 10, 2024

twschiller removed their assignment Apr 10, 2024

twschiller changed the title ~~#8206: add playwright tests for sidebar links~~ #8206: [WIP] add playwright tests for sidebar links Apr 10, 2024

twschiller marked this pull request as draft April 10, 2024 20:52

twschiller unassigned fungairino Apr 10, 2024

twschiller changed the title ~~#8206: [WIP] add playwright tests for sidebar links~~ #8206: fix link behavior for iframe and add playwright tests for sidebar links Apr 10, 2024

twschiller commented Apr 10, 2024

View reviewed changes

twschiller changed the title ~~#8206: fix link behavior for iframe and add playwright tests for sidebar links~~ #8206: fix links in Sidebar for MSEdge iframe and add playwright tests for sidebar links Apr 10, 2024

twschiller mentioned this pull request Apr 10, 2024

MS Edge: clicking a link in a sidebar mod crashes MS Edge #8206

Open

Merge branch 'main' into feature/8206-playwright

ea293d0

twschiller changed the title ~~#8206: fix links in Sidebar for MSEdge iframe and add playwright tests for sidebar links~~ #8206: fix links in Sidebar for MSEdge and add playwright tests Apr 10, 2024

twschiller changed the title ~~#8206: fix links in Sidebar for MSEdge and add playwright tests~~ #8206: [WIP fix links in Sidebar for MSEdge and add playwright tests Apr 10, 2024

twschiller changed the title ~~#8206: [WIP fix links in Sidebar for MSEdge and add playwright tests~~ #8206: [WIP[ fix links in Sidebar for MSEdge and add playwright tests Apr 10, 2024

twschiller changed the title ~~#8206: [WIP[ fix links in Sidebar for MSEdge and add playwright tests~~ #8206: [WIP] fix links in Sidebar for MSEdge and add playwright tests Apr 10, 2024

vercel bot deployed to Preview April 10, 2024 22:26 View deployment

twschiller assigned grahamlangford Apr 18, 2024

twschiller removed this from the 1.8.13 milestone Apr 18, 2024

twschiller added this to the 1.8.14 milestone Apr 30, 2024

Merge branch 'main' into feature/8206-playwright

dd12bd2

vercel bot deployed to Preview May 9, 2024 12:47 View deployment

grahamlangford assigned fungairino and unassigned grahamlangford May 9, 2024

fungairino added 2 commits May 9, 2024 17:08

Merge branch 'main' into feature/8206-playwright

074fb91

Work around msedge bugs

591279f

fungairino marked this pull request as ready for review May 9, 2024 21:54

Merge branch 'main' into feature/8206-playwright

5e48708

vercel bot deployed to Preview May 9, 2024 21:55 View deployment

fix snapshots

011345d

vercel bot deployed to Preview May 10, 2024 13:38 View deployment

grahamlangford reviewed May 10, 2024

View reviewed changes

Merge branch 'main' into feature/8206-playwright

4085536

vercel bot deployed to Preview May 10, 2024 14:42 View deployment

wip

f9bd5c6

vercel bot deployed to Preview May 10, 2024 15:30 View deployment

fungairino changed the title ~~#8206: [WIP] playwright tests for MS Edge sidebar links~~ #8206: playwright tests for MS Edge sidebar links May 10, 2024

fix snapshots

d74be30

vercel bot deployed to Preview May 10, 2024 15:46 View deployment

always reopen sidebar

a1b896a

vercel bot deployed to Preview May 10, 2024 16:34 View deployment

grahamlangford removed this from the 1.8.14 milestone May 10, 2024

twschiller added this to the 2.0.0 milestone May 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#8206: playwright tests for MS Edge sidebar links #8216

#8206: playwright tests for MS Edge sidebar links #8216

twschiller commented Apr 10, 2024 •

edited

fungairino Apr 10, 2024

twschiller Apr 10, 2024 •

edited

codecov bot commented May 9, 2024 •

edited

github-actions bot commented May 9, 2024 •

edited

github-actions bot commented May 9, 2024 •

edited

mnholtz commented May 9, 2024 •

edited

grahamlangford May 10, 2024

fungairino May 10, 2024 •

edited


		await sideBarPage.getByRole("link", { name: "Markdown Text Link" }).click();

		aria-label="Open Extension Console"
		title="Open Extension Console"

#8206: playwright tests for MS Edge sidebar links #8216

Are you sure you want to change the base?

#8206: playwright tests for MS Edge sidebar links #8216

Conversation

twschiller commented Apr 10, 2024 • edited

What does this PR do?

Remaining Work

Discussion

Potential Improvements

Future Work

Checklist

fungairino Apr 10, 2024

Choose a reason for hiding this comment

twschiller Apr 10, 2024 • edited

Choose a reason for hiding this comment

codecov bot commented May 9, 2024 • edited

Codecov Report

github-actions bot commented May 9, 2024 • edited

Playwright test results - MV2

Details

github-actions bot commented May 9, 2024 • edited

Playwright test results - MV3

Details

mnholtz commented May 9, 2024 • edited

grahamlangford May 10, 2024

Choose a reason for hiding this comment

fungairino May 10, 2024 • edited

Choose a reason for hiding this comment

twschiller commented Apr 10, 2024 •

edited

twschiller Apr 10, 2024 •

edited

codecov bot commented May 9, 2024 •

edited

github-actions bot commented May 9, 2024 •

edited

github-actions bot commented May 9, 2024 •

edited

mnholtz commented May 9, 2024 •

edited

fungairino May 10, 2024 •

edited