Page MenuHomePhabricator

Separate Lazy loading code from Skin.js
Closed, ResolvedPublic

Description

The Skin.js code contains code with the responsibility of lazy loading images and references. It currently does this with little testing and by emitting and watching for events on an eventBus that is passed into it as a parameter.

Acceptance criteria

  • We will refactor code so that the Skin makes use of 2 tightly scoped modules that add lazy loading images and lazy loading references capabilities.
  • We will add test coverage for the Skin class

We added tests for the lazyImageLoader which used to be part of the Skin class (https://gerrit.wikimedia.org/r/486372)

  • We will add tests coverage for newly created lazyReferencesLoader.js and lazyImagesLoader.js code.
  • We will remove usages of jQuery in the codebase

Per T214658#4952285

QA steps

  • Go to an article on a mobile device with images.
  • Disable internet browser connection to verify that images in sections are not loaded by default
  • Enable internet browser connection and notice how images load as you scroll to or open the sections.
  • Opt into beta.

References

Important: Use a test article that exists on the beta cluster with references e.g. https://en.m.wikipedia.beta.wmflabs.org/wiki/Selenium_References_test_page?mobileaction=beta

  • Find an article with references and notice how the references do not load with the internet connection disabled
  • Enable internet and confirm references load

If any of the above does not hold true please report it and move this card to needs more work.

QA Results

StatusDetails
✅ PassedT214658#4970710

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 25 2019, 1:04 AM

Change 483515 had a related patch set uploaded (by Jdlrobson; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: reduce jQuery in lazyImageLoader

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

Change 486372 had a related patch set uploaded (by Jdlrobson; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: add tests to lazyImageLoader

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

Change 484305 had a related patch set uploaded (by Jdlrobson; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: move lazy image loader placeholder query

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

Change 485122 had a related patch set uploaded (by Jdlrobson; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: make lazyImageLoader jQuery usage consistent

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

Change 483516 had a related patch set uploaded (by Jdlrobson; owner: Stephen Niedzielski):
[mediawiki/skins/MinervaNeue@master] Hygiene: revise lazyImageLoader.loadImages() API

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

Change 484306 had a related patch set uploaded (by Jdlrobson; owner: Stephen Niedzielski):
[mediawiki/skins/MinervaNeue@master] Update: use lazyImageLoader to query placeholders

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

Change 484748 had a related patch set uploaded (by Jdlrobson; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: isolate lazy image load state

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

Change 484767 had a related patch set uploaded (by Jdlrobson; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: separate lazy references loading

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

Change 484576 had a related patch set uploaded (by Jdlrobson; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: remove Skin "changed" event

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

Change 483515 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hygiene: reduce jQuery in lazyImageLoader

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

Change 484305 merged by Jdlrobson:
[mediawiki/extensions/MobileFrontend@master] Hygiene: move lazy image loader placeholder query

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

Change 484306 abandoned by Niedzielski:
Update: use lazyImageLoader to query placeholders

Reason:
Collapsed into I2bf42366c0e27462c32162124d07761b91d66166 given dependency mismatch.

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

Change 483516 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Hygiene: revise lazyImageLoader.loadImages() API

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

Change 486372 merged by Jdlrobson:
[mediawiki/extensions/MobileFrontend@master] Hygiene: add tests to lazyImageLoader

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

Niedzielski removed Niedzielski as the assignee of this task.Feb 4 2019, 6:16 PM
Niedzielski added a subscriber: Niedzielski.

This is ready for sign off by anyone.

Change 487922 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: replace getAttribute with dataset and style

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

pmiazga claimed this task.Feb 5 2019, 6:08 PM

Change 487922 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hygiene: replace getAttribute with dataset and style

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

pmiazga added a comment.EditedFeb 6 2019, 1:41 AM

The acceptance criteria says We will add test coverage for the Skin class but Skin.js has no coverage.

Skin.js                                   |        0 |        0 |        0 |        0 |... 26,136,143,147 |

Both lazyReferencesLoader.js and lazyImagesLoader.js are nicely covered:

lazyImageLoader.js                        |    95.65 |    93.75 |      100 |    95.65 |                51 |
lazyReferencesLoader.js                   |     91.3 |       80 |    85.71 |     91.3 |       91,93,95,98 |

BTW, it would be awesome if we can cover also lazyImageTransformer as the coverage for that file is 0.

Last acceptance criteria says that we will remove usage of the jQuery lib, but the jQuery is still required for Deferreds and declaring types for function params, and attaching eventListeners.
Do we want to drop that last acceptance criteria?

pmiazga removed pmiazga as the assignee of this task.Feb 6 2019, 1:48 AM
pmiazga updated the task description. (Show Details)
pmiazga added a subscriber: pmiazga.
phuedx added a subscriber: phuedx.Feb 6 2019, 11:27 AM

What's the rationale for this skipping Needs QA (the first time it moved to Ready for Signoff, I guess)?

Jdlrobson updated the task description. (Show Details)Feb 6 2019, 4:02 PM

This was a significant refactor and probably should go through QA. Have added some steps.

Jdlrobson triaged this task as Normal priority.Feb 6 2019, 6:15 PM
Niedzielski raised the priority of this task from Normal to Needs Triage.
Niedzielski triaged this task as Normal priority.Feb 6 2019, 7:11 PM

Note the above comment from Stephen: T214658#4930340

Jdlrobson added a comment.EditedFeb 13 2019, 5:58 PM

I'm a little concerned that this code is riding the train (And some of it is live!) but has not been QAed...

Last acceptance criteria says that we will remove usage of the jQuery lib, but the jQuery is still required for Deferreds and declaring types for function params, and attaching eventListeners.

jQuery is the underlying implementation for util.Deferred() which lazyImageLoader uses. lazyImageLoader has no direct jQuery dependency. Swapping out the underlying util implementation for Promises or a polyfill is out of scope.

it would be awesome if we can cover also lazyImageTransformer as the coverage for that file is 0.

I agree but we've spent ages on this task and have already made significant improvements to the codebase because of it. This part will have to wait for another day.

The acceptance criteria says We will add test coverage for the Skin class but Skin.js has no coverage.

Many tests were added for lazy image loading but Skin itself is still not especially testable. I'll look into this.

The acceptance criteria says We will add test coverage for the Skin class but Skin.js has no coverage.

Many tests were added for lazy image loading but Skin itself is still not especially testable. I'll look into this.

@Jdlrobson, per your AC I've taken a closer look at Skin and I don't think it will be the best use of time to write tests for it. Part of the reason is that Skin does not provide many seams and it modifies the page's HTML. I'm thinking at the end of it we would have similar feelings as we did for CtaDrawer.test.js. It's only about 140 lines but look at what we're actually validating:

Skin.js
var
	browser = require( './Browser' ).getSingleton(),
	lazyImageLoader = require( './lazyImages/lazyImageLoader' ),
	lazyImageTransformer = require( './lazyImages/lazyImageTransformer' ),
	lazyReferencesLoader = require( './lazyReferencesLoader' ),
	View = require( './View' ),
	util = require( './util' ),
	mfExtend = require( './mfExtend' );

/**
 * Representation of the current skin being rendered.
 *
 * @class Skin
 * @extends View
 * @uses Browser
 * @uses Page
 * @fires Skin#click
 * @fires Skin#references-loaded
 * @fires Skin#changed
 * @param {Object} params Configuration options
 * @param {OO.EventEmitter} params.eventBus Object used to listen for
 * scroll:throttled, resize:throttled, and section-toggled events
 */
function Skin( params ) {
	var self = this,
		options = util.extend( {}, params );

	this.page = options.page;
	this.name = options.name;
	if ( options.mainMenu ) {
		this.mainMenu = options.mainMenu;
		mw.log.warn( 'Skin: Use of mainMenu is deprecated.' );
	}
	this.eventBus = options.eventBus;
	options.isBorderBox = false;
	View.call( this, options );
	this.referencesGateway = options.referencesGateway;

	if (
		mw.config.get( 'wgMFLazyLoadImages' )
	) {
		util.docReady( function () {
			var
				container = document.getElementById( 'content' ),
				// todo: remove when tests are only headless. There is no #content in
				//       Special:JavaScriptTest.
				images = ( container && lazyImageLoader.queryPlaceholders( container ) ) || [];
			self.lazyImageTransformer = lazyImageTransformer.newLazyImageTransformer(
				// eslint false positive: self.$ is not a jQuery collection
				// eslint-disable-next-line jquery/no-bind
				self.eventBus, self.$.bind( self ), util.getWindow().height() * 1.5, images
			);
			self.lazyImageTransformer.loadImages();
		} );
	}

	if ( mw.config.get( 'wgMFLazyLoadReferences' ) ) {
		this.eventBus.on( 'section-toggled', function ( data ) {
			lazyReferencesLoader.loadReferences(
				self.eventBus, data, self.referencesGateway, self.page
			);
		} );
	}
}

mfExtend( Skin, View, {
	/**
	 * @memberof Skin
	 * @instance
	 * @mixes View#defaults
	 * @property {Object} defaults Default options hash.
	 * @property {Page} defaults.page page the skin is currently rendering
	 * @property {ReferencesGateway} defaults.referencesGateway instance of references gateway
	 */
	defaults: {
		page: undefined
	},

	/**
	 * @inheritdoc
	 * @memberof Skin
	 * @instance
	 */
	postRender: function () {
		var $el = this.$el;
		if ( browser.supportsAnimations() ) {
			$el.addClass( 'animations' );
		}
		if ( browser.supportsTouchEvents() ) {
			$el.addClass( 'touch-events' );
		}
		util.parseHTML( '<div class="transparent-shield cloaked-element">' )
			.appendTo( $el.find( '#mw-mf-page-center' ) );
		if ( this.lazyImageTransformer ) {
			this.lazyImageTransformer.loadImages();
		}

		/**
		 * Fired when the skin is clicked.
		 * @event Skin#click
		 */
		this.$( '#mw-mf-page-center' ).on( 'click', this.emit.bind( this, 'click' ) );
	},

	/**
	 * Returns the appropriate license message including links/name to
	 * terms of use (if any) and license page
	 * @memberof Skin
	 * @instance
	 * @return {string}
	 */
	getLicenseMsg: function () {
		var licenseMsg,
			mfLicense = mw.config.get( 'wgMFLicense' ),
			licensePlural = mw.language.convertNumber( mfLicense.plural );

		if ( mfLicense.link ) {
			if ( this.$( '#footer-places-terms-use' ).length > 0 ) {
				licenseMsg = mw.msg(
					'mobile-frontend-editor-licensing-with-terms',
					mw.message(
						'mobile-frontend-editor-terms-link',
						this.$( '#footer-places-terms-use a' ).attr( 'href' )
					).parse(),
					mfLicense.link,
					licensePlural
				);
			} else {
				licenseMsg = mw.msg(
					'mobile-frontend-editor-licensing',
					mfLicense.link,
					licensePlural
				);
			}
		}
		return licenseMsg;
	}
} );

module.exports = Skin;

Blach. Here's about what I would expect the tests to look like:

  1. Rendered View has expected HTML (note: i think this includes the page?)
  2. Rendered View respects menu option
  3. Rendered View respects lazily loaded images option
  4. Rendered View respects lazily loaded images option (no images present)
  5. Rendered View respects lazily loaded references option
  6. Rendered View respects lazily loaded references option (no references)
  7. Rendered View annotates animation support
  8. Rendered View annotates touch event support
  9. Rendered View responds to clicks
  10. getLicenseMsg()
  11. getLicenseMsg() (no terms)
  12. getLicenseMsg() (no license)

And those tests have to wait for all the side-effects of the constructor to complete. Let me know if you feel strongly about this bullet and I'll do it. Otherwise, please remove the AC and resolve this task.

Jdlrobson updated the task description. (Show Details)Feb 14 2019, 8:02 PM

Skin and I don't think it will be the best use of time to write tests for it.

You are probably right. We've added tests for lazyImageLoader which used to be part of the the Skin class, so I think we can consider the fact we did this, as improving test coverage.

Waiting on QA now, as that's an important precursor to resolving this.

Edtadros reassigned this task from Edtadros to Jdlrobson.Feb 19 2019, 1:19 AM
Edtadros added a subscriber: Edtadros.

@Jdlrobson, For the last two steps:

  • Find an article with references and notice how the references do not load with the internet connection disabled
  • Enable internet and confirm references load

Here is what I'm seeing with or without internet access while in opted into Beta:

Once I click on the "View full list of citations" link I get the following error.

I tried a few articles with the same result. The images above are from the Dog page. I verified that there are references that appear normally when opted out of the beta.

Jdlrobson updated the task description. (Show Details)Feb 19 2019, 10:38 PM
Jdlrobson updated the task description. (Show Details)

Here is what I'm seeing with or without internet access while in opted into Beta:

@Edtadros for this one looks like you need to use a test article on the beta cluster (https://en.m.wikipedia.beta.wmflabs.org/wiki/Dog?mobileaction=beta will work or the one I've added to the description!) On staging we have some magic that pulls content from production and that's interfering with testing here. Sorry about that!

Jdlrobson reassigned this task from Jdlrobson to Edtadros.Feb 19 2019, 11:23 PM

I've moved this to the top of the column as this is in production now so we should prioritise QA.

Test Results

Status: ✅ PASS
OS: iOS 12.1.4
Browser: Safari

Test Artifact(s):

Images


References


Edtadros reassigned this task from Edtadros to ovasileva.Feb 21 2019, 6:04 AM
Edtadros updated the task description. (Show Details)

Thanks @Jdlrobson, the clarification really helped!

ovasileva removed ovasileva as the assignee of this task.Feb 21 2019, 9:24 AM
ovasileva added a subscriber: ovasileva.
phuedx removed a subscriber: phuedx.Feb 25 2019, 1:27 PM
Jdlrobson raised the priority of this task from Normal to Needs Triage.Feb 26 2019, 7:10 PM
Jdrewniak closed this task as Resolved.Feb 26 2019, 10:04 PM