-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Conversation
If `pagecontainer` is set to a different _wrapper_, page scrolls. `height: 99.9%` should be assigned to `ui-mobile-viewport` to avoid this issue. http://plnkr.co/edit/qMHTRFIBTSQkMX3Qbog8?p=preview
Fixes #7842 |
@Palestinian your change is cause a test failure. Either the test assumptions are no longer valid and needs to be updates or your change has broken something. |
@arschmitz Shall I close this and create a new PR? |
No just find out why the tests are failing and fix them though we need to discuss still if this is a change we want in #7842 |
@Palestinian Bump any progress on this? |
@arschmitz No, honestly not. I tried once and it failed. I didn't know what to do next. What do you recommend? |
It looks like tests related to silent scroll i would look at the failing test and see if you can see why your change would make it fail. Then determine if the test just needs to be updated or if your change really is breaking something. Running the tests locally is probably the best way to debug this. |
@arschmitz this test fails on Chrome desktop even on master. |
Chrome has a rounding error. If you |
OK, that rounding error seems to be fixed in a bleeding-edge version of Chrome. |
@gabrielschulhof but it does not fail on master on phantom so something is still wrong here |
If `pagecontainer` is set to a different _wrapper_, page scrolls. `height: 99.9%` should be assigned to `ui-mobile-viewport` to avoid this issue. http://plnkr.co/edit/qMHTRFIBTSQkMX3Qbog8?p=preview Fixes jquery-archivegh-7842 Closes jquery-archivegh-7841
@arschmitz Yeah, it was comparing the result of silentScroll() to 0, whereas it should've been $.mobile.defaultHomeScroll. My modified version of the test works on both master and this branch. |
@jaspermdegroot can you think of any potential problems with this? |
We should not land this PR, because it's not the right fix for the problem. The reason we set height for the The actual problem in @Palestinian his test page is the default browser style for
I am closing this PR. @Palestinian can you open a ticket for the issue, please? Do you want to create a new PR? |
@jaspermdegroot thank you! @arschmitz @gabrielschulhof Thank you both for sparing some time to look into this issue, I appreciate it :) |
If
pagecontainer
is set to a different wrapper, page scrolls.height: 99.9%
should be assigned toui-mobile-viewport
to avoid this issue. http://plnkr.co/edit/qMHTRFIBTSQkMX3Qbog8?p=preview