Skip to content

Conversation

mashalifshin
Copy link
Contributor

@mashalifshin mashalifshin commented Aug 22, 2025

Problem Statement

We need unique alt text on the ad images for our accessible-first Playwright tests to pass in the example apps, as that's the accessible way to select our ad elements.

Proposed Changes

We now construct alt text for the ad images on the client side in order to contextualize what kind of ad it is for users who aren't navigating visually. And we get the unique ids for the Playwright tests exactly as we need them.

Verification Steps

  • Load and inspect the examples and make sure the alt text looks good for all the ads.
  • Take a look at the new unit test in display.test.ts in core and make sure it's covering all the cases

export const DEFAULT_SERVICE_ENDPOINT: HTTPSURLString = IS_PRODUCTION
? "https://ads.mozilla.org/" // production
: "https://ads.allizom.org/" // staging
: "https://ads.mozilla.org/" // staging
Copy link
Contributor

Choose a reason for hiding this comment

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

ehhh.. can we just update playwright to set NODE_ENV=production instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, apologies for the mess, that would absolutely be the right way to do it, I will make that change. This was just me trying out a quick way to make sure this approach would work


test("Use staging endpoint URLs when not in production", () => {
expect(DEFAULT_SERVICE_ENDPOINT).toBe("https://ads.allizom.org/")
expect(DEFAULT_SERVICE_ENDPOINT).toBe("https://ads.mozilla.org/")
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't testing the right thing anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, thank you so much. Will fix.

@mashalifshin mashalifshin changed the title Fix playwright end to end tests Fix Playwright tests by adding more descriptive alt text to ad images Aug 26, 2025
@mashalifshin mashalifshin marked this pull request as ready for review August 26, 2025 01:42
@mashalifshin mashalifshin requested a review from a team as a code owner August 26, 2025 01:42
"example:iife": "npm run build && cd examples/iife && npm install && npm run dev",
"example:react": "npm run build && cd examples/react && npm install && npm run dev",
"example:iife": "npm run build:production && cd examples/iife && npm install && npm run dev",
"example:react": "npm run build:production && cd examples/react && npm install && npm run dev",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add example:iife:staging and example:react:staging scripts here to be able to also explicitly run the examples against staging (not as the default)

@justindarc
Copy link
Contributor

Before we have to resort to hard-coding these generic alt texts, can you check and see if we can just locate the <img> elements in the Playwright tests using their existing [data-placement-id] attributes? For example:

const billboard = page.locator("img[data-placement-id='pocket_billboard_1']")

If this works for correctly identifying rendered ads in Playwright, we can avoid hard-coding the alt text and having to do a potentially-fragile mapping to the placement ID. Really, the alt text should come exclusively from MARS and our single generic "Mozilla Ad" text is really only intended as an absolute last resort.

@mashalifshin mashalifshin marked this pull request as draft October 21, 2025 23:36
@mashalifshin
Copy link
Contributor Author

Before we have to resort to hard-coding these generic alt texts, can you check and see if we can just locate the <img> elements in the Playwright tests using their existing [data-placement-id] attributes? For example:

const billboard = page.locator("img[data-placement-id='pocket_billboard_1']")

If this works for correctly identifying rendered ads in Playwright, we can avoid hard-coding the alt text and having to do a potentially-fragile mapping to the placement ID. Really, the alt text should come exclusively from MARS and our single generic "Mozilla Ad" text is really only intended as an absolute last resort.

I haven't had a chance to try this but I think that would probably work, and there are lots of ways we can successfully locate and select these elements for test. But I think if we go this route, we are moving away from Playwright's best practice of using user-facing attributes. Writing our tests that way will actually give us more guarantees from Playwright under the hood that a real user can access and navigate the element on the page, in a way that selecting by a data attribute or test id would not.

Playwright has a generator that will show you what the correct accessible locator is for an element. Using that, I see that the accessible locator for the ad should be getByRole('link', { name: 'Mozilla Ad' }).first(), getByRole('link', { name: 'Mozilla Ad' }).nth(1), etc etc etc. So I think we can get by with selecting this way and just baking in the order of the ads on the page, still getting our accessible-first selectors and not needing to rely on MARS to make any changes now to fix our tests.

But I think this exposes a bit of an accessibility issue. I think it would be better if the ads each had their own placement prefix, so the person using screen reader can just jump to the ad they mean without needing to loop through them all or thinking about their order on the page. But I'm not sure on this -- you also made a good point in our conversation about it a while back, like maybe the screen reader user doesn't care that it's a Billboard and that's not meaningful in the alt text? I'll make a post in #accessibility about this so they can make a general recommendation about the alt text, and what would be helpful for users on a pocket-like example page.

There is also a ticket open for ads-eng/MARS which relates to "permanent canonical test placements" and specifies what we'd need for those to be "real enough" to consistently/repeatably drive a full suite of playwright e2e tests of the example apps.

So, to summarize:

  1. To fix the test suite right now, but do it in a way that leverages Playwright's accessible locators, use getByRole('link', { name: 'Mozilla Ad' }).first()... nth(3), and rely on the order on the page to differentiate. I'll try this and report back!
  2. When we're getting ready to be live on a surface, we'll need canonical test placements that are "real enough" to drive the example apps through a full click/view test. Maybe we even just use the prod placements, not sure. That work is captured in AE-1032. If MARS can make the alt text at least have a prefix that's "unique per-surface with a predictable format" then we can upgrade our locators to directly select each specific ad.
  3. Provide good, real, full alt text for ads coming from MARS. No ticket for this yet but I'll create one after I consult with the folks in #accessibility for requirements.

@mashalifshin
Copy link
Contributor Author

Been playing around with this for about a day now and as far as I can tell the MARS mock placements are back to behaving the same as they were before these started failing regularly (returning ads consistently, standard alt text, images being served). So I am going to close this PR and just re-enable the tests again. 🤞

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.

2 participants