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

fix(all): remove all direct use of browser globals #10826

Closed
wants to merge 11 commits into from
Closed

fix(all): remove all direct use of browser globals #10826

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b71fafc
fix(all) use $window and $document over their respective globals.
Aug 2, 2017
6767ce7
remove $$mdMetaProvider
Aug 2, 2017
3335096
no global state in $$mdLayout
Aug 2, 2017
59e9f5f
support defining panel presets with a function
Aug 2, 2017
8a27c80
fix(all) use $window and $document over their respective globals.
Aug 2, 2017
b197cc9
localize themeConfig on a per app basis
Aug 2, 2017
608d4f0
fix $mdThemingProvider.enableBrowserColor queue
Aug 2, 2017
575f995
ensure all tests pass
Aug 2, 2017
d88aab9
fix newly introduced window.document calls
Jan 18, 2018
5cc67ed
fix undefined MutationObserver
graingert Aug 20, 2018
c1da2fb
fix theming service case
graingert Aug 20, 2018
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
7 changes: 4 additions & 3 deletions .eslintrc.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,9 +261,6 @@
"parserOptions": {
"ecmaVersion": 5
},
"env": {
"browser": true
},
"rules": {
"arrow-parens": "error",
"global-require": "error",
Expand All @@ -273,6 +270,9 @@
"no-process-env": "error",
"no-sync": "error"
},
"env": {
"node": false
},
"globals": {
"angular": true,
"CryptoJS": true,
Expand All @@ -292,6 +292,7 @@
"**/*.spec.js"
],
"env": {
"browser": true,
"jasmine": true
},
"rules": {
Expand Down
15 changes: 8 additions & 7 deletions docs/app/js/app.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,7 @@ function(SERVICES, COMPONENTS, DEMOS, PAGES, $location, $rootScope, $http, $wind
};
})

.directive('menuToggle', ['$mdUtil', '$animateCss', '$$rAF', function($mdUtil, $animateCss, $$rAF) {
.directive('menuToggle', ['$mdUtil', '$animateCss', '$$rAF', '$document', function($mdUtil, $animateCss, $$rAF, $document) {
return {
scope: {
section: '='
Expand Down Expand Up @@ -567,7 +567,7 @@ function(SERVICES, COMPONENTS, DEMOS, PAGES, $location, $rootScope, $http, $wind
var negativeOffset = activeHeight * 2;
var newScrollTop = activeOffset + parentScrollPosition - negativeOffset;

$mdUtil.animateScrollTo(document.querySelector('.docs-menu').parentNode, newScrollTop);
$mdUtil.animateScrollTo($document[0].querySelector('.docs-menu').parentNode, newScrollTop);
}
});
});
Expand All @@ -594,7 +594,8 @@ function(SERVICES, COMPONENTS, DEMOS, PAGES, $location, $rootScope, $http, $wind
'$location',
'$rootScope',
'$mdUtil',
function($scope, COMPONENTS, BUILDCONFIG, $mdSidenav, $timeout, $mdDialog, menu, $location, $rootScope, $mdUtil) {
'$document',
function($scope, COMPONENTS, BUILDCONFIG, $mdSidenav, $timeout, $mdDialog, menu, $location, $rootScope, $mdUtil, $document) {
var self = this;

$scope.COMPONENTS = COMPONENTS;
Expand Down Expand Up @@ -634,7 +635,7 @@ function($scope, COMPONENTS, BUILDCONFIG, $mdSidenav, $timeout, $mdDialog, menu,
this.autoFocusContent = false;


var mainContentArea = document.querySelector("[role='main']");
var mainContentArea = $document[0].querySelector("[role='main']");
var scrollContentEl = mainContentArea.querySelector('md-content[md-scroll-y]');


Expand Down Expand Up @@ -752,8 +753,8 @@ function($scope, $attrs, $location, $rootScope) {
};
}])

.controller('LayoutTipsCtrl', [
function() {
.controller('LayoutTipsCtrl', ['$document',
function($document) {
var self = this;

/*
Expand All @@ -762,7 +763,7 @@ function() {
self.toggleButtonText = "Hide";

self.toggleContentSize = function() {
var contentEl = angular.element(document.getElementById('toHide'));
var contentEl = angular.element($document[0].getElementById('toHide'));

contentEl.toggleClass("ng-hide");

Expand Down
5 changes: 3 additions & 2 deletions docs/app/js/demoInclude.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@ angular.module('docsApp').directive('demoInclude', [
'$q',
'$compile',
'$timeout',
function($q, $compile, $timeout) {
'$document',
function($q, $compile, $timeout, $document) {
return {
restrict: 'E',
link: postLink
Expand Down Expand Up @@ -71,7 +72,7 @@ function($q, $compile, $timeout) {
styles = styles.join('\n'); //join styles as one string

var styleElement = angular.element('<style>' + styles + '</style>');
document.body.appendChild(styleElement[0]);
$document[0].body.appendChild(styleElement[0]);

scope.$on('$destroy', function() {
styleElement.remove();
Expand Down
2 changes: 1 addition & 1 deletion src/components/autocomplete/demoBasicUsage/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
</md-item-template>
<md-not-found>
No states matching "{{ctrl.searchText}}" were found.
<a ng-click="ctrl.newState(ctrl.searchText)">Create a new one!</a>
<md-button ng-click="ctrl.newState(ctrl.searchText, $event)">Create a new one!</md-button>
Copy link
Contributor

Choose a reason for hiding this comment

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

It is hard to focus on a PR that seems to be trying to do a lot of changes at once, especially when it includes changes like this line that seems unrelated (changing <a> to <md-button>)

I don't think I'll be able to comfortably be able to review a change like this given that it tries to make too many changes at once. I'd prefer to see a PR that works through a predetermined set of steps that brings us to our goal (remove use of globals), e.g.

  • Replace uses of document with $document
  • Replace uses of console.log with md-dialog
  • Replace uses of window with $window
  • ...

If the PRs were separated as such, it would be much easier to parse and review. As it is now, it is too risky of a change to confidently LGTM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's just a doc change because they were using window.event.

I just swapped the a for a button because you can't have a a without an href, and I was editing that line anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can try running it through presubmits once it is reviewed (@Splaktar - do you want to look at this). If it passes then it's alright with me

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I'll review before adding the merge ready label and asking for it to be run through the presubmits.

Copy link
Member

Choose a reason for hiding this comment

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

The new behavior here is quite buggy.
screen shot 2018-09-04 at 12 34 07 pm

Let's completely remove the concept of a link or button in the md-not-found for this demo.

</md-not-found>
</md-autocomplete>
<br/>
Expand Down
12 changes: 9 additions & 3 deletions src/components/autocomplete/demoBasicUsage/script.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
.module('autocompleteDemo', ['ngMaterial'])
.controller('DemoCtrl', DemoCtrl);

function DemoCtrl ($timeout, $q, $log) {
function DemoCtrl ($timeout, $q, $log, $mdDialog) {
var self = this;

self.simulateQuery = false;
Expand All @@ -18,8 +18,14 @@

self.newState = newState;

function newState(state) {
alert("Sorry! You'll need to create a Constitution for " + state + " first!");
function newState(state, $event) {
$mdDialog.show(
$mdDialog
.alert()
.title('state creation failed.')
Copy link
Member

Choose a reason for hiding this comment

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

Let's completely remove the concept of a link or button in the md-not-found, and thus this newState() function for this demo.

.textContent("Sorry! You'll need to create a Constitution for " + state + " first!")
.targetEvent($event)
);
}

// ******************************
Expand Down
4 changes: 2 additions & 2 deletions src/components/autocomplete/demoInsideDialog/script.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,15 @@
.module('autocompleteDemoInsideDialog', ['ngMaterial'])
.controller('DemoCtrl', DemoCtrl);

function DemoCtrl($mdDialog) {
function DemoCtrl($mdDialog, $document) {
var self = this;

self.openDialog = function($event) {
$mdDialog.show({
controller: DialogCtrl,
controllerAs: 'ctrl',
templateUrl: 'dialog.tmpl.html',
parent: angular.element(document.body),
parent: $document.find('body'),
targetEvent: $event,
clickOutsideToClose:true
});
Expand Down
6 changes: 4 additions & 2 deletions src/components/autocomplete/js/autocompleteController.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ var ITEM_HEIGHT = 48,
INPUT_PADDING = 2; // Padding provided by `md-input-container`

function MdAutocompleteCtrl ($scope, $element, $mdUtil, $mdConstant, $mdTheming, $window,
$animate, $rootElement, $attrs, $q, $log, $mdLiveAnnouncer) {
$animate, $rootElement, $attrs, $q, $log, $mdLiveAnnouncer,
$document) {

// Internal Variables.
var ctrl = this,
Expand All @@ -22,7 +23,8 @@ function MdAutocompleteCtrl ($scope, $element, $mdUtil, $mdConstant, $mdTheming,
fetchesInProgress = 0,
enableWrapScroll = null,
inputModelCtrl = null,
debouncedOnResize = $mdUtil.debounce(onWindowResize);
debouncedOnResize = $mdUtil.debounce(onWindowResize),
document = $document[0];

// Public Exported Variables with handlers
defineProperty('hidden', handleHiddenChange, true);
Expand Down
5 changes: 3 additions & 2 deletions src/components/autocomplete/js/highlightController.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,11 @@ angular
.module('material.components.autocomplete')
.controller('MdHighlightCtrl', MdHighlightCtrl);

function MdHighlightCtrl ($scope, $element, $attrs) {
function MdHighlightCtrl ($scope, $element, $attrs, $document) {
this.$scope = $scope;
this.$element = $element;
this.$attrs = $attrs;
this.$document = $document;

// Cache the Regex to avoid rebuilding each time.
this.regex = null;
Expand Down Expand Up @@ -64,7 +65,7 @@ MdHighlightCtrl.prototype.applyRegex = function(text) {

this.$element.append(tokenEl);
} else {
this.$element.append(document.createTextNode(token));
this.$element.append(this.$document[0].createTextNode(token));
}

}.bind(this));
Expand Down
2 changes: 1 addition & 1 deletion src/components/chips/demoContactChips/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<div ng-controller="ContactChipDemoCtrl as ctrl" layout="column" ng-cloak>

<md-content class="md-padding autocomplete" layout="column">
<md-content class="md-padding autocomplete" layout="column" ng-if="ctrl.allContacts">
<md-contact-chips
ng-model="ctrl.contacts"
ng-change="ctrl.onModelChange(ctrl.contacts)"
Expand Down
54 changes: 46 additions & 8 deletions src/components/chips/demoContactChips/script.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,23 +2,61 @@
'use strict';

// If we do not have CryptoJS defined; import it
if (typeof CryptoJS === 'undefined') {
var cryptoSrc = 'https://cdnjs.cloudflare.com/ajax/libs/crypto-js/3.1.2/rollups/md5.js';
var scriptTag = document.createElement('script');
scriptTag.setAttribute('src', cryptoSrc);
document.body.appendChild(scriptTag);
function cryptoJsLoader() {
var prom;
var src = '//cdnjs.cloudflare.com/ajax/libs/crypto-js/3.1.2/rollups/md5.js';
function loadCryptoJSInner($document, $q, $window) {
var document = $document[0];

function resolveCrypto() {
if (typeof $window.CryptoJS !== 'undefined') {
return $q.resolve($window.CryptoJS);
}

return $q.reject(new Error("Can't resolve CryptoJS"));
}

function loadCrypto() {
return $q(function (resolve, reject) {
var s;
s = document.createElement('script');
s.src = src;
s.onload = resolve;
s.onerror = reject;
document.head.appendChild(s);
});
}

return $q.resolve(undefined)
.then(resolveCrypto)
.catch(loadCrypto)
.then(resolveCrypto);
}

return function($document, $q, $window) {
if (prom) {
Copy link
Member

Choose a reason for hiding this comment

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

What does prom stand for? Perhaps this can be renamed to be more clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

promise

return prom;
}

prom = loadCryptoJSInner($document, $q, $window);
};
}

var loader = cryptoJsLoader();

angular
.module('contactChipsDemo', ['ngMaterial'])
.controller('ContactChipDemoCtrl', DemoCtrl);

function DemoCtrl ($q, $timeout, $log, $mdConstant) {
function DemoCtrl ($q, $timeout, $log, $mdConstant, $document, $window) {
var self = this;
var pendingSearch, cancelSearch = angular.noop;
var lastSearch;

self.allContacts = loadContacts();
loader($document, $q, $window).then(function (CryptoJS) {
Copy link
Member

Choose a reason for hiding this comment

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

This throws:

TypeError: Cannot read property 'then' of undefined
    at DemoCtrl (docs-demo-scripts.js:589)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Urgh, probably a missing return

self.allContacts = loadContacts(CryptoJS);
});

self.contacts = [self.allContacts[0]];
self.asyncContacts = [];
self.keys = [$mdConstant.KEY_CODE.COMMA];
Expand Down Expand Up @@ -89,7 +127,7 @@
$log.log('The model has changed to ' + JSON.stringify(newModel) + '.');
}

function loadContacts() {
function loadContacts(CryptoJS) {
var contacts = [
'Marina Augustine',
'Oddr Sarno',
Expand Down
9 changes: 8 additions & 1 deletion src/components/chips/js/chipController.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ angular
* @param $mdUtil
* @constructor
*/
function MdChipCtrl ($scope, $element, $mdConstant, $timeout, $mdUtil) {
function MdChipCtrl ($scope, $element, $mdConstant, $timeout, $mdUtil, $document) {
/**
* @type {$scope}
*/
Expand All @@ -39,6 +39,11 @@ function MdChipCtrl ($scope, $element, $mdConstant, $timeout, $mdUtil) {
*/
this.$mdUtil = $mdUtil;

/**
* @type {$document}
*/
this.$document = $document;

/**
* @type {boolean}
*/
Expand Down Expand Up @@ -140,6 +145,8 @@ MdChipCtrl.prototype.goOutOfEditMode = function() {
* @param {Element} node
*/
MdChipCtrl.prototype.selectNodeContents = function(node) {
var document = this.$document[0];
var window = this.$window;
var range, selection;
if (document.body.createTextRange) {
range = document.body.createTextRange();
Expand Down
11 changes: 7 additions & 4 deletions src/components/datepicker/js/calendar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@
* @ngInject @constructor
*/
function CalendarCtrl($element, $scope, $$mdDateUtil, $mdUtil,
$mdConstant, $mdTheming, $$rAF, $attrs, $mdDateLocale) {
$mdConstant, $mdTheming, $$rAF, $attrs, $mdDateLocale, $document) {

$mdTheming($element);

Expand All @@ -128,6 +128,9 @@
/** @final */
this.$mdDateLocale = $mdDateLocale;

/** @final */
this.$document = $document;

/** @final {Date} */
this.today = this.dateUtil.createDateAtMidnight();

Expand Down Expand Up @@ -213,7 +216,7 @@

var handleKeyElement;
if ($element.parent().hasClass('md-datepicker-calendar')) {
handleKeyElement = angular.element(document.body);
handleKeyElement = angular.element($document[0].body);
} else {
handleKeyElement = $element;
}
Expand Down Expand Up @@ -341,7 +344,7 @@
}

var cellId = this.getDateId(date, this.currentView);
var cell = document.getElementById(cellId);
var cell = this.$document[0].getElementById(cellId);
if (cell) {
cell.classList.add(this.FOCUSED_DATE_CLASS);
cell.focus();
Expand Down Expand Up @@ -372,7 +375,7 @@

// Apply the select class to the new selected date if it is set.
if (date) {
var dateCell = document.getElementById(this.getDateId(date, this.currentView));
var dateCell = this.$document[0].getElementById(this.getDateId(date, this.currentView));
if (dateCell) {
dateCell.classList.add(selectedDateClass);
dateCell.setAttribute('aria-selected', 'true');
Expand Down
Loading