Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Link text error fix when textContent is null #48

Closed
wants to merge 1 commit into from

Conversation

jasonruesch
Copy link
Contributor

Handled when the textContent is passed into the isDescriptiveText function as undefined or null in the link-text plugin. This fixes the error "Unable to get property 'replace' of undefined or null reference"
screen shot 2015-07-22 at 9 28 46 am

…ction as undefined or null in the link-text plugin. This fixes the error "Unable to get property 'replace' of undefined or null reference"
@jdan
Copy link
Owner

jdan commented Jul 22, 2015

This LGTM, thank you very much!

Out of curiosity, do you have an example of a page that throws this error? I have never seen it before and it might be worth sending a fix upstream as well.

@jasonruesch
Copy link
Contributor Author

Let me prepare one for you and I’ll send it your way shortly.

— Jason

On Jul 22, 2015, at 9:33 AM, Jordan Scales notifications@github.com wrote:

This LGTM, thank you very much!

Out of curiosity, do you have an example of a page that throws this error? I have never seen it before and it might be worth sending a fix upstream as well.


Reply to this email directly or view it on GitHub #48 (comment).

@jasonruesch
Copy link
Contributor Author

The error was caused by the following anchor tag:

As you can see, it has no text! Obviously, I’ll be fixing that in our system, but maybe the utility should’ve caught it rather than erring.

Thanks,

Jason

On Jul 22, 2015, at 9:33 AM, Jordan Scales notifications@github.com wrote:

This LGTM, thank you very much!

Out of curiosity, do you have an example of a page that throws this error? I have never seen it before and it might be worth sending a fix upstream as well.


Reply to this email directly or view it on GitHub #48 (comment).

@jasonruesch
Copy link
Contributor Author

Maybe the fix should be earlier in the call stack. With the fix I submitted, it gives the following help text for the error:

"The text "null" is unclear without context and may be confusing to screen readers. Consider rearranging the tags or including special screen reader text.” The fix I’m thinking is that empty string is passed into the isDescriptiveText function instead of null from the validateTextContent function. Just a thought.

Thanks,

Jason

On Jul 22, 2015, at 9:33 AM, Jordan Scales notifications@github.com wrote:

This LGTM, thank you very much!

Out of curiosity, do you have an example of a page that throws this error? I have never seen it before and it might be worth sending a fix upstream as well.


Reply to this email directly or view it on GitHub #48 (comment).

@jdan
Copy link
Owner

jdan commented Jul 22, 2015

Yep, I see it, thanks!

Per the instructions in CONTRIBUTING.md, we require contributors to sign our Contributor License Agreement before we can merge their code in. If you have any questions be sure to let me know!

The fix is fine where it is - I'll likely merge this onto a separate branch and add a custom error message.

@jdan
Copy link
Owner

jdan commented Jul 23, 2015

Landed in de2534f.

Turns out LinkTextPlugin was getting a little stale, so I made some improvements in 3497428 and added some unit tests in 1c86829.

Thanks @jasonruesch! 🌟

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

Successfully merging this pull request may close these issues.

2 participants