[Popover] Fix bad positioning on IOS devices #4638

w01fgang · 2016-07-06T22:00:11Z

Adds a device detection helper, maybe useful for cross-browser compatibility and fixes #3638

PR is linted.
Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

igorbt · 2016-08-12T11:29:14Z

@w01fgang we had the same problem, but we fixed it differently, we replaced position fixed with position absolute. Keeping it to position fixed results in uncontrollable animations. I will come back later with more info.

igorbt · 2016-08-12T14:05:24Z

In short, iOS renders fixed positioned elements as absolute when keyboard opens, but this "transition" from fixed to absolute is weirdly and uncontrollable animated and this results in flickering. For our project, the only good solution was to not use position fixed at all. So, both Popover and RenderToLayer should use position absolute instead of fixed. I will come back next week with a PR with the change I described.

w01fgang · 2016-08-14T13:48:05Z

@igorbt when you will make your PR, please let me know.

Taakn · 2016-10-11T21:35:03Z

@igorbt Did you end up merging anything? Your original fix worked for me @w01fgang

w01fgang · 2016-11-22T06:09:13Z

Actually I don't know why test fails on src/utils/withWidth.js, I didn't make any changes there.

oliviertassinari

Wow, that bug is quite embarrassing on iOS devices!

oliviertassinari · 2016-11-27T11:05:48Z

src/utils/device.spec.js

+
+describe('Device detection helper', () => {
+  before(() => {
+    global.window = {


Actually I don't know why test fails on src/utils/withWidth.js, I didn't make any changes there.

Yes, you have.

Ho, I forgot it, will fix it now.

oliviertassinari · 2016-11-27T11:08:47Z

src/utils/device.js

@@ -0,0 +1,69 @@
+/**
+* Returns an object with detected device and browser.


I'm not sure we need this all browser device detection code for iOS.
It could be as simple as:

// iOS detection from: http://stackoverflow.com/a/9039885/177710 if (/iPad|iPhone|iPod/.test(userAgent) && !window.MSStream) { return "iOS"; }

Yes, if we don't have any crossbrowser bugs in other components, it must be simple.

oliviertassinari · 2016-11-27T11:09:02Z

src/Popover/Popover.js

-     * layer, which will prevent clicks to the underlying
-     * elements, and trigger an `onRequestClose('clickAway')` call.
-     */
+    * If true, the popover will render on top of an invisible


We have some indent issue here.

Ok, I'll fix it.

w01fgang · 2016-11-27T14:19:46Z

@oliviertassinari please take a look.

oliviertassinari

We are almost there. Sorry for taking so long to review this PR.
It would also be great to rebase it on master and to squash down the commits (28).

oliviertassinari · 2016-11-27T14:23:24Z

src/Popover/Popover.js

@@ -240,7 +242,11 @@ class Popover extends Component {
    };

    a.right = rect.right || a.left + a.width;
-    a.bottom = rect.bottom || a.top + a.height;
+    if (/iPad|iPhone|iPod/.test(window.navigator.userAgent) && !window.MSStream) {


Still, I would move this logic into it's own module so we can test it 👼 .

Moved out to utils.

fixes mui#4499

oliviertassinari · 2016-12-10T19:27:06Z

It seems that the proposed fix (#4638) is only working fine in that context but breaking the others.

Following my comment, here is an example of broken context.

The Popover should be under the button, where more space is available.
I'm not sure we should push that PR path forward.

w01fgang · 2016-12-10T22:16:22Z

Yes, it might be, because the PR have been made for 0.14.x version or maybe 0.13.x.
In the 0.16.x it breaks some things, so I need some time to check it out.

oliviertassinari · 2016-12-10T23:45:27Z

Yes, it might be, because the PR have been made for 0.14.x version or maybe 0.13.x.

Sorry for taking so long to review that PR. Thanks for digging into that issue.

w01fgang · 2016-12-10T23:50:28Z

I checked it out. Now fix works only if input field in the focus.

oliviertassinari · 2016-12-15T23:09:18Z

src/Popover/Popover.js

@@ -240,7 +241,11 @@ class Popover extends Component {
    };

    a.right = rect.right || a.left + a.width;
-    a.bottom = rect.bottom || a.top + a.height;
+    if (isIOS() && document.activeElement.tagName === 'INPUT') {


Side node, with the next branch we would be using activeElement

oliviertassinari · 2016-12-15T23:12:05Z

I confirm, it's working great now. I'm gonna add some comments in the source code. Thanks 👍 !

stas-lesiuk · 2018-01-04T13:02:30Z

@w01fgang, on on 10 Dec 2016 you wrote:

Yes, it might be, because the PR have been made for 0.14.x version or maybe 0.13.x.
In the 0.16.x it breaks some things, so I need some time to check it out.

How does the situation looks like now? Is the fix merged to v0.16+ as well, or is it only on versions up to 0.14? Asking because I have a similar problem, not with the Autocomplete, but with an IconMenu popover on iOS devices. I'm using v0.16.7, but I've tested it on 0.20 and the issue is present there as well.

w01fgang · 2018-01-04T13:33:45Z

@stas-lesiuk on IOS before the version 11 CSS rule position: fixed counts from the top of the page, but from IOS 11 it works like it should work: from the top of the screen.
In this fix, we determine if the client's devise is IOS, but now we also should know the version to make this fix work.

stas-lesiuk · 2018-01-08T07:25:52Z

@w01fgang thanks for your help.
I have tried to copy Popover.js and IconMenu.js, and modify line:
if ((0, _iOSHelpers.isIOS)() && document.activeElement.tagName === 'INPUT') {
to
if ((0, () => true)() && document.activeElement.tagName === 'INPUT') {
and
if ((0, () => true)() && document.activeElement.tagName === 'INPUT') {

and unfortunately it seems that independently from the if statement result, there's still a problem with IconMenu Popover position on iOS devices. It looks like that's not the same issue like the initially addressed by you, however I wonder whether you may know the possible solution.
The difference seems to be that the issue appears only after zoom (screenshots attached), initially context menu position is right - it's next to menu, but the bigger zoom is the bigger distance becomes.

quorak · 2018-01-20T18:05:39Z

hey there. Do you have any plans on releasing this? The fix is not in the current v0.20 is it? thanks a lot

mbrookes · 2018-01-20T18:25:40Z

@quorak This PR was merged in Dec 2016!

quorak · 2018-01-20T18:33:56Z

yeah, it was merged on Dec 16th .
But last stable release v0.20 seems to be from Dec 4th , if I'm not mistaken.
https://github.com/mui-org/material-ui/tree/v0.20.0

muibot added the PR: Needs Review label Jul 6, 2016

oliviertassinari force-pushed the master branch from a711bfc to abc85d8 Compare July 26, 2016 20:12

oliviertassinari force-pushed the master branch from 53b828e to ea2538e Compare August 15, 2016 22:46

Taakn mentioned this pull request Oct 11, 2016

[AutoComplete] not working properly on mobile browsers #4499

Closed

muibot added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI PR: Needs Review and removed PR: Needs Review labels Nov 13, 2016

oliviertassinari removed the PR: Needs Review label Nov 27, 2016

oliviertassinari requested changes Nov 27, 2016

View reviewed changes

oliviertassinari added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 27, 2016

muibot added PR: Needs Review and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Nov 27, 2016

oliviertassinari reviewed Nov 27, 2016

View reviewed changes

w01fgang force-pushed the fixPopoverIOS branch from 826e0fe to 1838b34 Compare November 27, 2016 16:26

muibot added PR: out-of-date The pull request has merge conflicts and can't be merged PR: Needs Review and removed PR: Needs Review PR: out-of-date The pull request has merge conflicts and can't be merged labels Nov 27, 2016

w01fgang force-pushed the fixPopoverIOS branch from 5b91c3a to b3a9c8e Compare November 27, 2016 18:53

muibot added PR: out-of-date The pull request has merge conflicts and can't be merged PR: Needs Review and removed PR: Needs Review PR: out-of-date The pull request has merge conflicts and can't be merged labels Nov 27, 2016

Fix popover positioning on IOS

db6583b

fixes mui#4499

oliviertassinari added Safari component: Popover The React component. labels Nov 27, 2016

w01fgang added 3 commits December 3, 2016 19:17

move getOffsetTop to ios helper

bb81075

move getOffsetTop(el) inside if isIOS block

9ad0c51

fix: test for isIOS helper

9002ef7

oliviertassinari mentioned this pull request Dec 10, 2016

Wrong positioning of popover on IOS #3638

Closed

oliviertassinari removed the PR: Needs Review label Dec 10, 2016

use fix only in input field is focused

7b3b6ad

muibot added the PR: Needs Review label Dec 10, 2016

make test pass

82e9455

oliviertassinari approved these changes Dec 15, 2016

View reviewed changes

oliviertassinari added PR: Review Accepted and removed PR: Needs Review PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Dec 15, 2016

oliviertassinari merged commit ef384ac into mui:master Dec 15, 2016

oliviertassinari mentioned this pull request Dec 15, 2016

[Popover] Quick refactorisation #5798

Merged

3 tasks

w01fgang deleted the fixPopoverIOS branch December 18, 2016 13:25

stas-lesiuk mentioned this pull request Jan 11, 2018

IconMenu Popover position is wrong after page zoom on iOS deviced #9802

Closed

1 task

quorak mentioned this pull request Jan 22, 2018

Release latest master branch as new v0.21 #9992

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Popover] Fix bad positioning on IOS devices #4638

[Popover] Fix bad positioning on IOS devices #4638

w01fgang commented Jul 6, 2016 •

edited

Loading

igorbt commented Aug 12, 2016

igorbt commented Aug 12, 2016

w01fgang commented Aug 14, 2016 •

edited

Loading

Taakn commented Oct 11, 2016

w01fgang commented Nov 22, 2016

oliviertassinari left a comment

oliviertassinari Nov 27, 2016

w01fgang Nov 27, 2016

oliviertassinari Nov 27, 2016

w01fgang Nov 27, 2016

oliviertassinari Nov 27, 2016

w01fgang Nov 27, 2016

w01fgang commented Nov 27, 2016

oliviertassinari left a comment

oliviertassinari Nov 27, 2016

w01fgang Nov 27, 2016

oliviertassinari commented Dec 10, 2016

w01fgang commented Dec 10, 2016 •

edited

Loading

oliviertassinari commented Dec 10, 2016

w01fgang commented Dec 10, 2016

oliviertassinari Dec 15, 2016

oliviertassinari commented Dec 15, 2016

stas-lesiuk commented Jan 4, 2018

w01fgang commented Jan 4, 2018

stas-lesiuk commented Jan 8, 2018 •

edited

Loading

quorak commented Jan 20, 2018

mbrookes commented Jan 20, 2018

quorak commented Jan 20, 2018 •

edited

Loading

		@@ -0,0 +1,69 @@
		/**
		* Returns an object with detected device and browser.

[Popover] Fix bad positioning on IOS devices #4638

[Popover] Fix bad positioning on IOS devices #4638

Conversation

w01fgang commented Jul 6, 2016 • edited Loading

igorbt commented Aug 12, 2016

igorbt commented Aug 12, 2016

w01fgang commented Aug 14, 2016 • edited Loading

Taakn commented Oct 11, 2016

w01fgang commented Nov 22, 2016

oliviertassinari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

w01fgang commented Nov 27, 2016

oliviertassinari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented Dec 10, 2016

w01fgang commented Dec 10, 2016 • edited Loading

oliviertassinari commented Dec 10, 2016

w01fgang commented Dec 10, 2016

Choose a reason for hiding this comment

oliviertassinari commented Dec 15, 2016

stas-lesiuk commented Jan 4, 2018

w01fgang commented Jan 4, 2018

stas-lesiuk commented Jan 8, 2018 • edited Loading

quorak commented Jan 20, 2018

mbrookes commented Jan 20, 2018

quorak commented Jan 20, 2018 • edited Loading

w01fgang commented Jul 6, 2016 •

edited

Loading

w01fgang commented Aug 14, 2016 •

edited

Loading

w01fgang commented Dec 10, 2016 •

edited

Loading

stas-lesiuk commented Jan 8, 2018 •

edited

Loading

quorak commented Jan 20, 2018 •

edited

Loading