-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Fix Issue 8608: When using tab key focus does not appear correctly on chat rows #10368
Fix Issue 8608: When using tab key focus does not appear correctly on chat rows #10368
Conversation
@shawnborton Just double checking, are we okay with this? Screen.Recording.2022-08-12.at.5.50.23.AM.mov@andrewlor Still Inputs looks like a 1px outline, Screen.Recording.2022-08-12.at.5.49.13.AM.movcc: @puneetlath |
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.
Please handle this #10368 (comment)
Do we want to increase the border width of inputs to 2px? I think we'd have to make other considerations. The input focus is done through the The goal of this issue is to fix a bug, so I think changes to the general approach to styling is an increase in scope from the original issue. I think the best options to fix this bug are to either keep these changes as they are, or decrease the width back to 1px to be consistent with the inputs (happy to do either). And then it might make more sense to approach the general styling of all of these things holistically in another PR. Let me know what your thoughts are. |
@shawnborton your thoughts |
I'll defer to shawn, but I would think we should leave the inputs as they are. |
It just looks inconsistent to me, but I prefer 1px everywhere. |
I don't think we want to touch inputs or their styling. So maybe you are right, let's just use the simple 1px outline for now. |
@andrewlor Update screen recordings |
@Santhosh-Sellavel Updated screenshots / recordings |
@Expensify/design @puneetlath While testing this PR I found this, tab focus border is broken. Is this an issue should? |
PR Reviewer Checklist
|
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.
🎀 👀 🎀
C+ Reviewed
@puneetlath Tests well for me.
Except for this #10368 (comment). Most likely will be handled in a separate issue if it's a valid one.
Also, this is not caused by this PR, so approving this.
Regarding the comment above, I think that's fine for now. I think we can come back through and polish this up as another project. For example, I really do think that the tab focus should generally appear on the outside of most elements, but I see where it makes sense to do them as inset for things like chat rows. If there is an easy way to do inset for chat rows and outer shadow for all other elements, I'd be down to do that now. Otherwise we can follow up if needed. |
I think we should just do it as a follow-up since that's a bit larger scope than this job. |
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.
Sorry, found two quick things regarding color variables that we should fix.
@shawnborton if you approve the PR then we can merge. |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Details
When using the tab key on web to cycle through focusing of elements, the outline does not appear correctly when cycling through the chat rows.

Fixed Issues
$ #8608
Tests
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
filesSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
displayName
propertythis
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)PR Reviewer Checklist
The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
filesSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
have been tested & I retested again)/** comment above it */
displayName
propertythis
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)QA Steps
Screenshots
Web
chrome.mov
firefox.mov
safari.mov
Mobile Web
ios-safari.mov
(expected functionality still works with tab key on emulator, not sure if it's possible to recreate functionality on real device)
Desktop
desktop.mov
iOS
Android