Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge styles order #4664

Merged
merged 3 commits into from
Apr 25, 2018
Merged

Merge styles order #4664

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
492279f
Updating how style elements are injected (to preserve the location in…
dzearing Apr 24, 2018
342b02a
adding change file
dzearing Apr 24, 2018
9f2885d
Adding nit fixes.
dzearing Apr 25, 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
11 changes: 11 additions & 0 deletions common/changes/@uifabric/merge-styles/merge-styles-order_2018-04-24-16-59.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"changes": [
{
"packageName": "@uifabric/merge-styles",
"comment": "merge-styles: `style` elements which are created on the fly now \"bunch\" together, avoiding unpredictability in specificity.",
"type": "patch"
}
],
"packageName": "@uifabric/merge-styles",
"email": "[email protected]"
}
72 changes: 39 additions & 33 deletions packages/merge-styles/src/Stylesheet.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ let _stylesheet: Stylesheet;
* @public
*/
export class Stylesheet {
private _lastStyleElement?: HTMLStyleElement;
private _styleElement?: HTMLStyleElement;
private _rules: string[] = [];
private _config: IStyleSheetConfig;
Expand Down Expand Up @@ -158,28 +159,29 @@ export class Stylesheet {
public insertRule(
rule: string
): void {
const element = this._getElement();
const injectionMode = element ? this._config.injectionMode : InjectionMode.none;

switch (injectionMode) {
case InjectionMode.insertNode:
const { sheet } = element!;

try {
// tslint:disable-next-line:no-any
(sheet as any).insertRule(rule, (sheet as any).cssRules.length);
} catch (e) {
/* no-op on errors */
}
break;

case InjectionMode.appendChild:
_createStyleElement(rule);
break;

default:
this._rules.push(rule);
break;
const { injectionMode } = this._config;
const element = injectionMode !== InjectionMode.none ? this._getStyleElement() : undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this nesting of a switch inside an if-else is not very readable to me. Any reason not to just have a switch covering all states?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was because the "else" statement applies to all states. If there is no element created, because say, document is not available, cache the rules in the array for later use.


if (element) {
switch (this._config.injectionMode) {
case InjectionMode.insertNode:
const { sheet } = element!;

try {
(sheet as CSSStyleSheet).insertRule(rule, (sheet as CSSStyleSheet).cssRules.length);
} catch (e) {
// The browser will throw exceptions on unsupported rules (such as a moz prefix in webkit.)
// We need to swallow the exceptions for this scenario, otherwise we'd need to filter
// which could be slower and bulkier.
}
break;

case InjectionMode.appendChild:
element.appendChild(document.createTextNode(rule));
break;
}
} else {
this._rules.push(rule);
}

if (this._config.onInsertRule) {
Expand Down Expand Up @@ -212,9 +214,9 @@ export class Stylesheet {
this._keyToClassName = {};
}

private _getElement(): HTMLStyleElement | undefined {
private _getStyleElement(): HTMLStyleElement | undefined {
if (!this._styleElement && typeof document !== 'undefined') {
this._styleElement = _createStyleElement();
this._styleElement = this._createStyleElement();

// Reset the style element on the next frame.
window.requestAnimationFrame(() => {
Expand All @@ -223,17 +225,21 @@ export class Stylesheet {
}
return this._styleElement;
}
}

function _createStyleElement(content?: string): HTMLStyleElement {
const styleElement = document.createElement('style');
private _createStyleElement(): HTMLStyleElement {
const styleElement = document.createElement('style');

styleElement.setAttribute('data-merge-styles', 'true');
styleElement.type = 'text/css';

if (this._lastStyleElement && this._lastStyleElement.nextElementSibling) {
document.head.insertBefore(styleElement, this._lastStyleElement.nextElementSibling);
} else {
document.head.appendChild(styleElement);
}
this._lastStyleElement = styleElement;

styleElement.setAttribute('data-merge-styles', 'true');
styleElement.type = 'text/css';
if (content) {
styleElement.appendChild(document.createTextNode(content));
return styleElement;
}
document.head.appendChild(styleElement);

return styleElement;
}