Fixes #80. Guard against IE11 focusing SVG. #146
Conversation
|
@alice does this look ok to you? |
test/specs/svg.js
Outdated
| async function tabUntil(body, end) { | ||
| let el; | ||
| while (true) { | ||
| focused = await driver.executeScript(` |
There was a problem hiding this comment.
All of the tests that manually execute script through webdriver use a template literal. https://github.com/WICG/focus-visible/blob/master/test/specs/programmatic-focus-always.js#L32-L34
There was a problem hiding this comment.
Blargh, GitHub submitted my reply before I was finished typing.
I originally used template literals because the code was copied from another project and we had done it there. I can update all of the tests if you'd prefer that.
src/focus-visible.js
Outdated
| el.nodeName !== 'HTML' && | ||
| el.nodeName !== 'BODY' | ||
| el.nodeName !== 'BODY' && | ||
| el.classList && |
There was a problem hiding this comment.
can we do 'classList' in el && 'contains' in el.classList to avoid breaking my brain?
| break; | ||
| } | ||
| if (focused) { | ||
| // IE11's selenium driver won't move focus if we send it to body again |
There was a problem hiding this comment.
This whole block looks like the result of a lot of crying and furious Googling.
There was a problem hiding this comment.
It was mostly just swearing and trying stuff. Googling was no help.
| await body.click(); | ||
| await tabUntil(body, 'end'); | ||
| let actual = await driver.executeScript(` | ||
| return window.getComputedStyle(document.querySelector('#end')).outlineColor |
There was a problem hiding this comment.
Can you add a comment explaining how this tests for the failure mode you describe at the top of the file?
|
made some changes...let's see if sauce still passes 🤞 |
|
womp womp |
|
Thanks for this! We were getting a flood of Is there anything I can help with to get this merged? It looks like the tests timed out on Edge |
|
oh I totally forgot this had not landed yet! I think the only thing holding it back is my poorly written test. I may break this PR in half because I'm confident the fix works, but getting the test to run reliable in SauceLabs is weird. Let me try that, we can land the fix at least, and I'll do a separate PR for the test. |
|
oh sweet, it looks like the tests actually do pass. The previous failure was just Sauce Labs timing out. I misread the output :P Merging! |
|
Great, thanks so much! |
No description provided.