RFC: readability vs RTL compatibility in API naming

[mdjastrzebski]

RNTL is modelled after RTL, as one of our unstated goals we strive to reimplement RTL for React Native platform, expose familiar features and APIs while accounting for platform differences.

So far we have been reasonably successful with this approach as it delivered familiar and well thought out API. However recently, when we started to implement A11y-related features I’ve found that some of them are somewhat confusingly named:

• isInaccessible - which checks if the element is hidden from accessibility, which is a different concept that being accessibility element as indicated by accessible prop. Values of accessibility prop and results of isInaccessible are independent and all four combinations (true/false) are allowed.
• hidden query option - this option controls whether we include elements hidden from accessibility in our queries. However, it is unintuitive whether hidden: true or hidden: false includes the hidden elements and I have to refer to the documentation to check that.

I think readability of our code could be improved by renaming these in a following way:

• isInaccessible -> isHiddenFromAccessibility
• hidden option -> includeHidden

Note: the change is only about the naming, the arguments or behaviour do not change.

Such renaming would however break our naming compatibility with RTL.

I see couple of options how we could approach that:

1. Keep RTL naming
2. Rename them
3. Expose both names: promote the readable name as primary, while allowing RTL-compatible name as a secondary option.

I think we should consider using 3rd option, as it gives us both more explicit and readable code while catering for people with RTL background.

@thymikee @AugustinLF @pierrezimmermannbam @MattAgn What do you think?

[AugustinLF]

I vote for the third solution. Or even better, suggest RTL to change :) I agree that we need better name, and RTL's naming shouldn't be the lowest denominator. However having the same names is very important

• for people working with both web and native, this'll lead to painful times where you end up using the wrong variable name
• at Klarna we have a wrapper library to be able to write our tests once and run on both platforms (branching to RTL or RNTL), so consistent API is important, especially for things like parameters, or properties on object returned (queries for instance). What I mean is that it's easy enough for us to alias isInaccessible through an export, but aliasing hidden is kind of ugly, we need to create a wrapper function, it'll pollute stack traces, etc.

[pierrezimmermannbam]

I agree with @AugustinLF , the naming hidden is very confusing to me so I think it should change so I'm not in favor of the first option and keeping compatibility with RTL is also very important so having both names seems like the better option

[MattAgn]

I agree with the third option as well, even though I've been working on the hidden option for quite some time now, I still have a hard time remembering what hidden:true means. For hidden, I might even go for includeHiddenElements. If we wanted to be super clear, I'd say includeElementsHiddenFromAccessibility or includeElementsHiddenFromA11y but that might be a bit long

[AugustinLF]

IMO the length of the param shouldn't necessarily be a problem. Ideally you want to enable that as a global settings, and only change it when needed, which hopefully should be almost never.

[pierrezimmermannbam]

I agree with both @AugustinLF and @MattAgn on having a name as explicit as possible

[mdjastrzebski]

Thanks for your feedback guys! I will submit a related PR in the upcoming days.

[mdjastrzebski]

isInaccessible alias to isHiddenFromAccessibility merged as #1211.

How should we proceed with hidden renaming? I would opt for includeHidden naming because my only problem with original prop name is that hidden: true or hidden: false does not give a clear whether hidden elements are actually included or not. Changing that to includeHidden: true makes it clear.

Regarding using includeHiddenFromAccessibility, I think it's too verbose, because there is no other type of hidden (e.g. is includeHiddenFromVisibility), and you would still need to understand the function of the option first time you use it, so includeHidden vs includeHiddenFromAccessibility does not save you that learning step.

Re a11y abbreviation: I think we should avoid it, and use full accessibility spelling in the new features, as a11y might be confusing for user who did not meet with accessibility before. Another reference point would be that RN uses full accessibility spelling for all related props.

@thymikee @AugustinLF @pierrezimmermannbam @MattAgn wdyt about naming propositions. Would someone of you like to contribute PR for hidden option alias, after we establish the name.

I think it would be beneficial to release hidden option support together with improved naming, rather than in separate releases.

[MattAgn]

I agree that the most important is to know what hidden true or false means

As for includeHiddenFromAccessibility, I don't think it matters that much if it's verbose, as @AugustinLF said, most of the time, this option will be rarely used in the queries and the global config should be enough. Furthermore, since it will be rarely be used people might go a few month without using it or seeing it in their codebase. In that case, I think it's best to have a name as clear as possible to help them remember easily what it truly means when they need it.

For a11y VS accessibility and for the release plan, completely agree with you.

I'd be glad to take on the issue and implement the new naming we choose :)

[MattAgn]

For the implementation, here are some of my thoughts and questions (if we were to go with includeHidden for instance):

• for the typing of the queries and the configure function, we should only be able to specify either hidden or includeHidden, not both at the same time. Otherwise we would have to prioritise one over the other which would not be clear
• inside the code (mostly in our custom findAll), we should use the naming includeHidden and not hidden

What do you think?

[mdjastrzebski]

@MattAgn Pls go ahead the PR, I will release RNTL after merging that 🚢

I think we should not complicate things and we should assume that if both options are present then the preferred one (includeHidden) takes precedence over alias one (hidden).

Re naming, let's seen in a PR how the use cases in our tests and doc read with shorter (includeHidden) and/or longer version (includeHiddenFromAccessibility). Maybe that will help us with the naming decision.

[MattAgn]

Alright I'm on it