Skip to content
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

[$2000] mWeb - Extra space is displayed below the text box #8731

Closed
kbecciv opened this issue Apr 21, 2022 · 56 comments
Closed

[$2000] mWeb - Extra space is displayed below the text box #8731

kbecciv opened this issue Apr 21, 2022 · 56 comments
Assignees
Labels
Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Weekly KSv2

Comments

@kbecciv
Copy link

kbecciv commented Apr 21, 2022

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Open the URL staging.new.expensify.com
  2. Login
  3. Go to any chat
  4. Click on the text box

Expected Result:

No extra space is displayed below the text box

Actual Result:

Extra space is displayed below the text box

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • Mobile Web/safari - iPhone 12

Version Number: 1.1.56.0

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers): n/a

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Bug5541421_QA.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause

Slack conversation:

View all open jobs on GitHub

@melvin-bot
Copy link

melvin-bot bot commented Apr 21, 2022

Triggered auto assignment to @luacmartins (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@luacmartins
Copy link
Contributor

This can be external

@luacmartins luacmartins added the External Added to denote the issue can be worked on by a contributor label Apr 21, 2022
@melvin-bot
Copy link

melvin-bot bot commented Apr 21, 2022

Triggered auto assignment to @arielgreen (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@luacmartins
Copy link
Contributor

Waiting for the job to be exported!

@melvin-bot melvin-bot bot removed the Overdue label Apr 25, 2022
@arielgreen
Copy link
Contributor

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Apr 26, 2022
@melvin-bot
Copy link

melvin-bot bot commented Apr 26, 2022

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel (Exported)

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 26, 2022
@melvin-bot
Copy link

melvin-bot bot commented Apr 26, 2022

Current assignee @luacmartins is eligible for the Exported assigner, not assigning anyone new.

@melvin-bot melvin-bot bot changed the title mWeb - Extra space is displayed below the text box [$250] mWeb - Extra space is displayed below the text box Apr 26, 2022
@SALAH-EDDIB
Copy link

this Issue can be solved using css the problem is caused by two things first is an empty default div probably from the input library used and the other is space created with the min-height of the input
image

@aneequeahmad
Copy link
Contributor

PROPOSAL

PROBLEM AND SOLUTION

  • So the minHeight:90 is applied to the parent View in ReportActionCompose.js that should be removed for mobile web (check for mWeb would do the work to remove for specific mWeb).

  • Also, this style of styles.chatItemComposeSecondaryRow has to be removed for mWeb in this <View>
    <View style={[styles.chatItemComposeSecondaryRow, styles.flexRow, styles.justifyContentBetween]}>

  • Also, in ReportScreen minHeight in <View style={styles.chatFooter}> should be reduced to 50 from 65 in the case of mWeb OR

Above points when applied removes the extra space displayed below the text box.

@luacmartins
Copy link
Contributor

@aneequeahmad The issue seems to come from the browser toggling the navigation bar and the window not resizing properly to fill in the extra space. I think that we should address that behavior in the solution.

@aneequeahmad
Copy link
Contributor

@luacmartins Please look at the screenshots I have attached below and explained in steps.

  • there is a div of height 15px below text input.

Screen Shot 2022-05-01 at 2 07 25 AM

  • Also the container of textinput has min-height: 90px

Screen Shot 2022-05-01 at 2 08 52 AM

Solution

  • It looks like the minHeight is forcing the text input to have 90px height and removing it and removing height:15px from div of bottom of text input for mWeb fixes the issues.

After Fix
Screen Shot 2022-05-01 at 2 09 27 AM

@Santhosh-Sellavel
Copy link
Collaborator

@aneequeahmad The space is expected when the keyboard is not active. There is much more space when the keyboard is active. Check image below

Bug5541421_QA.mp4

@luacmartins
Copy link
Contributor

@aneequeahmad

there is a div of height 15px below text input.

This space is reserved for messages like User is typing or You appear to be offline. We should keep it there, since removing it would cause the compose input to shift upwards when we display those messages and that's not what we want.

Also the container of textinput has min-height: 90px

This is also expected, since it accounts for the temporary messages I described above + 1 line compose input + timezone message. We should not remove this.

As @Santhosh-Sellavel described, there's much more space that shows up once the navigation bar is hidden or the keyboard is displayed and that's the behavior that we should address.

@arielgreen arielgreen changed the title [$250] mWeb - Extra space is displayed below the text box [$500] mWeb - Extra space is displayed below the text box May 3, 2022
@arielgreen
Copy link
Contributor

Price increased.

@luacmartins
Copy link
Contributor

Still looking for proposals!

@luacmartins
Copy link
Contributor

@liyamahendra that is one of the causes. Another one is when the browser's address search bar is toggled (by scrolling the page)

@liyamahendra
Copy link
Contributor

Proposal

Problem

It is a known bug on iOS Mobile Safari, where extra spacing is added to the bottom of the page when the keyboard pops-up.

Solution

The workaround solution to this bug in mobile safari, is to snap & scroll the page back to the bottom end of the body when the keyboard has finished showing up completely.

Since the resize event didn't work on mobile safari, I resolved to using window.visualViewport and listen to it's scroll event.

Following is the my code for the above workaround fix:

 <script>
        var timer;
        var scrollEndDelay = 200; 
        
        var normalHeight = 0;

        var correctedSpacing = false;

        // https://developer.apple.com/library/archive/documentation/StringsTextFonts/Conceptual/TextAndWebiPhoneOS/KeyboardManagement/KeyboardManagement.html
        const keyboardHeightInPortrait = 432;
        const keyboardHeightInLandscape = 324;
        
        window.addEventListener("DOMContentLoaded", function() {
            if(normalHeight === 0) {
                normalHeight = window.innerHeight;
            }
        });

        function finishedShowingKeyboard () {
            
            if(window.innerHeight < normalHeight) {
                console.log("*** Keyboard shown ***");

                if(!correctedSpacing) {
                    correctedSpacing = true; 

                    // Detect orientation to use appropriate keyboard height
                    let isPortrait = window.matchMedia("(orientation: portrait)").matches;

                    // fully scroll the window to a position where it can't scroll anymore
                    window.scroll(0, (isPortrait) ? keyboardHeightInPortrait: keyboardHeightInLandscape);

                    // scroll the window back to the Y position of the bottom of the "root" div
                    setTimeout(function() {
                        let root = document.getElementById("root");
                        window.scroll(0, root.getBoundingClientRect().bottom);
                    }, 10);
                }
            } else {
                console.log("*** Keyboard dismissed ***");
                correctedSpacing = false;
            }
        }

        function viewportScrollHandler() {
            clearTimeout(timer); // clearing the timer as the scroll is still in progress
            timer = setTimeout(finishedShowingKeyboard, scrollEndDelay); // set the timer, so that when the scroll ends, this isn't cancelled and is execcuted
        }

        window.visualViewport.addEventListener('scroll', viewportScrollHandler);
        
</script>

Below is the screencast showing the workaround fix in action. The page automatically scrolls back to the top of the keyboard leaving no empty space visible to the user. I tested it with both bottom URL Address bar and top URL Address Bar.

update.mp4

@Santhosh-Sellavel Let me know your thoughts!

@Santhosh-Sellavel
Copy link
Collaborator

Santhosh-Sellavel commented May 30, 2022

I see when keyboard is space below composer( message input box) is reduced than usual size.

How your solution works on android mWeb, iPad & Android tablet mWeb ?

@Santhosh-Sellavel
Copy link
Collaborator

@liyamahendra We don't accept workarounds, I believe that's the case here too. We are looking for a full fledged solution that's solves the issue and doesn't cause regression on any other platform.

cc: @luacmartins

@jrstanley
Copy link
Contributor

@Santhosh-Sellavel @liyamahendra I think we would also need to consider custom keyboards here, and text inputs that may not be at the bottom of the screen.

@luacmartins
Copy link
Contributor

I agree with @Santhosh-Sellavel and @jrstanley! @liyamahendra thanks for the proposal, but I believe that your solution is hardcoding values and is not accounting for potential different devices/keyboard types and spaces that are not at the bottom of the screen.

@luacmartins
Copy link
Contributor

Moving this to a weekly as we look for more proposals

@luacmartins luacmartins added Weekly KSv2 and removed Daily KSv2 labels May 31, 2022
@Santhosh-Sellavel
Copy link
Collaborator

@luacmartins
I'm going on OOO. But I still keeping this assigned to me. If there are any immediate C+ duties needed here feel free to unassign me & re-apply exported label thanks!

@mvtglobally
Copy link

Issue not reproducible during KI retests. (First week)

@arielgreen
Copy link
Contributor

Since not reproducible, should we close?

@luacmartins
Copy link
Contributor

Issue not reproducible during KI retests. (First week)

Interesting! Maybe something changed on the OS/browser side 🤔

@arielgreen let's keep the issue open for another week to see if it's reproducible during the next KI retests. Meanwhile, let's NOT increase the price anymore.

@ahmdshrif
Copy link
Contributor

@luacmartins @mvtglobally I still repro it
also, I report another related issue here.

173244700-4765ef52-61a7-4f94-a414-0c5fff7ea914.mov

@luacmartins
Copy link
Contributor

@ahmdshrif I managed to repro as well. You're welcome to send a proposal if you are interested.

@Santhosh-Sellavel
Copy link
Collaborator

Still, we are open to proposals here.

@melvin-bot melvin-bot bot added the Overdue label Jun 23, 2022
@luacmartins
Copy link
Contributor

Still looking for proposals

@melvin-bot melvin-bot bot removed the Overdue label Jun 24, 2022
@arielgreen
Copy link
Contributor

@luacmartins is there a reason we're not increasing price? We shouldn't sit endlessly on issues; if it's valuable enough to fix, we should pay for it. If not, we should close it.

@luacmartins
Copy link
Contributor

Thanks for the ping @arielgreen! I'll just close this issue. This seems to be a well-known bug and any solutions would be a complex workaround.

@melvin-bot
Copy link

melvin-bot bot commented Aug 11, 2022

⚠️ Looks like this issue was linked to a possible regression on PRODUCTION here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@melvin-bot
Copy link

melvin-bot bot commented Aug 24, 2022

⚠️ Looks like this issue was linked to a possible regression on PRODUCTION here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Weekly KSv2
Projects
None yet
Development

No branches or pull requests