Page MenuHomePhabricator

Abandon use of globals in MobileFrontend classes
Closed, ResolvedPublic3 Estimated Story Points

Description

Acceptance criteria

  • We want to discourage access to certain jQuery and window to encourage behaviours that are compatible with modern browser developments
  • We want to reduce the possibility of side effects e.g, component SearchOverlay can impact TalkOverlay
  • We want to make it easier to test components and change their behaviour in different contexts.
  • We want to prevent us using jQuery/window directly in future coding. We'll enforce using eslint and mw.log.deprecate

Global use of jQuery for finding elements

Many of our classes access global jQuery to select elements. We talked about how this was an anti-pattern as using a global query can access elements outside the View.

ag "[^\.]\\$\( (?!(function|window|this|'<|document|'html|'body))" resources/ --ignore resources/**/[^a-z]*

Note: Regex issue. Should not match any filename which begin with lowercase letter...

Expected

  • Only View.js should do this...
  • Do not access 'body', 'html' from inside classes - move these outside the classes.

Global use of window, document

docfix x ~/git/core/extensions/MobileFrontend $ ag window resources/
resources/mobile.categories.overlays/CategoryAddOverlay.js
125:					window.location.hash = '#/categories';

resources/mobile.editor.common/EditorOverlayBase.js
65:		this.allowCloseWindow = mw.confirmCloseWindow( {
165:				!window.confirm( mw.msg( 'mobile-frontend-editor-new-page-confirm', mw.user ) )
205:			$( window ).off( 'beforeunload.mfeditorwarning' );
213:			window.location = mw.util.getUrl( title );
216:				window.location.reload();
241:			window.scrollTo( 0, 1 );
319:						self.allowCloseWindow.release();
324:				this.allowCloseWindow.release();

resources/mobile.editor.overlay/EditorOverlay.js
155:								if ( window.confirm( mw.msg( 'mobile-frontend-editor-switch-confirm' ) ) ) {
293:			window.scrollTo( 0, this.scrollTop );
457:						window.location = mw.util.getUrl( title );

resources/mobile.editor.ve/ve.init.mw.MobileFrontendArticleTarget.js
30:	this.onWindowScrollDebounced = ve.debounce( this.onWindowScroll.bind( this ), 100 );
31:	$( this.getElementWindow() ).on( 'scroll', this.onWindowScrollDebounced );
52:	$( this.getElementWindow() ).off( 'scroll', this.onWindowScrollDebounced );
82: * Handle window scroll events
84:ve.init.mw.MobileFrontendArticleTarget.prototype.onWindowScroll = function () {
85:	var $window, windowTop, contentTop,
88:	// The window can only scroll in iOS if the keyboard has been opened
90:		// iOS applies a scroll offset to the window to move the cursor
92:		$window = $( target.getElementWindow() );
93:		windowTop = $window.scrollTop();
96:		$window.scrollTop( 0 );
97:		surface.scrollTo( contentTop + windowTop );
247:	window.history.back();

resources/mobile.editor.ve/VisualEditorOverlay.js
149:}( mw.mobileFrontend, jQuery, window.ve ) );

resources/mobile.infiniteScroll/InfiniteScroll.js
66:		 * Listen to scroll on window and notify this._onScroll
112:			var scrollBottom = $( window ).scrollTop() + $( window ).height(),

resources/mobile.mediaViewer/ImageGateway.js
38:				imageSizeMultiplier = ( window.devicePixelRatio && window.devicePixelRatio > 1 ) ? window.devicePixelRatio : 1;
51:					iiurlwidth: findSizeBucket( $( window ).width() * imageSizeMultiplier ),
52:					iiurlheight: findSizeBucket( $( window ).height() * imageSizeMultiplier )

resources/mobile.mediaViewer/ImageOverlay.js
86:			window.location.hash = '#/media/' + encodeURIComponent( thumbnail.getFileName() );
205:			window.location.hash = '';
215:		 * Fit the image into the window if its dimensions are bigger than the window dimensions.
216:		 * Compare window width to height ratio to that of image width to height when setting
222:			var detailsHeight, windowWidth, windowHeight, windowRatio, $img;
226:			windowWidth = $( window ).width();
227:			windowHeight = $( window ).height() - detailsHeight;
228:			windowRatio = windowWidth / windowHeight;
231:			if ( this.imgRatio > windowRatio ) {
232:				if ( windowWidth < this.thumbWidth ) {
234:						width: windowWidth,
239:				if ( windowHeight < this.thumbHeight ) {
242:						height: windowHeight
250:		 * Function to adjust the height of details section to not more than 50% of window height.
254:			var windowHeight = $( window ).height();
255:			if ( this.$( '.details' ).height() > windowHeight * 0.50 ) {
256:				this.$( '.details' ).css( 'max-height', windowHeight * 0.50 );

resources/mobile.nearby/Nearby.js
218:				hash = window.location.hash;
232:					window.location.hash = $( this ).attr( 'id' );
238:				offset = $( window.location.hash ).offset();
241:					$( window ).scrollTop( offset.top );

resources/mobile.notifications.overlay/NotificationsOverlay.js
163:			window.location.href = this.badge.getNotificationURL();

resources/mobile.patrol.ajax/init.js
55:				// (or open it in a new window, bypassing this ajax module).

resources/mobile.pointerOverlay/PointerOverlay.js
137:			// use a global window event so that on resizes it is correctly positioned.

resources/mobile.references/references.less
14:	max-height: 400px;  // or half of window height, whichever is smaller (see ReferencesDrawerBeta.js)

resources/mobile.references/ReferencesDrawer.js
59:			var windowHeight = $( window ).height();
64:			if ( windowHeight / 2 < 400 ) {
65:				this.$el.css( 'max-height', windowHeight / 2 );
104:					window.location.hash = id;

resources/mobile.search/SearchOverlay.js
125:		 * SearchOverlay is not managed by OverlayManager and using window.history.back() causes
135:			window.location.hash = '';
168:			window.history.back();
180:			window.setTimeout( function () {
237:				window.location.href = $link.attr( 'href' );

resources/mobile.search/SearchOverlay.less
53:		// See https://msdn.microsoft.com/en-us/library/windows/apps/hh465740.aspx (T102325)

resources/mobile.startup/browser.js
115:			return window.innerWidth >= val || window.innerHeight >= val;
159:			return 'ontouchstart' in window;
177:			browser = new Browser( window.navigator.userAgent, $( 'html' ) );

resources/mobile.startup/Drawer.js
70:				$window = $( window );
72:				$window.one( 'click.drawer', $.proxy( self, 'hide' ) );
74:					$window.one( 'scroll.drawer', $.proxy( self, 'hide' ) );
85:			$( window ).off( '.drawer' );

resources/mobile.startup/Overlay.js
9:		$window = $( window );
12:	 * Mobile modal window
148:			window.history.back();
196:				window.scrollTo( 0, 1 );
205:				$window
210:						self._resizeContent( $window.height() );
227:				window.scrollTo( document.body.scrollLeft, this.scrollTop );
233:				$window.off( '.ios' );
246:		 * Fit the overlay content height to the window taking overlay header and footer heights
250:		 * @param {number} windowHeight The height of the window
252:		_resizeContent: function ( windowHeight ) {
254:				windowHeight -
279:				this._resizeContent( $( window ).height() );
288:								$window.scrollTop( 999 );
289:								keyboardHeight = $window.scrollTop();
290:								$window.scrollTop( 0 );
293:							if ( $window.height() > keyboardHeight ) {
294:								self._resizeContent( $window.height() - keyboardHeight );
299:						self._resizeContent( $window.height() );
301:						$window.scrollTop( 0 );

resources/mobile.startup/Page.js
56:		 * @cfg {string} defaults.hash Window location hash.
74:			hash: window.location.hash,
238:				$( window ).on( 'load', function () {
239:					window.location.hash = self.options.hash;

resources/mobile.startup/PageList.js
46:			window.setTimeout( function () {

resources/mobile.startup/Skin.js
60:		 * Tests current window size and if suitable loads styles and scripts specific for larger devices
173:				offset = $( window ).height() * 1.5,

resources/mobile.talk.overlays/TalkOverlay.js
111:					window.location = mw.util.getUrl( options.title );

resources/mobile.talk.overlays/TalkSectionAddOverlay.js
72:			if ( this._saveHit || empty || window.confirm( confirmMessage ) ) {
107:						window.history.back();

resources/mobile.toggle/toggle.js
216:				window.scrollTo( 0, $target.offset().top );
314:			var hash = window.location.hash;
332:				window.location.hash = internalRedirectHash;

Hygiene: move window.location manipulation to clients

Possible violators may be identified with rg -sg\!/tests window.location. They seem to fall mostly into two categories:

  1. Bound directly or almost directly (click or click-like event listener, simple function call, etc):
    • resources/mobile.nearby/Nearby._postRenderLinks() (in a private click handler)
    • resources/mobile.toggle/toggle.Toggle()
    • resources/mobile.categories.overlays/CategoryAddOverlay.onSaveClick()
    • resources/mobile.mediaViewer/ImageOverlay.setNewImage()
    • resources/mobile.references/ReferencesDrawer.showReference()

      It seems that these cases should generally be straightforward to solve by having the client pass a click listener.
  1. Bound to longer event chain:
    • resources/mobile.search/SearchOverlay.onExit() and onClickResult() (the latter is wired into the router)
    • resources/mobile.mediaViewer/ImageOverlay.onExit()
    • resources/mobile.editor.common/EditorOverlayBase.onSaveComplete()
    • resources/mobile.talk.overlays/TalkOverlay.postRender()
    • resources/mobile.editor.overlay/EditorOverlay.onSaveBegin()
    • resources/mobile.startup/Page.postRender()
    • resources/mobile.notifications.overlay/NotificationsOverlay.onError()

For #2, emit an event to the client or refactor to #1. e.g., in TalkOverlay, the caller creates a new instance and the postRender hook kicks off an HTTP request that may fail:

this.pageGateway.getPage( options.title ).fail( function ( resp ) {
  ...
  window.location = ...
  ...
} )...

Here we can either emit on the failure to the client or refactor the HTTP request to be initiated by the client and return a Deferred.

Details

SubjectRepoBranchLines +/-
mediawiki/extensions/MobileFrontendmaster+42 -6
mediawiki/skins/MinervaNeuemaster+4 -9
mediawiki/extensions/MobileFrontendmaster+25 -11
mediawiki/extensions/MobileFrontendmaster+64 -37
mediawiki/extensions/MobileFrontendmaster+7 -18
mediawiki/skins/MinervaNeuemaster+29 -6
mediawiki/extensions/MobileFrontendmaster+13 -13
mediawiki/extensions/MobileFrontendmaster+1 -1
mediawiki/extensions/MobileFrontendmaster+24 -1
mediawiki/extensions/MobileFrontendmaster+42 -37
mediawiki/extensions/MobileFrontendmaster+38 -23
mediawiki/extensions/MobileFrontendmaster+52 -26
mediawiki/extensions/MobileFrontendmaster+68 -48
mediawiki/extensions/MobileFrontendmaster+1 -2
mediawiki/extensions/MobileFrontendmaster+73 -54
mediawiki/skins/MinervaNeuemaster+3 -0
mediawiki/extensions/MobileFrontendmaster+18 -12
mediawiki/extensions/MobileFrontendmaster+23 -17
mediawiki/extensions/MobileFrontendmaster+22 -4
mediawiki/extensions/MobileFrontendmaster+39 -27
Show related patches Customize query in gerrit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Change 372445 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Hygiene: Do not use global $ function in classes

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

Change 372445 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hygiene: Do not use global $ function in classes

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

ovasileva set the point value for this task to 3.

Change 413783 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: move event modifier detection to util

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

Change 413802 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: move Nearby item click handler to caller

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

Change 413803 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Update: make Nearby URL subpage case-sensitive

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

Change 413783 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hygiene: move event modifier detection to util

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

Change 414844 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Limit jQuery usage to mobile.startup and entry points

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

Change 413802 merged by Jdlrobson:
[mediawiki/extensions/MobileFrontend@master] Hygiene: move Nearby item click handler to caller

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

Change 413803 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Update: make Nearby URL subpage case-sensitive

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

Change 415213 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: move Nearby scrolling to client

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

Change 415372 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: move CategoryAddOverlay window use to client

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

Change 415373 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/MinervaNeue@master] Hygiene: move CategoryAddOverlay window use to client

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

Change 414861 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Introduce View.prototype.parseHTML Always use local jQuery where possible

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

Change 415491 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Access to window object restricted to util class

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

Change 415492 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Access to html object restricted to util class

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

Change 415373 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Hygiene: move CategoryAddOverlay window use to client

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

Change 373962 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Use Function.bind in favor of $.proxy

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

Change 415213 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hygiene: move Nearby scrolling to client

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

Change 415372 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hygiene: move CategoryAddOverlay window use to client

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

Change 414861 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Introduce util.parseHTML and View.prototype.parseHTML

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

Change 415491 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Access to window object restricted to util class

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

Change 415492 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Access to html object restricted to util class

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

Change 373962 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Use Function.bind in favor of $.proxy inside components

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

Change 414844 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Limit jQuery usage to mobile.startup and entry points

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

Change 415922 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Restrict usage of jQuery to View and util.js

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

Change 415927 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Hygiene: window.setTimeout => setTimeout

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

Change 415927 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hygiene: window.setTimeout => setTimeout

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

Change 415933 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: use router in nearby

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

Change 415933 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hygiene: use router in nearby

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

Change 416503 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/MinervaNeue@master] Chore: move window.location use to client

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

Change 416504 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Chore: move window.location ImageOverlay use to client

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

Change 416503 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Chore: move window.location use to client

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

Change 416504 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Chore: move window.location ImageOverlay use to client

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

Change 416884 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: remove window.location use from overlays

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

Change 416885 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/MinervaNeue@master] Hygiene: remove window.location use from overlays

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

Change 417176 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Disallow the direct usage of window.location

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

Change 415922 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Restrict usage of jQuery to View and util.js

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

Change 416884 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hygiene: remove window.location use from overlays

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

Change 416885 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Hygiene: remove window.location use from overlays

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

Change 417176 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Disallow the direct usage of window.location

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

@Jhernandez care to look through what's happened here and sign off?

The work is by no means complete but we're in a good place to write better code going forward.

There's 2 remaining follow ups, but they don't need to be merged to call this task done as we've done quite a bit already (we can split those into a new task if they don't get merged and deemed necessary)
e.g. https://gerrit.wikimedia.org/r/417335 and https://gerrit.wikimedia.org/r/417336

To summarise the goals we talked about during the after-sync meeting:

  • We want to discourage access to certain jQuery and window to encourage behaviours that are compatible with modern browser developments
  • We want to reduce the possibility of side effects
  • We want to make it easier to test components and change their behaviour in different contexts.
  • We want to prevent us using jQuery/window directly in future coding. We'll enforce using eslint and mw.log.deprecate

@Jdlrobson I've checked and there are still some usages without the util module:

MobileFrontend (master $=) => ag "[^\.]\\$\( (?!(function|window|this|'<|document|'html|'body))" resources/  --ignore resources/**/[^a-z]*
resources/mobile.special.uploads.scripts/uploads.js
19:		if ( $( '.errorbox' ).length === 0 ) {

resources/mobile.special.nearby.scripts/nearby.js
15:				el: $( '#mw-mf-nearby' ),
27:			$btn = $( '#secondary-button' ).parent(),

resources/mobile.special.mobilediff.scripts/init.js
17:		var $patrolLinks = $( '.patrollink a' ),
20:			$spinner = $( new Icon( {
28:			$( e.target ).hide().after( $spinner );

resources/mobile.startup/Skin.js
211:				return loadImage( $( placeholder ) );

resources/mobile.special.mobileoptions.scripts/mobileoptions.js
128:			enableToggle = OO.ui.infuse( $( '#enable-beta-toggle' ) ),
130:			$form = $( '#mobile-options' );

resources/mobile.special.watchlist.scripts/watchlist.js
14:		var $watchlist = $( 'ul.page-list' );
17:		if ( $( '.mw-mf-watchlist-selector' ).length === 0 ) {
28:		$( '.more' ).remove();
33:			view = $( '.button-bar .is-on a' ).data( 'view' ),
34:			filter = $( '.mw-mf-watchlist-selector .selected a' ).data( 'filter' );

resources/mobile.editor.ve/ve.init.mw.MobileFrontendArticleTarget.js
94:		$window = $( target.getElementWindow() );
164:	this.overlay._fixIosHeader( $( '[contenteditable]' ) );

resources/mobile.special.userlogin.scripts/userlogin.js
6:		$( '#wpRemember' ).prop( 'checked', true );

resources/mobile.init/init.js
100:			$content = $( '#content #bodyContent' );
112:			isWatched: $( '#ca-watch' ).hasClass( 'watched' ),

resources/mobile.startup/Skin.js
211:				return loadImage( $( placeholder ) );

Do we want to fix these while we are at it or are they out of scope?

Also, the beggining of the description has expected checklist and at the end there is other acceptance criteria that covers a lot more than what has been done here. What is the valid one? Should we split the AC at the bottom into other tasks or consider it done?

Do we want to fix these while we are at it or are they out of scope?

There are a few things we didn't touch.

@Jdlrobson I've checked and there are still some usages without the util module:

MobileFrontend (master $=) => ag "[^\.]\\$\( (?!(function|window|this|'<|document|'html|'body))" resources/  --ignore resources/**/[^a-z]*
resources/mobile.special.uploads.scripts/uploads.js
19:		if ( $( '.errorbox' ).length === 0 ) {

resources/mobile.special.nearby.scripts/nearby.js
15:				el: $( '#mw-mf-nearby' ),
27:			$btn = $( '#secondary-button' ).parent(),

resources/mobile.special.mobilediff.scripts/init.js
17:		var $patrolLinks = $( '.patrollink a' ),
20:			$spinner = $( new Icon( {
28:			$( e.target ).hide().after( $spinner );

resources/mobile.special.mobileoptions.scripts/mobileoptions.js
128:			enableToggle = OO.ui.infuse( $( '#enable-beta-toggle' ) ),
130:			$form = $( '#mobile-options' );

resources/mobile.special.watchlist.scripts/watchlist.js
14:		var $watchlist = $( 'ul.page-list' );
17:		if ( $( '.mw-mf-watchlist-selector' ).length === 0 ) {
28:		$( '.more' ).remove();
33:			view = $( '.button-bar .is-on a' ).data( 'view' ),
34:			filter = $( '.mw-mf-watchlist-selector .selected a' ).data( 'filter' );


resources/mobile.special.userlogin.scripts/userlogin.js
6:		$( '#wpRemember' ).prop( 'checked', true );

resources/mobile.init/init.js
100:			$content = $( '#content #bodyContent' );
112:			isWatched: $( '#ca-watch' ).hasClass( 'watched' ),

We purposely left all the above since we decided it was okay to use jQuery inside initialisation scripts for the time being. The scope of the task ended up becoming centered around components.

resources/mobile.startup/Skin.js
211: return loadImage( $( placeholder ) );

Above this we do

$ = this.$.bind( this )

so that's a local $ usage.

resources/mobile.editor.ve/ve.init.mw.MobileFrontendArticleTarget.js
94: $window = $( target.getElementWindow() );
164: this.overlay._fixIosHeader( $( '[contenteditable]' ) );

We decided to leave this, since this is owned by VisualEditor team.

Do we want to fix these while we are at it or are they out of scope?

Also, the beggining of the description has expected checklist and at the end there is other acceptance criteria that covers a lot more than what has been done here. What is the valid one? Should we split the AC at the bottom into other tasks or consider it done?

I've added a checklist to the start of the task description. I think it's best to assess this task based on the goals.
Let's ignore the list at the end :)

Thanks for the clarification and the comments, this seems done then.