Page MenuHomePhabricator

Port RelatedArticles to Codex
Closed, ResolvedPublic2 Estimated Story Points

Description

Note: For night mode we would like RelatedArticles to be using Codex so it benefits from the new color scheme in mobile. Given its prominence in the mobile UI, this seems an important feature to prioritize.

This would greatly simplify the extension, address several bugs

before

Screen Shot 2021-07-16 at 3.32.50 PM.png (326×2 px, 189 KB)

Screen Shot 2021-07-16 at 3.33.39 PM.png (646×1 px, 183 KB)

after

Screen Shot 2021-07-16 at 3.33.17 PM.png (640×1 px, 116 KB)

Screen Shot 2021-07-16 at 3.32.37 PM.png (436×1 px, 110 KB)

QA

Per T317573#8363185 RelatedArticles should fill the available space.

QA Results - Beta

ACStatusDetails
1T286835#9530887

QA Results - Prod

ACStatusDetails
1T286835#9540545

Related Objects

Event Timeline

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

Hi @Krinkle are there any updates from the performance team since our Sept 15 meeting T289718#7357086 about acceptable compromises we can make here to finish up this work? In our meeting with Roan I suggested possibilities included a click to load pattern, and limiting loading on scroll to browsers where the code was already cached in localStorage.

Here's my understanding at this time:

Performance guideline

We generally don't use JS to visually change the page, especially with a layout shift (apart from modal interaction). These best practices are not WMF-specific. In 2015, after addressing a few remaining violations, we further embraced this by removing the legacy "top" queue and other forms of synchronous JS. This left RelatedArticles, CentralNotice, and some aspects of MobileFrontend (TOC, section collapse, and site CSS) as regularly causing JS-induced layout shifts. MobileFrontend has since improved.

With the current approach, we look for these areas of concern: Layout shifts, JS costs in long tasks (delay PLT, or jank), JS cost in transfer size, other transfer size costs (HTML/CSS).

Layout shifts are often a canary for excess JS costs, but RelatedArticles avoided that by using little JS and rendering a simple HTML string. Hence, it remained a low priority.

I suggested possibilities included a click to load pattern, and limiting loading on scroll to browsers where the code was already cached in localStorage.

Using the old approach, with Vue.js on top, both types of JS costs are likely added in significant amounts. This is the concern I raised during development, and led to this perf review. I agree that a click-to-load pattern would effectively hide the added costs. And that would technically meet the performance criteria, and adequately address our concern.

This approach would not be my recommendation, however, as I suspect it would create a subpar experience, and I am unsure of the described maintenance benefits. I'll explain my thinking below, for your team to take into consideration as you see fit.

Technical approach

I understand RelatedArticles, as conceived six year ago in 2015, was a conscious compromise informed partly by:

  1. .. then-current CDN configuration with its ttl/max-age at 30 days.
  2. .. assumed typical age of CDN-served content.
  3. .. MVP deadline with limited/frontend resources.

As of 2017, our CDN is capped to 24 hours. Though, we generally served relatively new copies even with a CDN ttl of 30 days, given: we purge after edits, have cascading updates, popular articles are popular, and there's finite cache capacity for infrequently accessed content. This played a large role in shortening it to 24h (T124954).

I think it is unfortunate that RelatedArticles was able to linger in this state for so long. I do understand it though, as it presumably remained unquestioned that its implementation was an unavoidable compromise. I'm hoping that, with the information available today, and management permitting the feature some attention for refurbishment, that there's excitement to do better than before?

In my estimation, it is achievable with relatively little effort to:

  • .. improve RelatedArticles's reach and accessibility, (by promoting it to Basic, rather than further limiting and delaying access via a mandatory click. This seems likely to reduce the kind of serendipity browsing the feature is known for today..)
  • .. improve latency and move towards equitable performance, (by eliminating network delays, esp per-page API requests).
  • .. improve the perf footprint, (by eliminating JS cost, rather than growing it further).
  • .. reduce maintenance cost (by building it with declarative frontend styles and HTML templates, instead of 3-5 custom RL modules and JS/PHP code.).

Perhaps a little something like this:

  • Append the HTML chunk server-side, using the same data source as today, but without extra JS in the middle to maintain and test.
  • Define minimal styles in LESS, using the same wikimedia-ui-base variables as Vue/WVUI, but through the mediawiki.theme core interface (I imagine the click button would need a similar stylesheet).
  • Define the HTML in a Mustache template. It wouldn't need a Vue-equivalent app wrapper, thus little to no duplication. WVUI and Codex are not yet ready for non-modal or server-side use at this time, so you'd very likely have to revisit this a fair bit over coming months/years while those mature. The re-use of Typeahead Element is interesting, but DRY is not an axiom. DRY is a trade-off that is sometimes appropriate, and sometimes not. Strategic copying of something that just works can greatly improve agility and reduce risk (e.g. do we need Design QA to regularly test RelatedArticles? It's quite a different form factor and visual context from Search). I suppose the "card" component could (later) become a standalone component in the Codex repository, and referenced as Mustache file via Composer. I believe this is a direction the Design Systems Team are also exploring right now, which simplifies things for teams like yours by owning and limiting this automation work inside the Codex repository.

I believe with the above your team would likely literally not have to touch the extension again for the next 5-10 years (not even to bump npm dependencies), by using stable platform features that keep RelatedArticles functional, performant, and visually on-brand.

I checked in with Alex, and the click-to-view pattern is a non-starter. So it seems this is blocked on the WVUI migration team having a solution for server-side rendering and a mechanism for loading CSS of Vue.js components on startup. I talked to @Catrope and petite-vue (T291666) might be able to help here, however we agreed that other Vue related things are more important right now, so we'll be freezing RelatedArticles indefinitely (hence moving to tracking).

Layout shifts are often a canary for excess JS costs, but RelatedArticles avoided that by using little JS and rendering a simple HTML string. Hence, it remained a low priority.

I was bit confused by the talk of layout shifts above. There is no layout shift (CMS) in related articles. There used to be, but that was resolved. Space is allocated and related articles are slotted in just as it's scrolled into view like so:

related.gif (663×351 px, 520 KB)

I just wanted to make sure that was clear.

Using the old approach, with Vue.js on top, both types of JS costs are likely added in significant amounts.

Our assumption going into this was that using WVUI/Vue.js more widely would eliminate JS cost on the long term as we grow usage of Vue.js. It also leads to greater design consistency across our product. I interpret this guidance as meaning porting RelatedArticles is blocked on having a server-side render option inside the Codex library or Vue.js usage being more widespread (which is a chicken/egg situation that I'm not sure how to resolve).

I do question whether it is preferable to server-side render a component that few users need or will see (given it's placement at the bottom of the page) and load render-blocking CSS for the Codex/WVUI card/search result component. To me, the current situation of delaying the load (but avoiding an API request) seems like the best possible outcome here from a performance perspective on the assumption that Vue.js usage becomes more widespread. The data in https://www.mediawiki.org/wiki/Reading/Web/Projects/Related_pages does not give the impression this needs to be a grade A experience.

Test wiki created on Patch demo by Jdlrobson using patch(es) linked to this task:

https://patchdemo.wmflabs.org/wikis/30047fa6f5/w/

Change 769143 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/RelatedArticles@master] Simplify the RelatedArticles extension to use Codex

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

Change 705033 abandoned by Jdlrobson:

[mediawiki/extensions/RelatedArticles@master] Simplify the RelatedArticles extension to use WVUI

Reason:

Rewritten in Codex: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/RelatedArticles/+/769143

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

@Jdlrobson FYI, in order to meet design specifications, we are removing the ListTile component and merging its contents into MenuItem (patch), and will later implement a separate Card component for use cases like this one. See this comment.

Ack. Is there any timeline about when the new Card component will be available or a process in which the web team can get that expedited? We are keen to get NearbyPages into production along with the first Codex release as we're currently maintaining two versions of Nearby.

Ack. Is there any timeline about when the new Card component will be available or a process in which the web team can get that expedited? We are keen to get NearbyPages into production along with the first Codex release as we're currently maintaining two versions of Nearby.

@Sarai-WMDE / @bmartinezcalvo, could you please give us a status update on plans for the Card component? My assumption is that this would be a fairly simple component to design and build, but I wanted to check that assumption.

We are keen to get NearbyPages into production along with the first Codex release as we're currently maintaining two versions of Nearby.

Note that using Codex in production is blocked on T302772: Application Security Review Request : Codex for now (along with the fact that Codex is still in alpha and we're still making breaking changes, like removing the ListTile component)

The patch for this has now been revised to use the new Codex Card component, bringing it in line with Codex and the design style guide.
Note: There is no layout shift with the final solution (flagged earlier as a performance concern) in this version as we reserve space for the card components to load.

It's a clear win for maintainability and provides the only path I can see to updating the feature to be in line with our style guide. The web team is not going to maintain a parallel implementation of the Codex Card component in Mustache. That's not sustainable or sensible from our perspective, even if we maintain lower level design tokens. The card component is too complicated for this to be feasible.

We have the understanding that we are at an impasse where Codex still cannot be loaded on page load, but we still do not have a clear understanding of why the loading pattern for RelatedArticles is problematic, given it occurs on an IntersectionObserver event that expresses a user action (scrolling to the bottom of the page) and given the low metrics for a user to scroll to the bottom to trigger it [2]. My understanding is click to load pattern is fine, but our designer has ruled out using a click to load pattern (it is not appropriate here). We are still lacking a decision around T248718 so I'll be asking about this ticket and how to move forward during the offsite next week!

Regarding server side rendering, I'm not convinced this is a good candidate for server-side rendering. From a product perspective we do not wish to display related articles until the user has scrolled to the bottom of the page after finishing the reading of an article (which is seen as an intention of "wanting to read more" [1]) and its click-through rates don't merit a feature that needs to be available to all users [2]. I think if we were to server-side render it, we would still add CSS to hide it (visibility: hidden) by default and reveal it on scroll.

[1] https://www.mediawiki.org/wiki/Reading/Web/Projects/Related_pages
[2] https://www.mediawiki.org/wiki/Reading/Web/Projects/Related_pages#Metrics_analysis

Test wiki on Patch demo by Jdlrobson using patch(es) linked to this task was deleted:

https://patchdemo.wmflabs.org/wikis/30047fa6f5/w/

Test wiki created on Patch demo by Jdforrester (WMF) using patch(es) linked to this task:
https://patchdemo.wmflabs.org/wikis/df71d98dab/w

Change 933201 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/RelatedArticles@master] Use ES6 templates for generating RelatedArticles

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

Jdlrobson changed the task status from Open to Stalled.Oct 3 2023, 12:21 AM

When T344386 is done, we'll be able to use Codex CSS components to finally get this over the finish line as we'll be able to load the slim additional card styles inside the JavaScript module (and avoid loading the entirety of Codex).

Change 933201 abandoned by Jdlrobson:

[mediawiki/extensions/RelatedArticles@master] Use ES6 templates for generating RelatedArticles

Reason:

Foldeed into parent

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

Jdlrobson changed the task status from Stalled to Open.EditedDec 20 2023, 1:18 AM

This is now unblocked. Using the new Codex code splitting the entire module packs in at 4kb 3.8kb.

Jdlrobson renamed this task from Port RelatedArticles to Vue.js to Port RelatedArticles to Codex.Jan 30 2024, 9:40 PM
Jdlrobson updated the task description. (Show Details)

Change 769143 merged by jenkins-bot:

[mediawiki/extensions/RelatedArticles@master] Simplify the RelatedArticles extension to use Codex CSS components

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

Jdlrobson set the point value for this task to 2.Feb 8 2024, 5:24 PM

Retroactively assigning points.

Edtadros subscribed.

Test Result - Beta

Status: ✅ PASS
Environment: beta
OS: macOS Sonoma
Browser: Chrome
Device: MBA
Emulated Device:NA

Test Artifact(s):

QA Steps

✅ AC1: The related pags image should have white space around it as per the After image in the task description

screenshot 468.png (389×1 px, 58 KB)

screenshot 467.png (844×388 px, 65 KB)

Jdlrobson claimed this task.

Test Result - Prod

Status: ✅ PASS
Environment: enwiki
OS: macOS Sonoma
Browser: Chrome
Device: MBA
Emulated Device:NA

Test Artifact(s):

QA Steps

✅ AC1: The related pags image should have white space around it as per the After image in the task description

screenshot 504.png (1×728 px, 167 KB)

screenshot 505.png (1×1 px, 208 KB)