Skip to content
This repository has been archived by the owner on Jun 3, 2024. It is now read-only.

QMAPS-1926 Allow scroll in suggest on iOS #1083

Merged
merged 7 commits into from
May 28, 2021

Conversation

bbecquet
Copy link
Contributor

@bbecquet bbecquet commented May 11, 2021

Description

Make it possible to scroll suggest results on iOS.
Previously, the whole page content scrolled. This was due to the virtual keyboard on iOS browsers not changing the viewport height. As a result, the list of suggestions fit entirely on the screen height (even if half of it was behind the keyboard) so it wasn't scrollable.
Here, we use a recent web API, Visual Viewport, which is supported in Safari 13+ and gives access to the real visible height, without the space taken by the keyboard. We use it to give a fixed height to the suggest list, so it becomes scrollable.

We also have to manage the case of an empty list or a list too small to be scrollable. In that case, the dropdown background element will scroll the page like before. So there is a special test for that to detect if the suggestions overflow their container or not. If not, we simply cancel any touchmove action.

Before After
https://user-images.githubusercontent.com/243653/119379835-41e1ec80-bcc0-11eb-888d-1c59b814256f.MOV https://user-images.githubusercontent.com/243653/119379907-532af900-bcc0-11eb-9a70-ebfedb19da74.MOV

@bbecquet
Copy link
Contributor Author

bbecquet commented May 17, 2021

Little update.
I think I've got everything working in a local PoC. By working, I mean:

  • when there are scrollable results, they can be scrolled without scrolling the whole body
  • when there are no results or a result list smaller than the overflow threshold, touching them don't scroll the whole body
  • touching the search bar doesn't scroll the body either

Actually, this also fixes a long standing bug on small non-iOS devices, where the last items of the result lists couldn't be reached, hidden by the keyboard.

The thing is the implementation is a bit messy for now. And if we build on top of the current master, it will stay that way, with global selectors and such. I really think finishing the React conversion of the TopBar before that would make things easier and cleaner. Done

@bbecquet bbecquet force-pushed the QMAPS-1926-iphone-suggest-scroll branch from 5c84891 to 000d4d9 Compare May 24, 2021 16:32
@bbecquet bbecquet marked this pull request as ready for review May 24, 2021 16:47
@bbecquet
Copy link
Contributor Author

Ok, since #1083 (comment) the React TopBar conversion has been merged, and I've updated this PR with a proposed final implementation based on it.

xem
xem previously approved these changes May 25, 2021
Copy link
Contributor

@xem xem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve the code.
Sadly, even on Browserstack + local testing, I can't manage to test it on iOS.
Let's test with a real iphone once it's in plive, ok?

@@ -101,6 +102,31 @@ const Suggest = ({
}
};

useEffect(() => {
if (isMobile && dropdownVisible && window.visualViewport) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that window.visualViewport is not available by default on Firefox for desktop, including in the responsive view.
(the flag dom.visualviewport.enabled can be enabled via "about:config").
So it could make testing a bit confusing, especially for a newcomer. Do you think it could be documented somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I've added a link to the API doc.

outputNode.removeEventListener('touchmove', cancelTouchScrollIfNotOverflow);
};
}
}, [isMobile, items, dropdownVisible, outputNode]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this effect could also listen to the visualViewport resize event, to adapt the element height after the keyboard is closed.
This is quite minor though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added it, as it was less difficult than what I thought at first.
Let's hope I didn't break anything else :P

@@ -36,6 +36,8 @@
position: fixed;
top: 64px;
width: 100vw;
background-color: $background;
overflow-y: scroll;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, why is this explicit "scroll" required, instead of "auto" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right it's not, I guess I got heavy-handed while battling with Safari :) I'll put auto instead.

@bbecquet bbecquet requested a review from amatissart May 28, 2021 15:31
@bbecquet bbecquet merged commit b820d8a into Qwant:master May 28, 2021
@bbecquet bbecquet deleted the QMAPS-1926-iphone-suggest-scroll branch May 28, 2021 16:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants