Open mtlynch opened 11 months ago
Hello @mtlynch , was checking the issue. This link is not working for me. https://github.com/tiny-pilot/tinypilot-pro/blob/d98885ba9b27b574024a394928eaebe737a9cac5/e2e/security.spec.js#L11-L18
@pipliya - It's in our Pro repo, which is not public, but here's what the link points to:
// This is a workaround for our unusual HTML pattern of styling checkboxes as
// toggles. We should use a locator based on the checkbox label once we fix
// https://github.com/tiny-pilot/tinypilot/issues/1638.
const locateToggle = (cssSelector) => {
return page.locator(".toggle").filter({ has: page.locator(cssSelector) });
};
await locateToggle("#require-authentication").click();
I'd like to arrange the HTML so that this Playwright code sets the <input id="include-sensitive-data">
checkbox in the HTML above.
await page.getByLabel("Hide Sensitive Data").setChecked(true);
Got it. Thanks!
tldr, I’m not sure we can find a way to maintain our current toggle button implementation and use Playwright’s getByLabel
locator.
Our current toggle button implementation is based on a <label>
and an <input type=checkbox>
element. We didn’t choose that element combination for semantic reasons primarily, but more so because it allows a rather simple implementation of a toggle button without needing any custom JS code. The <input>
element’s purpose is mainly for managing the internal state of the checkbox.
Playwright’s getByLabel
locator doesn’t seem to be compatible with that approach, because it wants to scroll the corresponding input element into the viewport before interacting with it.
I’ve tried turning the currently “naked” Require username and password
text into a <label for="require-authentication">Require username and password</label>
element, which correctly associates the label with the checkbox element. I’ve then tried the following Playwright queries – however, all unsuccessful, unfortunately:
await page.getByLabel("Require username and password").setChecked(true)
await page.getByLabel("Require username and password").setChecked(true, { force: true })
await page.getByLabel("Require username and password").click()
await page.getByLabel("Require username and password").click({ force: true })
await page.getByLabel("Require username and password").check()
await page.getByLabel("Require username and password").check({ force: true })
Playwright seems to resolve the checkbox element correctly. However, without the {force: true}
option, it fails because the “Target [is] closed”, and with the {force: true}
option, it fails because the “Element is outside of the viewport”.
Error: locator.setChecked: Target closed =========================== logs =========================== waiting for getByLabel('Require username and password') locator resolved to <input type="checkbox" id="require-authentication"/> attempting click action waiting for element to be visible, enabled and stable element is not visible - waiting... ============================================================
Error: locator.setChecked: Element is outside of the viewport =========================== logs =========================== waiting for getByLabel('Require username and password') locator resolved to <input type="checkbox" id="require-authentication"/> attempting click action waiting for element to be visible, enabled and stable forcing action element is visible, enabled and stable scrolling into view if needed done scrolling ============================================================
Since this ticket was estimated as “small”, and I wasn’t able to find a promising lead so far, I wanted to discuss our options (at least ones I could think of). I guess it’s mostly a cost<>benefit question in the end.
If we wanted to make our toggle button more semantic, we’d probably have to turn it into a web component. That way, we don’t have to use the <input type=checkbox>
hack for maintaining internal state, but the state could live in JS code then. That would also allow us to set an aria-checked
attribute on the component, for making it accessible to screen readers.
It seems possible to associate <label>
elements with custom web components, but I’m not sure whether that would also work with Playwright’s getByLabel
in a “native” way.
Maybe worth a try? Converting the current toggle implementation into a web component might require refactoring work down the line, though, as we probably have to adjust client-side JS code, e.g. for wiring event handlers.
getByLabel
locator more deeplyI haven’t understood yet why the getByLabel
would fail on interactions if an element is outside of the viewport, whereas our current CSS-based locator seems to succeed given the same circumstances.
The getByLabel
locator doesn’t provide a {force: true}
option, but maybe there is another way to make it behave similar to our current query approach?
I haven’t used Playwright much, so I’d have to dig deeper here to understand this better.
Maybe we find a way that keeps the current toggle button implementation as is, but that achieves compatibility with Playwright’s expectations. For instance, one thing that I have already tried is to change the checkbox’s CSS in a way that doesn’t hide it directly via opacity: 0
, like we do now, but that just moves it away, via position: absolute; left: -99999px
. That attempt didn’t work out, but maybe we’d be able to find another such technique that does the trick. I don’t have anything in mind specifically right now, but we might find an approach by poking around.
@jotaen4tinypilot - Thanks for checking into this!
I thought this would be more straightforward, so given the complexity, it's probably not worth diving into right now. Our current Playwright workaround isn't too ugly.
If/when we pick this back up, I think the most promising path is turning this into a web component.
Yeah, I feel the same. Shall we remove it from 2.6.2 for now?
Ah, right. I meant to do that yesterday. Removed!
This blog post gave an example of a JS-free toggle switch that's pretty close to what we're already doing:
https://www.htmhell.dev/adventcalendar/2023/2/#custom-switches
Let's give this an hour and see if we can adopt that solution. If not, we'll punt on this.
We use a bit of an unusual style our toggle-style HTML checkboxes:
Ideally, it would look a little closer to something like:
In Playwright, we can't select the checkbox through normal methods because the checkbox itself is technically not visible on the screen, so we have to select the
<label>
element instead. This likely affects users with screen readers as well.https://github.com/tiny-pilot/tinypilot-pro/blob/d98885ba9b27b574024a394928eaebe737a9cac5/e2e/security.spec.js#L11-L18