jenkins-bot has submitted this change and it was merged. Change subject: Make CSS transplantation in OO.ui.Frame actually wait for CSS ......................................................................
Make CSS transplantation in OO.ui.Frame actually wait for CSS Before e9ca44c86, there was code in place that waited for the CSS to load and fired 'initialize' asynchronously. It was initially removed because it was deemed unnecessary, but with later changes it has become necessary again. The original approach also didn't work in Chrome. This commit completely rewrites the CSS transplantation code to correctly wait for CSS to be loaded before emitting 'initialize', and to unbreak image URL references by no longer inlining same-origin styles. The new code transplants <style> tags directly, while <link> tags are rewritten as <style>@import url(..); #foo { ... }</style>. We then insert a <div id="foo"> into the document and wait for it to get styled, which will only happen after the @import has finished. For the styling of #foo we use font-family, because it allows us to set arbitrary string values. Additionally, we have to use visiblity: hidden; to hide windows initially, because display: none; causes the iframe to not load in Firefox. We reapply display: none; after the iframe initializes. Bug: 56630 Change-Id: I76688a5ad7bd144c5e46064119f607060e76f0d9 --- M src/OO.ui.Frame.js M src/OO.ui.Window.js M src/styles/OO.ui.Window.css 3 files changed, 104 insertions(+), 61 deletions(-) Approvals: Krinkle: Looks good to me, approved jenkins-bot: Verified diff --git a/src/OO.ui.Frame.js b/src/OO.ui.Frame.js index 3c6fb32..6a761fe 100644 --- a/src/OO.ui.Frame.js +++ b/src/OO.ui.Frame.js @@ -42,6 +42,76 @@ * @event initialize */ +/* Static Methods */ + +/** + * Transplant the CSS styles from as parent document to a frame's document. + * + * This loops over the style sheets in the parent document, and copies their nodes to the + * frame's document. It then polls the document to see when all styles have loaded, and once they + * have, invokes the callback. + * + * For details of how we arrived at the strategy used in this function, see #load. + * + * @static + * @method + * @inheritable + * @param {HTMLDocument} parentDoc Document to transplant styles from + * @param {HTMLDocument} frameDoc Document to transplant styles to + * @param {Function} [callback] Callback to execute once styles have loaded + */ +OO.ui.Frame.static.transplantStyles = function ( parentDoc, frameDoc, callback ) { + var i, numSheets, styleNode, newNode, timeout, pollNodeId, $pendingPollNodes, + $pollNodes = $( [] ), + // Fake font-family value + fontFamily = 'oo-ui-frame-transplantStyles-loaded'; + + for ( i = 0, numSheets = parentDoc.styleSheets.length; i < numSheets; i++ ) { + styleNode = parentDoc.styleSheets[i].ownerNode; + if ( callback && styleNode.nodeName.toLowerCase() === 'link' ) { + // External stylesheet + // Create a node with a unique ID that we're going to monitor to see when the CSS + // has loaded + pollNodeId = 'oo-ui-frame-transplantStyles-loaded-' + i; + $pollNodes = $pollNodes.add( $( '<div>', frameDoc ) + .attr( 'id', pollNodeId ) + .appendTo( frameDoc.body ) + ); + + // Add <style>@import url(...); #pollNodeId { font-family: ... }</style> + // The font-family rule will only take effect once the @import finishes + newNode = frameDoc.createElement( 'style' ); + newNode.textContent = '@import url(' + styleNode.href + ');\n' + + '#' + pollNodeId + ' { font-family: ' + fontFamily + '; }'; + } else { + // Not an external stylesheet, or no polling required; just copy the node over + newNode = frameDoc.importNode( styleNode, true ); + } + frameDoc.head.appendChild( newNode ); + } + + if ( callback ) { + // Poll every 100ms until all external stylesheets have loaded + $pendingPollNodes = $pollNodes; + timeout = setTimeout( function pollExternalStylesheets() { + while ( + $pendingPollNodes.length > 0 && + $pendingPollNodes.eq( 0 ).css( 'font-family' ) === fontFamily + ) { + $pendingPollNodes = $pendingPollNodes.slice( 1 ); + } + + if ( $pendingPollNodes.length === 0 ) { + // We're done! + $pollNodes.remove(); + callback(); + } else { + timeout = setTimeout( pollExternalStylesheets, 100 ); + } + }, 100 ); + } +}; + /* Methods */ /** @@ -55,20 +125,32 @@ * iframe is triggered when you call close, and there's no further load event to indicate that * everything is actually loaded. * - * By dynamically adding stylesheet links, we can detect when each link is loaded by testing if we - * have access to each of their `sheet.cssRules` properties. Every 10ms we poll to see if we have - * access to the style's `sheet.cssRules` property yet. + * In Chrome, stylesheets don't show up in document.styleSheets until they have loaded, so we could + * just poll that array and wait for it to have the right length. However, in Firefox, stylesheets + * are added to document.styleSheets immediately, and the only way you can determine whether they've + * loaded is to attempt to access .cssRules and wait for that to stop throwing an exception. But + * cross-domain stylesheets never allow .cssRules to be accessed even after they have loaded. * - * However, because of security issues, we never have such access if the stylesheet came from a - * different site. Thus, we are left with linking to the stylesheets through a style element with - * multiple `@import` statements - which ends up being simpler anyway. Since we created that style, - * we always have access, and its contents are only available when everything is done loading. + * The workaround is to change all `<link href="...">` tags to `<style>@import url(...)</style>` tags. + * Because `@import` is blocking, Chrome won't add the stylesheet to document.styleSheets until + * the `@import` has finished, and Firefox won't allow .cssRules to be accessed until the `@import` + * has finished. And because the contents of the `<style>` tag are from the same origin, accessing + * .cssRules is allowed. + * + * However, now that we control the styles we're injecting, we might as well do away with + * browser-specific polling hacks like document.styleSheets and .cssRules, and instead inject + * `<style>@import url(...); #foo { font-family: someValue; }</style>`, then create `<div id="foo">` + * and wait for its font-family to change to someValue. Because `@import` is blocking, the font-family + * rule is not applied until after the `@import` finishes. + * + * All this stylesheet injection and polling magic is in #transplantStyles. * * @fires initialize */ OO.ui.Frame.prototype.load = function () { var win = this.$element.prop( 'contentWindow' ), - doc = win.document; + doc = win.document, + frame = this; // Figure out directionality: this.dir = this.$element.closest( '[dir]' ).prop( 'dir' ) || 'ltr'; @@ -90,54 +172,12 @@ this.$content = this.$( '.oo-ui-frame-content' ); this.$document = this.$( doc ); - this.transplantStyles(); - this.initialized = true; - this.emit( 'initialize' ); -}; - -/** - * Transplant the CSS styles from the frame's parent document to the frame's document. - * - * This loops over the style sheets in the parent document, and copies their tags to the - * frame's document. `<link>` tags pointing to same-origin style sheets are inlined as `<style>` tags; - * `<link>` tags pointing to foreign URLs and `<style>` tags are copied verbatim. - */ -OO.ui.Frame.prototype.transplantStyles = function () { - var i, ilen, j, jlen, sheet, rules, cssText, styleNode, - newDoc = this.$document[0], - parentDoc = this.getElementDocument(); - for ( i = 0, ilen = parentDoc.styleSheets.length; i < ilen; i++ ) { - sheet = parentDoc.styleSheets[i]; - styleNode = undefined; - try { - rules = sheet.cssRules; - } catch ( e ) { } - if ( sheet.ownerNode.nodeName.toLowerCase() === 'link' && rules ) { - // This is a <link> tag pointing to a same-origin style sheet. Rebuild it as a - // <style> tag. This needs to be in a try-catch because it sometimes fails in Firefox. - try { - cssText = ''; - for ( j = 0, jlen = rules.length; j < jlen; j++ ) { - if ( typeof rules[j].cssText !== 'string' ) { - // WTF; abort and fall back to cloning the node - throw new Error( 'sheet.cssRules[' + j + '].cssText is not a string' ); - } - cssText += rules[j].cssText + '\n'; - } - cssText += '/* Transplanted styles from ' + sheet.href + ' */\n'; - styleNode = newDoc.createElement( 'style' ); - styleNode.textContent = cssText; - } catch ( e ) { - styleNode = undefined; - } + this.constructor.static.transplantStyles( this.getElementDocument(), this.$document[0], + function () { + frame.initialized = true; + frame.emit( 'initialize' ); } - if ( !styleNode ) { - // It's either a <style> tag or a <link> tag pointing to a foreign URL; just copy - // it to the new document - styleNode = newDoc.importNode( sheet.ownerNode, true ); - } - newDoc.body.appendChild( styleNode ); - } + ); }; /** diff --git a/src/OO.ui.Window.js b/src/OO.ui.Window.js index 39826d2..b87a65d 100644 --- a/src/OO.ui.Window.js +++ b/src/OO.ui.Window.js @@ -32,6 +32,9 @@ // Initialization this.$element .addClass( 'oo-ui-window' ) + // Hide the window using visibility: hidden; while the iframe is still loading + // Can't use display: none; because that prevents the iframe from loading in Firefox + .css( 'visibility', 'hidden' ) .append( this.$frame ); this.$frame .addClass( 'oo-ui-window-frame' ) @@ -268,6 +271,10 @@ this.$overlay ); + // Undo the visibility: hidden; hack from the constructor and apply display: none; + // We can do this safely now that the iframe has initialized + this.$element.hide().css( 'visibility', '' ); + this.emit( 'initialize' ); return this; @@ -318,9 +325,9 @@ OO.ui.Window.prototype.open = function ( data ) { if ( !this.opening && !this.closing && !this.visible ) { this.opening = true; - this.$element.show(); - this.visible = true; this.frame.run( OO.ui.bind( function () { + this.$element.show(); + this.visible = true; this.frame.$element.focus(); this.emit( 'opening', data ); this.setup( data ); diff --git a/src/styles/OO.ui.Window.css b/src/styles/OO.ui.Window.css index 77c7e94..0f9dc17 100644 --- a/src/styles/OO.ui.Window.css +++ b/src/styles/OO.ui.Window.css @@ -1,7 +1,3 @@ -.oo-ui-window { - display: none; -} - .oo-ui-window-head { -webkit-touch-callout: none; -webkit-user-select: none; -- To view, visit https://gerrit.wikimedia.org/r/96431 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: I76688a5ad7bd144c5e46064119f607060e76f0d9 Gerrit-PatchSet: 6 Gerrit-Project: oojs/ui Gerrit-Branch: master Gerrit-Owner: Catrope <roan.katt...@gmail.com> Gerrit-Reviewer: Krinkle <krinklem...@gmail.com> Gerrit-Reviewer: jenkins-bot _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits