User:Krinkle/Extension review/WebFonts

HTML escaping
When using localization messages, be sure to always make sure it is properly escaped to prevent potential html injections as well as preventing malformed markup with special characters.


 * If using jQuery's, use   instead of  . jQuery will make sure to set the elements' inner text value instead of the raw html. This is the best option and is also fastest in performance because it avoids escaping all together because .text goes almost straight into the browser, removing the need for escaping.
 * If using jQuery's, escape manually
 * If manually building an html string, escape manually by creating a message object and calling  (instead of the   shortcut, which does   ):

On that note, I noticed a lot of  calls even though the element being appended to is empty. In that case simply use.

Status : ✅

CSS Injection
Regarding.

It makes assumptions about which formats a fonts has, and yet at the same time it concatenates everything conditionally. If a font does not have a 'ttf' format, the syntax will be broken for  as there would be a missing semi-colon (which is only concatenated if there is a ttf format available).

A way around this bug is by using an array for that part of the css, and afterwards using join with a comma and adding a semi-colon at the end.

While at it, you may want to use  instead to create and inject style tags. It's a little more efficient/faster for performance.

Status : ✅
 * IE crashes if mw.uti.addCSS is used. Generally it works, except for css with @font-face, More: http://stackoverflow.com/questions/7931658/dynamically-adding-font-face-rule-in-ie-8-and-less - User:Santhosh.thottingal

Globals
Usage of config globals in JavaScript has been deprecated as of 1.18. It still works by default but wikis can disable it at will. In which case the following error appears and the rest is not loaded:

Use  instead.


 * IE crashes if mw.uti.addCSS is used. Generally it works, except for css with @font-face, More: http://stackoverflow.com/questions/7931658/dynamically-adding-font-face-rule-in-ie-8-and-less - User:Santhosh.thottingal

Status : ✅

HTML fragments
This fails in older versions of Internet Explorer. jQuery creates it's fragments using innerHTML. As documented, shorthand for fragments is not allowed. Input must be valid HTML, since a self-closing anchor tag is invalid, this fails in IE which is more stricter than other browsers.

For the creation of elements (instead of fragments), self-closing shorthand is allowed but there is no need to do so. And the alternative is shorter as well. (  instead of   ).

Status : ✅

Dependencies
I see several uses of $.cookie, don't forget to add the  module to the dependencies of your module to make sure it's loaded correctly.

User options / wgWebFontsEnabledByDefault

 * See also

Since it's used in PHP as a preference, please use it as a preference here as well. This condition doesn't make sense as this ignores logged-in users. Use  instead. Does that mean wgWebFontsEnabledByDefault no longer has to be exported to JavaScript ?

Alternatively you may want to remove the condition all together since the module is only loaded by PHP if the user has this preference anyway:

Status : ✅

Logging
Use  instead of conditionally calling. It is a no-op in production mode and in debug mode it either reroutes to console.log or creates a html-console on the fly.

Status : ✅

jQuery stuff
You can use multiple selectors separated by comma, just like in CSS. No need to repeat it twice.

I see this in another part of the code as well. Use jQuery method "chaining". Like:

You probably mean the -property instead of the  -attribute. Use  instead, as changing the attribute will not change the state of the checkbox (browsers use the attribute value to determine the state of the checkbox when the page loads, after that only the property is used and the attribute is ignored).

In older versions of jQuery one had to use, later they builded a mapping system into   to automatically update the property as well, however since it's not really an attribute they created   in jQuery 1.6, please use it.

Read more about the difference between attributes and properties here. See also .prop and .attr

Please be consistent. I'd recommend using, since that's what the code actually returns (it returns the index or  ).

Status : ✅

Code conventions
Nothing major, just a few minor points. See also the code conventions page for a more complete list, not going to repeat them here.

Status : ✅

Module
Right now there are two modules:
 * webfonts.fontlist: This initializes the $.webfonts object and populates the $.webfonts.config property
 * webfonts: (depends on webfonts.fontlist) Re-initializes the $.webfonts object, selectively keeping some values from the another object. Also performs the on-documentready events.

I'd recommend moving  into the   object instead of the jQuery object. Reason being that it doesn't appear be acting like a general purpose jQuery plugin (such as  or  ). Instead it's an object literal with helper methods doing something for MediaWiki, therefor it may be better to store this in.

The way it works now is good enough. For easier testing later though you may want to consider separating definition from execution as a good practice.


 * ext.webFonts.core module
 * ext.webFonts.js: Defines the mw.webFonts object. Much like the webfonts.js right now, except that config would just be
 * ext.webFonts.fontlist.js: Extends mw.webFonts.config object with the list of fonts and languages
 * ext.webFonts.css


 * ext.webFonts.init module
 * ext.webFonts.init.js: Depends on ext.webFonts.core. Only binds mw.webFonts.setup to document ready.

Also, set  for both of these modules so that they start loading and initializing from the head and only execute after document ready. (instead of now when they start downloading after document ready)

Status : ✅

font-family:none

 * Action: View English page when logged-out.
 * Reaction OK: No fonts are loaded; No menu appears; DOM remains unchanged
 * Action: View English page with  when logged-out
 * Reaction OK: Font 'MiriamCLM-Book' is loaded. Menu appears; Above span gets
 * Action: Click Reset
 * Reaction Problem: JavaScript sets  (which doesn't exist and browsers like Chrome fallback to an inappropriate serif font ). It should instead remove the font-family in order to fallback to how it was before ("reset").

Status : ✅

Default user options
This one is somewhat confusing. The confusion is caused because the default value for that is already set to true from the main WebFonts.php file. The  setting is now ignored for logged-in users, because it's set to true no matter what. The solution is fairly simple though, simply set the default user option value to " ". However do so from the UserGetDefaultOptions hook (since setting it from the main php file would leave no way for a wiki to actually configure wgWebFontsEnabledByDefault from LocalSettings.php).

Status : ✅

File structure
General structure looks good. Uses hooks and resource loader. May want to create a  directory and store css/js files that belong to the same module next to each other in the same directory, but the way it is now is fine too.

(Niklas) I would use folder name resources, and name the files ext.webfonts.css/js and ext.webfonts.fontlist.js.

Status : ✅

Context
The principle of contexts is still relatively new in MediaWiki, but I would really recommend making use of them for anything new. See Manual:RequestContext.php, short example:

Status : ✅