Open lanitochka17 opened 3 months ago
Triggered auto assignment to @CortneyOfstad (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.
@CortneyOfstad 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-vsp
@CortneyOfstad Eep! 4 days overdue now. Issues have feelings too...
Sorry, was OoO!
Was able to recreate, so going to get eyes on this!
Job added to Upwork: https://www.upwork.com/jobs/~0174c82b024da104ac
Triggered auto assignment to Contributor-plus team member for initial proposal review - @rojiphil (External
)
There're 2 problems:
https://github.com/Expensify/App/assets/165644294/5463b3a0-fad6-47f5-bc50-d2cea010e495
but in web platform, we use the div with contentEditable to present the input
-> we can't get the active input -> the logic to blur input here is not triggered
shouldEnableNewFocusManagement
inbut we did not enable it in PopoverWithMeasuredContent
getActiveInput
is implemented for both native and web platform, but it's not the same as this comment so we can separate this function in 2 file index.ts
and index.native.ts
For native platform, we can keep using currentlyFocusedInput
For web/desktop platform, we can use document.activeElement
const activeElement = document.activeElement
if(activeElement?.getAttribute('contentEditable') || activeElement?.tagName === 'INPUT'){
return activeElement
}
shouldEnableNewFocusManagement
in PopoverWithMeasuredContentUse ReportActionComposeFocusManager.composerRef.blur()
in
https://github.com/Expensify/App/assets/165644294/6450ad67-5890-4551-8741-751b0587346a
This is not a #newdot-quality bug - the feature is not unreliable, slow or performing redundant or unexpected API calls, it just doesn't work as designed
Thanks for the clarification @muttmuure!
@rojiphil there is a proposal here for review 👍
@rojiphil, @CortneyOfstad Eep! 4 days overdue now. Issues have feelings too...
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@rojiphil any updates on the reviews? Please have these completed by EOD.
@rojiphil @CortneyOfstad 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!
@rojiphil, @CortneyOfstad Still overdue 6 days?! Let's take care of this!
@dominictb Thanks for the proposal.
getActiveInput is implemented for both native and web platform, but it's not the same as this comment so we can separate this function in 2 file index.ts and index.native.ts
I am not sure if I understand why we need to separate getActiveInput
into two files. Especially since the keyboard gets closed on launching emoji picker as shown below even without this change. Can you please share more details on this?
https://github.com/Expensify/App/assets/3004659/58095810-5c07-4749-88fd-72399ee8524b
There're 2 reasons to separate this function into 2 files
currentlyFocusedField
is used for web/desktop, and currentlyFocusedInput
for native platform.
div
tag to render the input so TextInput.State.currentlyFocusedField()
won't work. That why I decide to use another way to detect active editable element (input or editable div) Can you reproduce the first bug I mentioned in my proposal?
Keyboard does not close when users open popover from three dot
@rojiphil ^
@rojiphil, @CortneyOfstad Huh... This is 4 days overdue. Who can take care of this?
Hi @rojiphil, Can you review my proposal above ^? Thanks
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
Can you review my proposal above ^? Thanks
@dominictb Sorry for the delay here. I will review your comments and provide an update sometime tomorrow my day.
Can you reproduce the first bug I mentioned in my proposal?
Keyboard does not close when users open popover from three dot
I could not reproduce this. I think there is a change in the UI design and the 3dot menu does not exist anymore.
Regarding separating getActiveInput into two files, I may be missing something here. If you notice in the video below for iOS mWeb, the issue does not occur with eMoji picker but issue occurs for action menu. Why this difference?
https://github.com/Expensify/App/assets/3004659/93837ab3-6ede-4876-9b84-fbe6f93a58f3
Also, why does the keyboard hide on Android mWeb whereas keyboard does not hide on iOS mWeb?
https://github.com/Expensify/App/assets/3004659/d0b9817d-e34c-48d5-94fb-dac31d0c10da
@rojiphil I think it's the platform problem. On iOS, I can long press at any unresponsive place and the keyboard is not hidden. But on android it's hidden.
After applying 2 updates as mentioned in my proposal, it works well. I believe it's the reason to add shouldEnableNewFocusManagement
to blur the input.
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@rojiphil, @CortneyOfstad Eep! 4 days overdue now. Issues have feelings too...
@rojiphil @CortneyOfstad this issue is now 4 weeks old, please consider:
Thanks!
@rojiphil any additional feedback on the comment here?
@rojiphil, @CortneyOfstad Still overdue 6 days?! Let's take care of this!
I think it's the platform problem. On iOS, I can long press at any unresponsive place and the keyboard is not hidden. But on android it's hidden.
Hmm… Ok. Looks like an android specific issue and may be unrelated to this issue. Let us revisit this a little later.
After applying 2 updates as mentioned in my proposal, it works well. I believe it's the reason to add shouldEnableNewFocusManagement to blur the input.
@dominictb I am still curious to know about my comment here on why the eMoji picker works fine without the proposed changes as per your proposal. Can you please investigate on this?
@rojiphil, @CortneyOfstad Whoops! This issue is 2 days overdue. Let's get this updated quick!
@dominictb any update on the comment here?
will give the feedback in a few hours
@rojiphil Ah, when you press(single click) on mweb, the keyboard is hidden automatically. But it didn't work in case of long press as I said here
After applying 2 updates as mentioned in my proposal, it works well. I believe it's the reason to add shouldEnableNewFocusManagement to blur the input.
@dominictb I am still curious to know about my comment here on why the eMoji picker works fine without the proposed changes as per your proposal. Can you please investigate on this?
@dominictb I am particularly interested to know this. Can you please update on this?
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@rojiphil, @CortneyOfstad Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
@rojiphil I already explained in here. What points do you want me to elaborate?
-> we can't get the active input -> the logic to blur input here is not triggered
@dominictb I am not thoroughly convinced yet of the proposed solution. But maybe I am missing something. Anyway, as I understand, not triggering the blur of the composer is the root cause of this issue. If so, would it not be simpler to blur the composer if it is focused when we show popover here?
@rojiphil Yes we can do that, but I think we already had the logic to do that in
-> we need to fix the bug at this place instead of making the workaround
Yes we can do that, but I think we already had the logic to do that
we need to fix the bug at this place instead of making the workaround
@dominictb I think the new logic was originally added for emoji picker keyboard issue to ensure that the keyboard focus comes back on dismissing the emoji modal.
However, the logic for bringing the focus back to composer for ReportActionContextMenu
is managed by ReportActionComposeFocusManager
as mentioned here.
So, why is it a workaround to make use of ReportActionComposeFocusManager
for blurring the composer?
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@rojiphil You mean using ReportActionComposeFocusManager.composerRef.blur()
in
,right?
@rojiphil Is the original issue not reproducible? The remaining issue is with long-pressing, right?
mWeb/Safari - Chat - Keyboard does not close when action menu opens
The original issue is not reproducible due to changes in the three dots UI, but a similar issue occurs when long pressing the reportActionItem.
In safari IOS, when we long pressing component, the keyboard will not automatically hide and we only intentionally blur the input focus in OptionRowLHN secondaryInteraction event
Move this code: https://github.com/Expensify/App/blob/8e79c59615feb7de82e4aaa42d8b2297542f5b3b/src/components/LHNOptionsList/OptionRowLHN.tsx#L167-L170
To the method that calls the popOver: showPopover
, showContextMenuForReport
, showPopoverMenu
e.g.
src/components/ShowContextMenuContext.ts
function showContextMenuForReport(
event: GestureResponderEvent | MouseEvent,
anchor: ContextMenuAnchor,
reportID: string,
action: OnyxEntry<ReportAction>,
checkIfContextMenuActive: () => void,
isArchivedRoom = false,
) {
if (!DeviceCapabilities.canUseTouchScreen()) {
return;
}
// Ensure that we blur the composer when opening context menu, so that only one component is focused at a time
if (DomUtils.getActiveElement()) {
(DomUtils.getActiveElement() as HTMLElement | null)?.blur();
}
When onSecondaryInteraction
or onLongPress
show a popOver, add this code:
if (DomUtils.getActiveElement()) {
(DomUtils.getActiveElement() as HTMLElement | null)?.blur();
}
For instance:
onSecondaryInteraction={(event) => {
// Ensure that we blur the composer when opening context menu, so that only one component is focused at a time
if (DomUtils.getActiveElement()) {
(DomUtils.getActiveElement() as HTMLElement | null)?.blur();
}
showPopover(event);
}}
or that can also be placed in showContextMenu(
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: 1.4.85-6 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): gibethlehem+3gmail.com Issue reported by: Applause - Internal Team
Action Performed:
Expected Result:
The keyboard closes when the action menu is opened
Actual Result:
The keyboard stays open when the action menu is opened
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/2497443c-14a8-4c30-86c0-f96de3c46c55
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @rojiphil