fix(html-lang-valid): only run rule when attribute has value

[thibaudcolas]

Depends on #3669. Closes issue #3624. This updates the rule’s selector as discussed so the rule is reported as inapplicable when encountering empty lang or xml:lang attributes.

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 to test/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.

[straker]

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 [html=""], our matches code for attributes checks if the regex generated from css-parser matches (which it does). But it also checks to see if the value property is falsy, which in this case it is (value: ""), so the matches thinks the attribute matches, which then is negated with the not pseudo class causing the matcher to fail even though the attribute has a value. We'll need to fix this before we can merge this pr

[thibaudcolas]

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.

[straker]

Have a pr that should fix the issue.

[thibaudcolas]

Rebased onto the latest develop, with the fix so this now seems to be working as expected. I believe this is now ready for review, though note I’ve only tested the change with the added unit test so far.

[straker]

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)

[straker]

@thibaudcolas anything I can do to help with the pr?

[straker]

@thibaudcolas I'll go ahead and add the tests so this can be reviewed and merged.

Resolved

[WilcoFiers]

[x] Reviewed for security

[thibaudcolas]

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.