Fix reveal enableScroll calculation of scrollTop #10786
Conversation
The target scrollTop must be calculated BEFORE the is-reveal-open class gets removed. Otherwise iOS is not able to determine the correct top value but uses "auto". To add a scrollTop param to the disableScroll is not necessary but due to consistency I've done it here the same way.
| @@ -406,7 +410,7 @@ class Reveal extends Plugin { | |||
|
|
|||
| _this.$element.attr('aria-hidden', true); | |||
|
|
|||
| _this._enableScroll(); | |||
| _this._enableScroll(scrollTop); | |||
There was a problem hiding this comment.
Could we move enableScroll before removing is-reveal-open, instead of passing the scrollTop ? I think this would be cleaner, but I don't know the codebase enough to be sure this would not cause others bugs.
There was a problem hiding this comment.
I first tried this way and it fixed the iOS issue (original scrollTop restored after having closed the reveal).
But unfortunately non-iOS / desktop has got the issue then!
The reason is that the class is-reveal-open adds fixed position to the html and thus the scrollTop restoration don't work before removing that class.
My solution was the only way I've found to get rid of the issue for all cases.
There was a problem hiding this comment.
@ncoden what do you think?
Would be great to see this iOS issue be gone in my projects (without adjusting the sources temporary)
There was a problem hiding this comment.
Sorry for the late reply. I'll check if it can be cleaner, then merge this.
… in Reveal Try to keep a more or less enclosed behavior by using defaults properties. The functions behaviors can be changed when needed.
|
@SassNinja I think it is clean enough now. You you check if this works in your project ? I have some trouble to reproduce the issue. |
|
Yup @ncoden it's still working for me. |
…roll for v6.5.0 7e67525 Fix reveal enableScroll calculation of scrollTop 4f27733 refactor: add defaults to `_enableScroll`/`_disableScroll` properties in Reveal 808fc3b style: document the foundation#10786 bug in Reveal Co-Authored-By: Nicolas Coden <nicolas@ncoden.fr> Signed-off-by: Nicolas Coden <nicolas@ncoden.fr>
This PR fixes an issue with the scrollTop calculation of the
_enableScrollmethod in the reveal component.Precisely the issue occurred for me on iOS / iPhone.
After some debugging I found out the line
parseInt($("html").css("top"))doesn't work as expected in this scenario because the class.is-reveal-opengot removed before.Therefore the html element got
position:staticagain an returnsautoas top-value on iOS.To fix this I've moved the scrollTop calculation from the disabledScroll/enableScroll methods to the place they get called and pipe them through as param.
So now the top value gets calculated before
.is-reveal-openis removed.@IamManchanda @kball would you review?