fix(html-lang-valid): only run rule when attribute has value#3663
Conversation
|
It looks like you've encountered a bug in our code. Digging into this it looks like when we try to match the not expression of |
|
Thank you for looking into this @straker. I’m feeling out of my depth here so I’ll leave this as-is until someone else can devise a fix. |
|
Have a pr that should fix the issue. |
24c0701 to
8b4829b
Compare
8b4829b to
eb04e60
Compare
eb04e60 to
95b2224
Compare
|
Rebased onto the latest |
There was a problem hiding this comment.
Looking good! We should also update the html-valid-lang integration tests to check for this. If you create two more iframes inside the frames dir, one using lang and the other for xml:lang and make the attributes empty, and then add the frames to the html-valid-lang.html file, that should be enough (since it shouldn't modify the passes/violation count of the test)
|
@thibaudcolas anything I can do to help with the pr? |
|
@thibaudcolas I'll go ahead and add the tests so this can be reviewed and merged. |
…on-inapplicable-html-lang-valid
|
[x] Reviewed for security |
|
Thank you for picking this up! I had a busy couple months and wouldn’t have been able to get to this until now, it’s great to see it having gone through. |
Depends on #3669. Closes issue #3624. This updates the rule’s selector as discussed so the rule is reported as inapplicable when encountering empty
langorxml:langattributes.It would be nice if this detail of the implementation was documented somewhere but I’m not sure where would be a good place.
For tests, I added another "applicability" test case similar to
html-lang-valid virtual-rule – is inapplicable without lang or xml:lang. I also considered adding a test case totest/integration/full/html-lang-valid/html-lang-valid.js, however we’re not checking applicability in there currently, and it seems the only thing we could do is add another frame and check it isn’t in the result – so I decided to leave this out. Happy to revisit if needed.This is failing currently in tests, with existing passes and violations not getting picked up by the new selector. I’m not entirely sure why – could be because of how Axe converts selectors to its own expression format but that’s just a guess.