Fix for Issue #6109 - aria-current has incorrect value "true"

[brandontruggles]

For Issue #6109. I updated the default value of aria-current on NavLink to "page". I also added "false" as a valid option, as defined in the spec for aria-current.

Also included are updated tests for NavLink, to reflect the above changes. The tests pass when I run npm run test in the root directory.

Please let me know if any further changes need to be made for this PR. If any documentation for NavLink needs to be changed, please send me a link to the locations that need to be modified, and I will update them accordingly. Thank you!

[pshrmn]

Looking at this now, I think that false shouldn't actually be included because this prop defines what is set when the <NavLink> is active, and aria-current=false indicates when it is not.

I am wondering if the default value when the <NavLink> isn't active should be changed. Currently it is set to null, but from the w3 spec I think maybe we should be setting it to false.

If the attribute is not present or its value is an empty string or undefined, the default value of false applies

It wouldn't really affect what is rendered because a null prop isn't included in the node's attributes, so it is just a matter of how explicit aria-current should be.

[brandontruggles]

@pshrmn Looking at the implementation of this...is there ever a time where any of the above values are used other than true?

If you take a look at the following line, where aria-current is passed to Link, the value only ever evaluates to true or null: https://github.com/ReactTraining/react-router/blob/233e6c51c047f1dc4e1cbabc48e7935a2a2b9d64/packages/react-router-dom/modules/NavLink.js#L45

Doesn't this mean that no matter what value is passed to ariaCurrent in NavLink (other than false), that the same value of true will be used regardless? This would mean that making "page" the default option would make no difference in the actual DOM property of the underlying link. Am I missing something? Is this intentional?

I think we can keep null as the default value, as the default value of aria-current on the underlying DOM link will be false when no aria-current property is explicity passed to it.

[pshrmn]

isActive && ariaCurrent uses the value of ariaCurrent

[timdorr]

@brandonrninefive The boolean logic is a shortcut to a longer ternary (or even longer if/else). If isActive is true, then the result will be the value of ariaCurrent. It doesn't convert it to a binary.

Another neat fact is that the second value in a && pair won't be evaluated if the first is false, resulting in some possible performance optimization by avoiding expensive operations (e.g., !isCached && populateCache())

But I think the problem here is the string "false" vs the actual boolean typed value of false. The spec refers to the boolean, even though you can use quotes to make it a string too. It's weird and subtle. Fortunately, since we have React handling things for us, we can leave this out and let it deal with it.

[brandontruggles]

@timdorr Ah, I did not know about that shorthand for the ternary operator. Thank you for the info! If there's anything else needed for this PR, just let me know!