Open lanitochka17 opened 1 month ago
Triggered auto assignment to @lschurr (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.
@lschurr FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors
We think that this bug might be related to #vip-vsb
Job added to Upwork: https://www.upwork.com/jobs/~01a3374b61f565b9cf
Triggered auto assignment to Contributor-plus team member for initial proposal review - @situchan (External
)
I'm the author of https://github.com/Expensify/App/pull/44399. That's our bad not to note a precondition for the composer to automatically refocus: it must already be focused before interacting with the context menu. So this is not a valid bug considering the current expectations. We can still change the expectations but should note that this is not a regression. Thanks.
Edited by proposal-police: This proposal was edited at 2024-08-05 18:25:46 UTC.
The composer is NOT refocused
When a report is focused, we set the composer focus callback with shouldFocusForNonBlurInputOnTapOutside
as false
by default
When we click on the copy link, we call ReportActionComposeFocusManager.focus
function to focus on the composer. The difference here is willBlurTextInputOnTapOutside
is false
in the native platform and true
in other platforms and shouldFocusForNonBlurInputOnTapOutside
is false
by default so the composer is always re-focused on web/desktop and not re-focused on native
We should keep track the composer is focused or not when we open the context menu. Then we pass it as payload data when we open the context menu.
ReportActionComposeFocusManager
, create a new variable to keep track of the time the composer is blurred. And create a function to check this. We have a check time - (composerBluredTime ?? 0) <= 100
because when we open the context menu, the composer is blurred and then the context menu is opened immediately.let composerBluredTime: number | null = null;
function blur() {
composerBluredTime = new Date().getTime();
}
function isJustBlured(time: number) {
return time - (composerBluredTime ?? 0) <= 100;
}
ReportActionComposeFocusManager.blur()
here when the composer is blurredReportActionComposeFocusManager.blur()
isComposerFocused
hereisComposerFocused = false,
PopoverReportActionContextMenu
, get this param here and pass it to BaseReportActionContextMenu
isComposerFocused
as a payload data hereisComposerFocused
param as ReportActionComposeFocusManager.isJustBlured(new Date().getTime()) || ReportActionComposeFocusManager.isFocused()
when we open the context menu here.false,
ReportActionComposeFocusManager.isJustBlured(new Date().getTime()) || ReportActionComposeFocusManager.isFocused(),
isComposerFocused
here and check if it's true we will hideContextMenu(true, () => {
if (!isComposerFocused) {
return;
}
ReportActionComposeFocusManager.focus(true);
});
The test branch here https://github.com/nkdengineer/App/tree/fix/45204
If we want to not focus on the composer after we select the context menu action in mWeb, we can add shouldFocusInputOnScreenFocus
check here.
if ((!willBlurTextInputOnTapOutside && !shouldFocusForNonBlurInputOnTapOutside && !shouldFocusInputOnScreenFocus) || !isFocused) {
return;
}
shouldFocusInputOnScreenFocus
is used to consistent the focus behavior mWeb and native so we can use this
Platform | Focused before menu | Focused after copying link (before fix) | Focused after copying link (after fix) |
---|---|---|---|
Web | ✅ | ✅ | |
Web | ❌ | ✅ | |
mSafari | ✅ | ✅ | |
mSafari | ❌ | ✅ | |
mChrome | ✅ | ✅ | |
mChrome | ❌ | ✅ | |
iOS | ✅ | ✅ | |
iOS | ❌ | ❌ (bug to fix) | ✅ |
Android | ✅ | ✅ (but no keyboard popup) | |
Android | ❌ | ❌ (bug to fix) | ✅ |
So the expected behavior is to refocus always on all platforms regardless of pre-focus status?
So the expected behavior is to refocus always on all platforms regardless of pre-focus status?
I think it's expected.
Any update @situchan? Are we saying this is expected and not a bug?
@lschurr, @situchan Whoops! This issue is 2 days overdue. Let's get this updated quick!
Any update @situchan? Are we saying this is expected and not a bug?
I am saying this is not worth fixing.
@Expensify/design what do you think of https://github.com/Expensify/App/issues/45204#issuecomment-2224812289?
Hmm interesting one for sure. My first reaction is that I don't think I would expect the composer to be refocused after using the Copy link menu item. The user is basically electing to tap outside of the composer box... so I don't think we should assume that they want to head back into the composer box immediately after interacting with a different part of the app. Curious what Danny and Jon think though!
At the same time, if we only have two places (iOS and Android) where we aren't doing this behavior, I can see where it makes sense to make everything consistent so we have the same exact behavior on all platforms.
My first reaction is that I don't think I would expect the composer to be refocused after using the Copy link menu item. The user is basically electing to tap outside of the composer box... so I don't think we should assume that they want to head back into the composer box immediately after interacting with a different part of the app.
Totally agree with this 100%.
At the same time, if we only have two places (iOS and Android) where we aren't doing this behavior, I can see where it makes sense to make everything consistent so we have the same exact behavior on all platforms.
Personally I think it makes more sense to not refocus the input on any platform after an action like this... but that might just be my personal preference :run-away:
.
On mobile, the keyboard takes up so much space and sometimes it can be hard to remove focus from the composer (and get the keyboard to go away). And trying to browse a conversation with the keyboard up is pretty frustrating. So I think I would still prefer not auto-focusing the composer. Just :my-two-cents:
.
Thanks for the feedback. Looks like we wanna focus only on web / desktop, not focus on all mobile platforms (iOS, android, mWeb) for consistency.
Platform | Focused before menu | Focused after copying link (before fix) | Focused after copying link (after fix) |
---|---|---|---|
Web | ✅ | ✅ | |
Web | ❌ | ✅ | |
mSafari | ✅ | ✅ | |
mSafari | ❌ | ✅ (bug to fix) | ❌ |
mChrome | ✅ | ✅ | |
mChrome | ❌ | ✅ (bug to fix) | ❌ |
iOS | ✅ | ✅ | |
iOS | ❌ | ❌ | |
Android | ✅ | ✅ (but no keyboard popup) | |
Android | ❌ | ❌ |
I'm with Danny on this one. I don't think it makes that much sense to do on mobile due to the keyboard popping up. On desktop you got two input methods (keyboard and mouse) and focusing the composer doesn't break any flow or require a dismissal so that's fine for me. But on mobile, I just think it'd be more disruptive
Updated proposal https://github.com/Expensify/App/issues/45204#issuecomment-2222341773 with new expected behavior.
Cool cool, thanks to you both for your thoughts - I like where you landed.
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@nkdengineer's alternative solution to use canFocusInputOnScreenFocus
looks good to me.
(Note: !willBlurTextInputOnTapOutside &&
condition should be removed. Otherwise next conditions will never run on mWeb)
New expected behavior after discussion: https://github.com/Expensify/App/issues/45204#issuecomment-2231479608
🎀👀🎀 C+ reviewed
Triggered auto assignment to @srikarparsi, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
Hm, I think there might've been a little confusion here but let's confirm.
I think based on what @shawnborton said here and what @dannymcclain said here, the composer shouldn't focus after a message was copied (if it wasn't focussed already).
Based on this diagram, it seems like we want to focus the composer after copying only on web and desktop (based on my testing) but not other platforms. I understand the keyboard argument on smaller screens but I think it makes sense to not focus the composer if it wasn't already focussed on any platform. @shawnborton @dannymcclain, I think this maybe is what you are also saying but wanted to confirm.
but I think it makes sense to not focus the composer if it wasn't already focussed on any platform
Yup, I can totally get down with that approach (cc @Expensify/design for vis)
Yeah that's what I was thinking too.
So what's next on this one? Are we fixing and assigning to a Contributor or closing?
@srikarparsi could you clarify?
Hi yes, I updated the issue body and the title to reflect the expected behavior. @nkdengineer would you like to update your proposal to cover this case for all platforms and not just mSafari and mChrome? I can open it up to proposals if you can't get to this one.
@srikarparsi So we don't want to refocus when we click on any option in context menu, right?
I update my main solution https://github.com/Expensify/App/issues/45204#issuecomment-2222341773 with new expected behavior.
Platform | Focused before menu | Focused after copying link (before fix) | Focused after copying link (after fix) |
---|---|---|---|
Web / desktop | ✅ | ✅ | |
Web / desktop | ❌ | ✅ (bug to fix) | ❌ |
mSafari | ✅ | ✅ | |
mSafari | ❌ | ✅ (bug to fix) | ❌ |
mChrome | ✅ | ✅ | |
mChrome | ❌ | ✅ (bug to fix) | ❌ |
iOS | ✅ | ✅ | |
iOS | ❌ | ❌ | |
Android | ✅ | ✅ (but no keyboard popup) | |
Android | ❌ | ❌ |
@srikarparsi please confirm this diagram is final
@lschurr, @srikarparsi, @situchan Whoops! This issue is 2 days overdue. Let's get this updated quick!
@srikarparsi please confirm this diagram is final
Yes, this looks good. Let's also add desktop to the list. It currently has the same behavior as web and we should also fix it.
@nkdengineer we should still refocus on all platforms if it was focused already. Looks like your main solution stops refocusing no matter previously focused or not.
@situchan When we open the context menu, the composer is always unfocused so I don't think we need to re-focused on the composer if it was focused.
cc @srikarparsi What do you think?
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@lschurr @srikarparsi @situchan this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!
When we open the context menu, the composer is always unfocused so I don't think we need to re-focused on the composer if it was focused.
Sorry for the delayed reply. We should refocus the composer anytime the composer was already focussed. There was a little reasoning for that here. We only don't want to refocus the composer if it wasn't already focussed before opening the context menu.
Got it, let me check again and update my solution.
@situchan I updated my main solution https://github.com/Expensify/App/issues/45204#issuecomment-2222341773 with new expected behavior.
Just a bump here @situchan
@nkdengineer please correct RCA based on new expected behavior. It should explain why web based platforms always refocus while not on native.
We should keep track the composer is focused or not when we open the context menu.
This logic should already exist and work perfect on native.
@situchan I updated my RCA.
https://github.com/Expensify/App/issues/45204#issuecomment-2222341773
This logic should already exist and work perfect on native.
What is the logic?
What is the logic?
I believe @situchan is referring to the logic which keeps track of whether or not the composer is focusses when we open the context menu.
I mean I want to know what logic in the App we can use to do that.
I meant we already have that logic on native. If not, how come native app works in current version?
@situchan On native, if the composer is focused, it will not be unfocused when we click outside or do anything except close the report. If the composer is not focused, when we copy the link this will not focus again as I mentioned in the RCA of my proposal.
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@lschurr, @srikarparsi, @situchan Eep! 4 days overdue now. Issues have feelings too...
reviewing updated proposal
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: 9.0.6-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Email or phone of affected tester (no customers): applausetester+en@applause.expensifail.com Issue reported by: Applause - Internal Team
Issue found when executing PR https://github.com/Expensify/App/pull/44399
Action Performed:
Expected Result:
User doesn't expect the composer to get refocused.
Actual Result:
The composer is refocused.
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
https://github.com/Expensify/App/assets/78819774/04a02f66-9bc9-42f3-a9c9-c0ae026f953c
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @situchan