Skip to content
This repository was archived by the owner on Sep 5, 2024. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
25 changes: 19 additions & 6 deletions src/components/panel/panel.js
Original file line number Diff line number Diff line change
Expand Up @@ -1825,23 +1825,33 @@ MdPanelRef.prototype._configureEscapeToClose = function() {
*/
MdPanelRef.prototype._configureClickOutsideToClose = function() {
if (this.config['clickOutsideToClose']) {
var target = this.panelContainer;
var sourceElem;
var target = this.config['propagateContainerEvents'] ?
angular.element(document.body) :
this.panelContainer;
var sourceEl;

// Keep track of the element on which the mouse originally went down
// so that we can only close the backdrop when the 'click' started on it.
// A simple 'click' handler does not work,
// it sets the target object as the element the mouse went down on.
// A simple 'click' handler does not work, it sets the target object as the
// element the mouse went down on.
var mousedownHandler = function(ev) {
sourceElem = ev.target;
sourceEl = ev.target;
};

// We check if our original element and the target is the backdrop
// because if the original was the backdrop and the target was inside the
// panel we don't want to panel to close.
var self = this;
var mouseupHandler = function(ev) {
if (sourceElem === target[0] && ev.target === target[0]) {
if (self.config['propagateContainerEvents']) {

// We check if the sourceEl of the event is the panel element or one
// of it's children. If it is not, then close the panel.
if (sourceEl !== self.panelEl[0] && !self.panelEl[0].contains(sourceEl)) {
self.close();
}

} else if (sourceEl === target[0] && ev.target === target[0]) {
ev.stopPropagation();
ev.preventDefault();

Expand Down Expand Up @@ -2713,6 +2723,7 @@ MdPanelPosition.prototype._constrainToViewport = function(panelEl) {
}
};


/**
* Switches between 'start' and 'end'.
* @param {string} position Horizontal position of the panel
Expand Down Expand Up @@ -2902,6 +2913,7 @@ MdPanelAnimation.prototype.closeTo = function(closeTo) {
return this;
};


/**
* Specifies the duration of the animation in milliseconds.
* @param {number|{open: number, close: number}} duration
Expand All @@ -2927,6 +2939,7 @@ MdPanelAnimation.prototype.duration = function(duration) {
}
};


/**
* Returns the element and bounds for the animation target.
* @param {string|!Element|{top: number, left: number}} location
Expand Down
28 changes: 26 additions & 2 deletions src/components/panel/panel.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -544,6 +544,20 @@ describe('$mdPanel', function() {
expect(PANEL_EL).not.toExist();
});

it('should close when clickOutsideToClose set to true and ' +
'propagateContainerEvents is also set to true', function() {
var config = {
propagateContainerEvents: true,
clickOutsideToClose: true
};

openPanel(config);
Copy link
Member

Choose a reason for hiding this comment

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

It might make the test slightly more robust if you had a check that the PANEL_EL exists, right after opening it. This should avoid cases where the test will pass, if the openPanel function didn't open it up for some reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The open panel result is pretty well tested in the tests that proceed this one. You will also notice in the test right before this one, should close when clickOutsideToClose set to true does not expect the PANEL_EL to exist before it expects it to be closed. I went with that understanding to write this one.

Copy link
Member

Choose a reason for hiding this comment

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

While it's true that the opening logic is well tested, we should try to keep the tests as independent as possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

@crisbeto I go back and forth on this one. Because the open existence is tested earlier, we get a better test signal when opens fails because the failure is in the test that tests open. If we write it in every test, then all will fail and the messages don't point to the actual problem.

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't say it's necessary for every test, but it makes sense for this one since the test would fail if there isn't an open panel. This may be down to personal preference, though.


clickPanelContainer(getElement('body'));

expect(PANEL_EL).not.toExist();
});

it('should not close when escapeToClose set to false', function() {
openPanel();

Expand Down Expand Up @@ -3025,12 +3039,22 @@ describe('$mdPanel', function() {
attachedElements.push(element);
}

function clickPanelContainer() {
/**
* Returns the angular element associated with a CSS selector or element.
* @param el {string|!angular.JQLite|!Element}
* @returns {!angular.JQLite}
*/
function getElement(el) {
var queryResult = angular.isString(el) ? document.querySelector(el) : el;
return angular.element(queryResult);
}

function clickPanelContainer(container) {
if (!panelRef) {
return;
}

var container = panelRef.panelContainer;
container = container || panelRef.panelContainer;

container.triggerHandler({
type: 'mousedown',
Expand Down