Skip to content
This repository was archived by the owner on May 29, 2019. It is now read-only.

fix(modal): prevent body content shifting when vertical scrollbar #4782

Closed
wants to merge 3 commits into from
Closed

fix(modal): prevent body content shifting when vertical scrollbar #4782

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d8daf3a
fix(modal): prevent body content shifting when vertical scrollbar
kunal999 Oct 29, 2015
4c13747
fix(modal): remove unnecessary .eq(0) from $document.find('body')
kunal999 Oct 30, 2015
b9e89e4
fix(modal): clean up paddings for modal window
kunal999 Oct 30, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions karma.conf.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ module.exports = function(config) {
'node_modules/angular/angular.js',
'node_modules/angular-mocks/angular-mocks.js',
'node_modules/angular-sanitize/angular-sanitize.js',
'node_modules/bootstrap/dist/css/bootstrap.css',
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this affect the tests?

Copy link
Author

Choose a reason for hiding this comment

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

Yes. Without bootstrap.css, the tests always pass. The tests rely on the modal related classes in bootstrap css which remove the scroll bar from the body when the modal is opened.

'misc/test-lib/helpers.js',
'src/**/*.js',
'template/**/*.js'
Expand Down
3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
"version": "1.0.0-SNAPSHOT",
"homepage": "http://angular-ui.github.io/bootstrap/",
"dependencies": {},
"scripts":{
"scripts": {
"test": "grunt"
},
"repository": {
Expand All @@ -15,6 +15,7 @@
"angular": "^1.4.4",
"angular-mocks": "^1.4.4",
"angular-sanitize": "^1.4.4",
"bootstrap": "^3.3.5",
"grunt": "^0.4.5",
"grunt-contrib-concat": "^0.5.1",
"grunt-contrib-copy": "^0.8.0",
Expand Down
84 changes: 81 additions & 3 deletions src/modal/modal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,31 @@ angular.module('ui.bootstrap.modal', ['ui.bootstrap.stackedMap'])
}
}])

/**
* A helper, for measuring and maintaining scrollbar properties - used by modalWindow and $modalStack
*/
.factory('$$scrollbarHelper', function () {
var scrollbarWidth;

return {
measureScrollbar: measureScrollbar,
scrollbarWidth: scrollbarWidth
};

// from modal.js of bootstrap
function measureScrollbar () { // thx walsh
var scrollDiv = document.createElement('div');
scrollDiv.className = 'modal-scrollbar-measure';
document.body.appendChild(scrollDiv);
var width = scrollDiv.offsetWidth - scrollDiv.clientWidth;
document.body.removeChild(scrollDiv);
this.scrollbarWidth = width;
}
})

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to see this in the $position service as scrollbarWidth(), we might need to use it for auto/smart positioning. Would be nice to cache the result to avoid manipulating the DOM every time we need this value as it should not change.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree - we have a related issue in #4317.

Copy link
Contributor

Choose a reason for hiding this comment

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

$position service now has a scrollbarWidth function.

.directive('uibModalWindow', [
'$uibModalStack', '$q', '$animate', '$injector',
function($modalStack , $q , $animate, $injector) {
'$uibModalStack', '$q', '$animate', '$injector', '$$scrollbarHelper',
function($modalStack , $q , $animate, $injector, $$scrollbarHelper) {
var $animateCss = null;

if ($injector.has('$animateCss')) {
Expand Down Expand Up @@ -202,6 +224,34 @@ angular.module('ui.bootstrap.modal', ['ui.bootstrap.stackedMap'])
var modal = $modalStack.getTop();
if (modal) {
$modalStack.modalRendered(modal.key);

// get topmost modal object
var bodyIsOverflowing;
var modalIsOverflowing;

// only when modal is attached to body
if(modal.value.appendTo === 'body'){

// check bodyOverflowing property that was set when opening modal
if(modal.value.modalDomEl.bodyOverflowing){
bodyIsOverflowing = true;
}

// start - from adjustDialog method of modal.js of bootstrap
// check if modal is overflowing
modalIsOverflowing = element[0].scrollHeight > document.documentElement.clientHeight;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should use $document[0].clientHeight


if(!$$scrollbarHelper.scrollbarWidth){
$$scrollbarHelper.measureScrollbar();
}

if(modalIsOverflowing && !bodyIsOverflowing){
element.css('padding-left', $$scrollbarHelper.scrollbarWidth + 'px');
}else if(bodyIsOverflowing && !modalIsOverflowing){
element.css('padding-right', $$scrollbarHelper.scrollbarWidth + 'px');
}
// end - from adjustDialog method of modal.js of bootstrap
}
}
});
}
Expand Down Expand Up @@ -235,11 +285,13 @@ angular.module('ui.bootstrap.modal', ['ui.bootstrap.stackedMap'])
'$injector',
'$$multiMap',
'$$stackedMap',
'$$scrollbarHelper',
function($animate , $timeout , $document , $compile , $rootScope ,
$q,
$injector,
$$multiMap,
$$stackedMap) {
$$stackedMap,
$$scrollbarHelper) {
var $animateCss = null;

if ($injector.has('$animateCss')) {
Expand Down Expand Up @@ -291,6 +343,13 @@ angular.module('ui.bootstrap.modal', ['ui.bootstrap.stackedMap'])
openedClasses.remove(modalBodyClass, modalInstance);
appendToElement.toggleClass(modalBodyClass, openedClasses.hasKey(modalBodyClass));
toggleTopWindowClass(true);

// reset any right padding set to body when opening the modal
// make sure no other modal is open before resetting
if(modalWindow.appendTo === 'body' && openedWindows.length() === 0){
var body = $document.find('body');
body.css('padding-right', modalWindow.modalDomEl.originalBodyRightPadding);
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 way too opinionated - this will make it near impossible for a user to override this, and is not a good solution.

}
});
checkRemoveBackdrop();

Expand Down Expand Up @@ -463,6 +522,25 @@ angular.module('ui.bootstrap.modal', ['ui.bootstrap.stackedMap'])
appendToElement.append(modalDomEl);
appendToElement.addClass(modalBodyClass);

// if modal is attached to body, then check if body is overflowing and set bodyOverflowing property on modal object and set right padding equal to scrollbarWidth
if (modal.appendTo === 'body' && document.body.scrollHeight > document.documentElement.clientHeight){
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto


// set bodyOverflowing property
modalDomEl.bodyOverflowing = true;

// set right padding to body
if(openedWindows.length() === 1){
if(!$$scrollbarHelper.scrollbarWidth){
$$scrollbarHelper.measureScrollbar();
}

var body = $document.find('body');
// set originalBodyRightPadding property to reset it when the last modal closes
modalDomEl.originalBodyRightPadding = body.css('padding-right');
Copy link
Contributor

Choose a reason for hiding this comment

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

That is an unnecessarily verbose key name - please choose a better name, such as bodyRightPadding.

body.css('padding-right', ($$scrollbarHelper.scrollbarWidth + parseInt(modalDomEl.originalBodyRightPadding || '0'))+'px');
}
}

$modalStack.clearFocusListCache();
};

Expand Down
52 changes: 52 additions & 0 deletions src/modal/test/modal.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1225,4 +1225,56 @@ describe('$uibModal', function () {
expect(called).toBeTruthy();
});
});

describe('body content when vertical scrollbar present', function() {
it('should not shift document elements - no body right padding specified', function() {

var largeBlockWithButton = '<div class="container"><button class="btn btn-default" id="clickMeButton" ng-click="open()">Open me!</button><div style="height: 3000px;"><p>&nbsp;</p><p>Block with large height to force vertical scroll</p></div></div></div>';
var element = angular.element(largeBlockWithButton);
angular.element(document.body).append(element);

var clickMeButton = $document.find('div #clickMeButton');
var buttonLeftBeforeModalOpen = clickMeButton.offset().left;

var modal = open({template: '<div>Content</div>'});
expect($document).toHaveModalsOpen(1);
var buttonLeftAfterModalOpen = clickMeButton.offset().left;
expect(buttonLeftAfterModalOpen).toBe(buttonLeftBeforeModalOpen);

dismiss(modal, 'closing in test');

expect($document).toHaveModalsOpen(0);
var buttonLeftAfterModalClose = clickMeButton.offset().left;
expect(buttonLeftAfterModalClose).toBe(buttonLeftBeforeModalOpen);

element.remove();
});

it('should not shift document elements - body has right padding specified', function() {

// add right body padding
var body = $document.find('body').eq(0);
body.css('padding-right', '50px');

var largeBlockWithButton = '<div class="container"><button class="btn btn-default" id="clickMeButton" ng-click="open()">Open me!</button><div style="height: 3000px;"><p>&nbsp;</p><p>Block with large height to force vertical scroll</p></div></div></div>';
var element = angular.element(largeBlockWithButton);
angular.element(document.body).append(element);

var clickMeButton = $document.find('div #clickMeButton');
var buttonLeftBeforeModalOpen = clickMeButton.offset().left;

var modal = open({template: '<div>Content</div>'});
expect($document).toHaveModalsOpen(1);
var buttonLeftAfterModalOpen = clickMeButton.offset().left;
expect(buttonLeftAfterModalOpen).toBe(buttonLeftBeforeModalOpen);

dismiss(modal, 'closing in test');

expect($document).toHaveModalsOpen(0);
var buttonLeftAfterModalClose = clickMeButton.offset().left;
expect(buttonLeftAfterModalClose).toBe(buttonLeftBeforeModalOpen);

element.remove();
});
});
});