Page MenuHomePhabricator

Graph must not use synchronous `window.mw` check (doesn't work asynchronous)
Closed, ResolvedPublic

Description

Graph currently uses document.write. More-over, it hardcodes if(window.mw) (instead of using ResourceLoader::makeLoaderConditionalScript) which means it is broken in current master because this condition changed.

Graph.body.php:

	$imgTagHtml::rawElement( 'img', array( 'class' => 'mw-wiki-graph-img', .. );
	..
if ( $imgTag ) {
	$encImg = FormatJson::encode( $imgTag, false, FormatJson::UTF8_OK );
	$result .= Html::inlineScript( 'if(!window.mw){document.write(' . $encImg . ');}' ) .
			Html::rawElement( 'noscript', array(), $imgTag );

If you need inline javascript that runs only when jQuery/ResourceLoader initialise, use ResourceLoader::makeInlineScript (which is a wrapper around Html::inlineScript and ResourceLoader::makeLoaderConditionalScript).

However in this case it actually does the opposite. It checks for absence rather than presence of window.mw. And also uses <noscript>. This no longer works in current MediaWiki master because mediawiki.js loads asynchronously.

Instead of this javascript and noscript code, use a plain image with a class and hide it in CSS. E.g. .client-js .mw-graph-something { display: none; }.


See also:

Event Timeline

Krinkle created this task.Aug 5 2015, 1:18 AM
Krinkle assigned this task to Yurik.
Krinkle raised the priority of this task from to Unbreak Now!.
Krinkle updated the task description. (Show Details)
Krinkle added a project: Graphs.
Krinkle added subscribers: Krinkle, Yurik.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 5 2015, 1:18 AM
Krinkle lowered the priority of this task from Unbreak Now! to High.Aug 5 2015, 1:44 AM
Krinkle updated the task description. (Show Details)
Krinkle set Security to None.
Yurik added a comment.Aug 5 2015, 3:37 PM

@Krinkle, what would you recommend to achieve the following behaviour:

  • For non-javascript browsers, insert <img> tag
  • For browsers with old javascript (e.g. IE7), insert the <img> tag (same as non-js)
  • For modern browsers, insert the <div> tag

@Krinkle, what would you recommend to achieve the following behaviour:

  • For non-javascript browsers, insert <img> tag
  • For browsers with old javascript (e.g. IE7), insert the <img> tag (same as non-js)
  • For modern browsers, insert the <div> tag
  • Use javascript (delivered as ResourceLoader module) to render the <div> tag. This is naturally not applied to non-js and old browsers.
  • Use a plain <img> element for the fallback that is hidden by default and made visible with CSS using its classname as descendent of .client-nojs. Using e.g. .client-js .mw-graph-something { display: none; } (in an addModuleStyles top module).

Change 229840 had a related patch set uploaded (by Ori.livneh):
Improve fallback rendering logic

https://gerrit.wikimedia.org/r/229840

ori closed this task as Resolved.Aug 6 2015, 7:29 PM
ori added a subscriber: ori.

Change 229840 merged by jenkins-bot:
Improve fallback rendering logic

https://gerrit.wikimedia.org/r/229840

Change 229967 had a related patch set uploaded (by Ori.livneh):
Improve fallback rendering logic

https://gerrit.wikimedia.org/r/229967

Change 229967 merged by jenkins-bot:
Improve fallback rendering logic

https://gerrit.wikimedia.org/r/229967