-
Notifications
You must be signed in to change notification settings - Fork 273
fix: improve text matching #1222
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
Conversation
Codecov ReportBase: 94.67% // Head: 94.72% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1222 +/- ##
==========================================
+ Coverage 94.67% 94.72% +0.05%
==========================================
Files 42 44 +2
Lines 2946 2979 +33
Branches 440 442 +2
==========================================
+ Hits 2789 2822 +33
Misses 157 157
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Should we also considering matching? <View>
<Text>Hello</Text>
<Text>World</Text>
</View> That's an open question, I feel it had some value, but could also lead to weird things. Do screen readers can read this type of text together, or do they treat that as distinct elements? |
@AugustinLF I think in the short term we should not consider supporting that, as the current Afaik how a11y & screen readers work, in your example the However, we are getting into an "edge case" territory here, I would rather not support returning non- |
Yeah, the breaking change factor is a fair one, you're right. And if we want to allow that, it should probably start behind a flag. |
9810bd3
to
cbf35e9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really like the new API/file structure, I feel it makes things much clearer :)
I approve but do agree with the comment about renaming the textContent
file to getTextContent
.
@@ -45,7 +45,7 @@ test('React Native API assumption: nested <Text> renders single host element', ( | |||
</Text> | |||
</Text> | |||
); | |||
expect(getHostSelf(view.getByText('Hello'))).toBe(view.getByTestId('test')); | |||
expect(getHostSelf(view.getByText(/Hello/))).toBe(view.getByTestId('test')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this a breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test relied of nested Text not being matched, so I needed to change whole string match to partial match. This was a limitation on previous text matching and not a feature per se. IMO that is not a breaking change as test required a minor tweak without affecting the essence of the test.
Summary
TLDR: Make this pass:
while not breaking this:
Resolves #1221
Resolves #1181
What
Match only the deepest
Text
element containing given content while supporting nestedText
elements.In the light of #1221, user expectation for handling nested texts is reasonable, this MIGHT be considered as a bug fix rather than a breaking change. Or we might consider this breaking change, since the case is not clear cut.
All in all, as text matching has been complex subject in the past, let's be extra careful with this PR.
How
findAll
function to acceptdeepestOnly
option that when enabled prevents from matching given node if any of its descendants is matched.a. This option could be used with other queries if useful, but can be breaking change depending on usage
b. The updated
findAll
preserves the same tree walk order as initial one sogetByAll*
unit tests relying on matching order should work the same.c. New
findAll
is based onfindAll
from React Test Renderer, with necessary tweaksgetTextContent()
function that can work with nestedText
elements. Implementation based ontoHaveTextContent()
matcher from Jest Native*ByText
queries to match only the deepestText
using newgetTextContent()
function.This should keep only return a single match even in the case of nested
Text
elements. The matchedText
element will be the deepest one that fulfils the predicate.Test plan
Add some simple test for desired effect. Make all existing test pass as is. Tweak only tests that actually checked that there is no nesting text matching.