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

fix(util): disableScrollAround should not remove disabled scroll mask #9547

Merged

Conversation

devversion
Copy link
Member

  • When the scroll mask is disabled per the new options which were recently added, the specified element will be used as scroll mask (under the hood)
    If restoring scroll, the scroll mask will be removed from the DOM (even the disabled one) - This is invalid.

This causes an issue when restoring the scroll, while the element is still showing up (e.g dialog with z-index)

@ThomasBurleson Also I need to correct my self, the issue within the $mdPanel is not reproducable (but present), because the element will be anyways detached by the panel.

Part of #9256

@devversion devversion added the needs: review This PR is waiting on review from the team label Sep 7, 2016
@devversion devversion added the needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved label Sep 10, 2016

return $mdUtil.disableScrollAround._enableScrolling = function() {
return $mdUtil.disableScrollAround._restoreScroll = function() {
if (!--$mdUtil.disableScrollAround._count) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems dangerous. How about

if ( --$mdUtil.disableScrollAround._count < 1 ) {
....
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah will change while I'm at it.

@ThomasBurleson ThomasBurleson added in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved and removed needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved needs: review This PR is waiting on review from the team labels Sep 10, 2016
@ThomasBurleson ThomasBurleson added this to the 1.1.2 milestone Sep 10, 2016
* When the scroll mask is disabled per the new options which were recently added, the specified element will be used as scroll mask (under the hood)
  If restoring scroll, the scroll mask will be removed from the DOM (even the disabled one) - This is invalid.

This causes an issue when restoring the scroll, while the element is still showing up (e.g dialog with z-index)
@devversion devversion force-pushed the fix/util-not-remove-if-scrollmask branch from 1f78786 to cbe27a9 Compare September 10, 2016 11:38
@devversion devversion added needs: review This PR is waiting on review from the team and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved labels Sep 10, 2016
@devversion
Copy link
Member Author

@ThomasBurleson Addressed the comments you added.

@ThomasBurleson ThomasBurleson added needs: presubmit and removed needs: review This PR is waiting on review from the team labels Sep 10, 2016
@jelbourn jelbourn merged commit bbb9ec5 into angular:master Sep 21, 2016
@devversion devversion deleted the fix/util-not-remove-if-scrollmask branch September 21, 2016 15:29
crisbeto added a commit to crisbeto/material that referenced this pull request Sep 24, 2016
Frank3K pushed a commit to Frank3K/material that referenced this pull request Oct 8, 2016
…angular#9547)

* When the scroll mask is disabled per the new options which were recently added, the specified element will be used as scroll mask (under the hood)
  If restoring scroll, the scroll mask will be removed from the DOM (even the disabled one) - This is invalid.

This causes an issue when restoring the scroll, while the element is still showing up (e.g dialog with z-index)
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