From 331df226f59c786ae3c662ef9fbbaa2f2c2550e1 Mon Sep 17 00:00:00 2001 From: Nicholas Ray Date: Wed, 10 Jul 2019 18:56:04 -0600 Subject: [PATCH] Make Minerva use new PageHTMLParser.js and refactored Page.js MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In I02f8645aac1d7b081eaba8f2ac92a2ef51f78182, Page.js was made into a model and its html parsing responsibilities were moved to PageHTMLParser. Additionally, a singleton for the current page (currentPage.js) and a singleton for the curent page html parser (currentPageHTMLParser.js) were introduced to replace the usage of M.getCurrentPage(). This commit refactors Minerva to make use of these changes. Notable changes: * 🐛 The event bus singleton was added to references.js since it previously relied on an instance of Skin to listen for the `references-loaded` event. However, this event is emitted on the event bus singleton. * Additionally, I didn't see a reason why the `references-loaded` event needed to also pass the current page instance when the only file listening to it is references.js (which already has the current page instance) so I removed that and the convoluted passing of page.js within the file. I assumed this logic was unecessary. * The call to drawer.showReferences in references.js now was made to pass the currentPage instance as well as the currentPageHTMLParser. This is to prepare for I6e858bbe73f83166476b5b2c0fdac1cca7404246 where the getReferences() interface for ReferencesMobileViewGateway.js and ReferencesHtmlScraperGateway.js is refactored to accept both of these instances. Additionally, references.js was refactored to support event delegation which gets rid of some parsing/setup logic. Bug: T193077 Depends-On: I02f8645aac1d7b081eaba8f2ac92a2ef51f78182 Change-Id: I2f32dbcc4ebaa4bebb297cda1ecce3457922b343 --- resources/skins.minerva.options/categories.js | 4 +- .../skins.minerva.scripts/errorLogging.js | 7 +-- resources/skins.minerva.scripts/init.js | 13 ++--- .../page-issues/index.js | 53 ++++++++++--------- resources/skins.minerva.scripts/references.js | 35 ++++-------- resources/skins.minerva.scripts/toc.js | 3 +- resources/skins.minerva.talk/init.js | 4 +- resources/skins.minerva.toggling/init.js | 9 ++-- resources/skins.minerva.watchstar/init.js | 7 ++- .../skins.minerva.scripts/pageIssues.test.js | 4 +- 10 files changed, 66 insertions(+), 73 deletions(-) diff --git a/resources/skins.minerva.options/categories.js b/resources/skins.minerva.options/categories.js index 545143e..b269b6f 100644 --- a/resources/skins.minerva.options/categories.js +++ b/resources/skins.minerva.options/categories.js @@ -25,7 +25,7 @@ module.exports = function () { return categoryOverlay( { api: new mw.Api(), isAnon: isAnon, - title: M.getCurrentPage().title, + title: mobile.currentPage().title, eventBus: eventBus } ); } ); @@ -40,7 +40,7 @@ module.exports = function () { return new CategoryAddOverlay( { api: new mw.Api(), isAnon: isAnon, - title: M.getCurrentPage().title, + title: mobile.currentPage().title, eventBus: eventBus } ); } ); diff --git a/resources/skins.minerva.scripts/errorLogging.js b/resources/skins.minerva.scripts/errorLogging.js index 4ace83b..e716ced 100644 --- a/resources/skins.minerva.scripts/errorLogging.js +++ b/resources/skins.minerva.scripts/errorLogging.js @@ -12,8 +12,9 @@ errorSamplingRate = config.get( 'wgMinervaErrorLogSamplingRate', 0 ), sessionToken = user.sessionId(), EVENT_CLIENT_ERROR_LOG = 'wikimedia.event.WebClientError', - page = M.getCurrentPage(), - util = M.require( 'mobile.startup' ).util, + mobile = M.require( 'mobile.startup' ), + currentPage = mobile.currentPage(), + util = mobile.util, errorExperiment = { name: 'WebClientError', enabled: errorSamplingRate > 0, @@ -29,7 +30,7 @@ wgVersion: config.get( 'wgVersion' ), mobileMode: config.get( 'wgMFMode', 'desktop' ), isAnon: user.isAnon(), - revision: page.getRevisionId() + revision: currentPage.getRevisionId() }; if ( isErrorLoggingEnabled ) { diff --git a/resources/skins.minerva.scripts/init.js b/resources/skins.minerva.scripts/init.js index 3e2a8fc..ad5fdf2 100644 --- a/resources/skins.minerva.scripts/init.js +++ b/resources/skins.minerva.scripts/init.js @@ -13,8 +13,9 @@ Button = mobile.Button, Anchor = mobile.Anchor, overlayManager = OverlayManager.getSingleton(), - page = M.getCurrentPage(), - $redLinks = page.getRedLinks(), + currentPage = mobile.currentPage(), + currentPageHTMLParser = mobile.currentPageHTMLParser(), + $redLinks = currentPageHTMLParser.getRedLinks(), api = new mw.Api(), eventBus = mobile.eventBusSingleton, namespaceIDs = mw.config.get( 'wgNamespaceIds' ); @@ -50,7 +51,7 @@ * @param {JQuery.Object} [$container] Optional container to search within */ function initMediaViewer( $container ) { - page.getThumbnails( $container ).forEach( function ( thumb ) { + currentPageHTMLParser.getThumbnails( $container ).forEach( function ( thumb ) { thumb.$el.off().data( 'thumb', thumb ).on( 'click', onClickImage ); } ); } @@ -96,7 +97,7 @@ return mobile.mediaViewer.overlay( { api: api, - thumbnails: page.getThumbnails(), + thumbnails: currentPageHTMLParser.getThumbnails(), title: decodeURIComponent( title ), eventBus: eventBus } ); @@ -347,8 +348,8 @@ initTabsScrollPosition(); // Setup the issues banner on the page // Pages which dont exist (id 0) cannot have issues - if ( !page.isMissing ) { - issues.init( overlayManager, page ); + if ( !currentPage.isMissing ) { + issues.init( overlayManager, currentPageHTMLParser ); } } ); diff --git a/resources/skins.minerva.scripts/page-issues/index.js b/resources/skins.minerva.scripts/page-issues/index.js index 0c2c181..b916dd8 100644 --- a/resources/skins.minerva.scripts/page-issues/index.js +++ b/resources/skins.minerva.scripts/page-issues/index.js @@ -1,7 +1,7 @@ ( function ( M ) { /** @typedef {Object.} IssueSummaryMap */ - var Page = M.require( 'mobile.startup' ).Page, + var PageHTMLParser = M.require( 'mobile.startup' ).PageHTMLParser, KEYWORD_ALL_SECTIONS = 'all', config = mw.config, NS_MAIN = 0, @@ -24,7 +24,7 @@ * will be rendered above the heading. * This function comes with side effects. It will populate a global "allIssues" object which * will link section numbers to issues. - * @param {Page} page to search for page issues inside + * @param {PageHTMLParser} pageHTMLParser parser to search for page issues * @param {string} labelText what the label of the page issues banner should say * @param {string} section that the banner and its issues belong to. * If string KEYWORD_ALL_SECTIONS banner should apply to entire page. @@ -36,7 +36,7 @@ * * @return {{ambox: JQuery.Object, issueSummaries: IssueSummary[]}} */ - function insertBannersOrNotice( page, labelText, section, inline, overlayManager ) { + function insertBannersOrNotice( pageHTMLParser, labelText, section, inline, overlayManager ) { var $metadata, issueUrl = section === KEYWORD_ALL_SECTIONS ? '#/issues/' + KEYWORD_ALL_SECTIONS : '#/issues/' + section, @@ -44,10 +44,10 @@ issueSummaries = []; if ( section === KEYWORD_ALL_SECTIONS ) { - $metadata = page.$el.find( selector ); + $metadata = pageHTMLParser.$el.find( selector ); } else { // find heading associated with the section - $metadata = page.findChildInSectionLead( parseInt( section, 10 ), selector ); + $metadata = pageHTMLParser.findChildInSectionLead( parseInt( section, 10 ), selector ); } // clean it up a little $metadata.find( '.NavFrame' ).remove(); @@ -118,9 +118,9 @@ * that opens them in a mobile friendly overlay. * @ignore * @param {OverlayManager} overlayManager - * @param {Page} page + * @param {PageHTMLParser} pageHTMLParser */ - function initPageIssues( overlayManager, page ) { + function initPageIssues( overlayManager, pageHTMLParser ) { var section, /** @type {IssueSummary[]} */ @@ -128,7 +128,7 @@ /** @type {IssueSummaryMap} */ allIssues = {}, label, - $lead = page.getLeadSectionElement(), + $lead = pageHTMLParser.getLeadSectionElement(), issueOverlayShowAll = CURRENT_NS === NS_CATEGORY || CURRENT_NS === NS_TALK || !$lead, inline = newTreatmentEnabled && CURRENT_NS === 0; @@ -142,7 +142,7 @@ if ( CURRENT_NS === NS_TALK || CURRENT_NS === NS_CATEGORY ) { section = KEYWORD_ALL_SECTIONS; // e.g. Template:English variant category; Template:WikiProject - issueSummaries = insertBannersOrNotice( page, mw.msg( 'mobile-frontend-meta-data-issues-header-talk' ), + issueSummaries = insertBannersOrNotice( pageHTMLParser, mw.msg( 'mobile-frontend-meta-data-issues-header-talk' ), section, inline, overlayManager ).issueSummaries; allIssues[ section ] = issueSummaries; } else if ( CURRENT_NS === NS_MAIN ) { @@ -150,35 +150,38 @@ if ( issueOverlayShowAll ) { section = KEYWORD_ALL_SECTIONS; issueSummaries = insertBannersOrNotice( - page, label, section, inline, overlayManager + pageHTMLParser, label, section, inline, overlayManager ).issueSummaries; allIssues[ section ] = issueSummaries; } else { // parse lead section = '0'; issueSummaries = insertBannersOrNotice( - page, label, section, inline, overlayManager + pageHTMLParser, label, section, inline, overlayManager ).issueSummaries; allIssues[ section ] = issueSummaries; if ( newTreatmentEnabled ) { // parse other sections but only in group B. In treatment A no issues are shown // for sections. - page.$el.find( Page.HEADING_SELECTOR ).each( function ( i, headingEl ) { - var $headingEl = $( headingEl ), - sectionNum = $headingEl.find( '.edit-page' ).data( 'section' ); + pageHTMLParser.$el.find( PageHTMLParser.HEADING_SELECTOR ).each( + function ( i, headingEl ) { + var $headingEl = $( headingEl ), + sectionNum = $headingEl.find( '.edit-page' ).data( 'section' ); - // Note certain headings matched using Page.HEADING_SELECTOR may not be - // headings and will not have a edit link. E.g. table of contents. - if ( sectionNum ) { - // Render banner for sectionNum associated with headingEl inside - // Page - section = sectionNum.toString(); - issueSummaries = insertBannersOrNotice( - page, label, section, inline, overlayManager - ).issueSummaries; - allIssues[ section ] = issueSummaries; + // Note certain headings matched using + // PageHTMLParser.HEADING_SELECTOR may not be headings and will + // not have a edit link. E.g. table of contents. + if ( sectionNum ) { + // Render banner for sectionNum associated with headingEl inside + // Page + section = sectionNum.toString(); + issueSummaries = insertBannersOrNotice( + pageHTMLParser, label, section, inline, overlayManager + ).issueSummaries; + allIssues[ section ] = issueSummaries; + } } - } ); + ); } } } diff --git a/resources/skins.minerva.scripts/references.js b/resources/skins.minerva.scripts/references.js index 8b5c551..0cef127 100644 --- a/resources/skins.minerva.scripts/references.js +++ b/resources/skins.minerva.scripts/references.js @@ -1,9 +1,9 @@ ( function ( M ) { var drawer, - skin = M.require( 'skins.minerva.scripts/skin' ), - page = M.getCurrentPage(), router = require( 'mediawiki.router' ), mobile = M.require( 'mobile.startup' ), + currentPage = mobile.currentPage(), + currentPageHTMLParser = mobile.currentPageHTMLParser(), ReferencesGateway = mobile.ReferencesGateway, ReferencesMobileViewGateway = mobile.ReferencesMobileViewGateway, referencesMobileViewGateway = ReferencesMobileViewGateway.getSingleton(), @@ -36,9 +36,8 @@ * @ignore * @param {JQuery.Event} ev Click event of the reference element * @param {ReferencesDrawer} drawer to show the reference in - * @param {Page} page */ - function showReference( ev, drawer, page ) { + function showReference( ev, drawer ) { var urlComponents, result, $dest = $( ev.currentTarget ), href = $dest.attr( 'href' ); @@ -51,7 +50,7 @@ if ( urlComponents.length > 1 ) { href = '#' + urlComponents[ 1 ]; } - result = drawer.showReference( href, page, $dest.text() ); + result = drawer.showReference( href, currentPage, $dest.text(), currentPageHTMLParser ); // Previously showReference method returns nothing so we check its truthy // Can be removed when I5a7b23f60722eb5017a85c68f38844dd460f8b63 is merged. if ( result ) { @@ -85,29 +84,13 @@ if ( !drawer ) { drawer = referenceDrawerFactory(); } - showReference( ev, drawer, ev.data.page ); + showReference( ev, drawer ); } - /** - * Make references clickable and show a drawer when clicked on. - * @ignore - * @param {Page} page - */ - function setup( page ) { - var $refs = page.$el.find( 'sup.reference a' ); - - if ( $refs.length ) { - $refs - .off( 'click.references' ) - .on( 'click.references', { - page: page - }, onClickReference ); - } + function init() { + // Make references clickable and show a drawer when clicked on. + mobile.util.getDocument().on( 'click', 'sup.reference a', onClickReference ); } - setup( page ); - // When references are lazy loaded you'll want to take care of nested references - skin.on( 'references-loaded', function ( page ) { - setup( page ); - } ); + init(); }( mw.mobileFrontend ) ); diff --git a/resources/skins.minerva.scripts/toc.js b/resources/skins.minerva.scripts/toc.js index a625a6f..fbe665c 100644 --- a/resources/skins.minerva.scripts/toc.js +++ b/resources/skins.minerva.scripts/toc.js @@ -1,5 +1,6 @@ ( function ( M ) { var mobile = M.require( 'mobile.startup' ), + currentPage = mobile.currentPage(), Toggler = mobile.Toggler, TableOfContents = mobile.toc.TableOfContents, eventBus = mobile.eventBusSingleton, @@ -40,7 +41,7 @@ // add a ToC only for "view" action (user is reading a page) // provided a table of contents placeholder has been rendered if ( mw.config.get( 'wgAction' ) === 'view' && $toc.length > 0 ) { - init( M.getCurrentPage() ); + init( currentPage ); } }( mw.mobileFrontend ) ); diff --git a/resources/skins.minerva.talk/init.js b/resources/skins.minerva.talk/init.js index c768154..32ec306 100644 --- a/resources/skins.minerva.talk/init.js +++ b/resources/skins.minerva.talk/init.js @@ -49,9 +49,9 @@ talkOptions = { api: api, title: title, - // T184273 using `getCurrentPage` because 'wgPageName' + // T184273 using `currentPage` because 'wgPageName' // contains underscores instead of spaces. - currentPageTitle: M.getCurrentPage().title, + currentPageTitle: mobile.currentPage().title, licenseMsg: skin.getLicenseMsg(), eventBus: eventBus, id: id diff --git a/resources/skins.minerva.toggling/init.js b/resources/skins.minerva.toggling/init.js index 4b820f0..bbdd52e 100644 --- a/resources/skins.minerva.toggling/init.js +++ b/resources/skins.minerva.toggling/init.js @@ -1,13 +1,14 @@ ( function ( M ) { var - page = M.getCurrentPage(), // eslint-disable-next-line no-jquery/no-global-selector $contentContainer = $( '#mw-content-text > .mw-parser-output' ), mobile = M.require( 'mobile.startup' ), + currentPage = mobile.currentPage(), + currentPageHTMLParser = mobile.currentPageHTMLParser(), Toggler = mobile.Toggler, eventBus = mobile.eventBusSingleton; - if ( !page.getLeadSectionElement() ) { + if ( !currentPageHTMLParser.getLeadSectionElement() ) { // Operating in desktop Minerva mode. Stop execution. (T172948) return; } @@ -46,9 +47,9 @@ // avoid this running on Watchlist if ( - !page.inNamespace( 'special' ) && + !currentPage.inNamespace( 'special' ) && mw.config.get( 'wgAction' ) === 'view' ) { - init( $contentContainer, 'content-', page ); + init( $contentContainer, 'content-', currentPage ); } }( mw.mobileFrontend ) ); diff --git a/resources/skins.minerva.watchstar/init.js b/resources/skins.minerva.watchstar/init.js index 5256dc1..a2b4877 100644 --- a/resources/skins.minerva.watchstar/init.js +++ b/resources/skins.minerva.watchstar/init.js @@ -1,6 +1,9 @@ ( function ( M ) { - var Watchstar = M.require( 'mobile.startup' ).Watchstar, + var + mobile = M.require( 'mobile.startup' ), + currentPage = mobile.currentPage(), + Watchstar = mobile.Watchstar, user = mw.user; /** @@ -24,6 +27,6 @@ } ); } } - init( M.getCurrentPage() ); + init( currentPage ); }( mw.mobileFrontend ) ); diff --git a/tests/qunit/skins.minerva.scripts/pageIssues.test.js b/tests/qunit/skins.minerva.scripts/pageIssues.test.js index 34f0ffb..2be6c6a 100644 --- a/tests/qunit/skins.minerva.scripts/pageIssues.test.js +++ b/tests/qunit/skins.minerva.scripts/pageIssues.test.js @@ -4,7 +4,7 @@ pageIssues = M.require( 'skins.minerva.scripts/pageIssues' ), insertBannersOrNotice = pageIssues.test.insertBannersOrNotice, OverlayManager = mobile.OverlayManager, - Page = mobile.Page, + PageHTMLParser = mobile.PageHTMLParser, overlayManager = OverlayManager.getSingleton(), $mockContainer = $( '
' + @@ -19,7 +19,7 @@ inline = true, SECTION = '0', processedAmbox = insertBannersOrNotice( - new Page( { el: $mockContainer } ), + new PageHTMLParser( $mockContainer ), labelText, SECTION, inline, overlayManager ).ambox;