Skip to content
This repository has been archived by the owner on Dec 7, 2022. It is now read-only.

Fixes issue #183 #188

Merged
merged 3 commits into from
Jul 25, 2015
Merged

Fixes issue #183 #188

merged 3 commits into from
Jul 25, 2015

Conversation

ricksbrown
Copy link
Collaborator

Actually this is a workaround, not a fix because the bug is in closure compiler. I'll break it down in detail below.

First understand that the function axs.properties.findTextAlternatives can legitimately return null.
Then take a look at the way we call it:

var textAlternatives = axs.properties.findTextAlternatives(element, {});
if (!textAlternatives || textAlternatives.trim() === '')

All good. But then take a look at what closure compiler produces:

return "" === axs.properties.findTextAlternatives(a, {}).trim() ? !1 : !0;

That will throw an exception if the return value is null.
The easiest workaround is to get closure compiler to produce something else. The proposed change does not change the logic of the audit but results in safe code generated by closure compiler:

a = axs.properties.findTextAlternatives(a, {});
  return null == a || "" === a.trim() ? !1 : !0;

@ricksbrown
Copy link
Collaborator Author

@alice PTAL

@@ -43,7 +43,7 @@ axs.AuditRules.addRule({
// Ignore elements which have a negative tabindex and no text content,
// as they will be skipped by assistive technology
var textAlternatives = axs.properties.findTextAlternatives(element, {});
if (!textAlternatives || textAlternatives.trim() === '')
if (textAlternatives == null || textAlternatives.trim() === '')
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use === null - closure compile should catch if findTextAlternatives weirdly returns undefined anywhere.

@alice
Copy link
Contributor

alice commented Jul 24, 2015

Please update the Changelog: https://github.com/GoogleChrome/accessibility-developer-tools/blob/master/Changelog.md - manual process to help me write release notes. Sorry for the overhead :(

Once that's done and the nit addressed, this is 👍 - definitely an improvement regardless of the closure weirdness.

ricksbrown added a commit that referenced this pull request Jul 25, 2015
Fixes issue #183
Updated changelog and addressed nit.
@ricksbrown ricksbrown merged commit f909250 into GoogleChrome:master Jul 25, 2015
@ricksbrown
Copy link
Collaborator Author

No worries about the changelog overhead, it is well worth the effort IMHO.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants