-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Fix: Remove accessible={false} blocking Appium automation #78527
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
Changes from all commits
05d1030
f89872b
5e2e910
719be11
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1983,7 +1983,7 @@ function PureReportActionItem({ | |
| preventDefaultContextMenu={draftMessage === undefined && !hasErrors} | ||
| withoutFocusOnSecondaryInteraction | ||
| accessibilityLabel={translate('accessibilityHints.chatMessage')} | ||
| accessible | ||
| accessibilityRole={CONST.ROLE.BUTTON} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kirillbilchenko Sorry for the late question, but why do we need this?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @shubham1206agra sure, from the guide https://github.com/Expensify/App/blob/main/contributingGuides/ACCESSIBILITY.md
So chat message in the end is interactive element that user need press on to open context menu, and that's the reason
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This line introduced multiple regressions.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @situchan could be the case, but this is something mentioned in the guide, so in this case make sense to update the guide or add some additional information about this corner case. This is very good catch in the end, but I'm not sure how this can be prevented in future.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @NJ-2020 by the way revert will not break this pr, this was added only because I was told to follow accessibility guide that we have in repo, and that's the only reason why it was added, I gave my explanation before, so maybe
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. git bisect points to this line as the cause of this issue #86871 |
||
| sentryLabel={CONST.SENTRY_LABEL.REPORT.PURE_REPORT_ACTION_ITEM} | ||
| > | ||
| <Hoverable | ||
|
|
||
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.
@shubham1206agra this is still a concern
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.
I believe this comment is for @kirillbilchenko
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.
in this case this property is by default equal to true, and removing it just removing unnecessary declaration