[go: up one dir, main page]

Page MenuHomePhabricator

Real time preview mixes up the transclusion list
Closed, ResolvedPublicBUG REPORT

Description

Steps to replicate the issue:

  • Have real time preview on
  • Go to edit a page with, say, a couple dozen transcluded pages, say World of Warcraft

What happens?:
The list of transcluded pages is jumbled up in non-alphabetical order

image.png (895×476 px, 76 KB)

(Also reproducible by turning RTPV on after starting the edit, and you'll even seen the list flicker)

What should have happened instead?: Alphabetized list. I think the precise sorting is by namespace number then alphabet, but I haven't looked closely in a bit.

image.png (936×552 px, 91 KB)

Software version: 1.41.0-wmf.3 (b2a9d4e) 10:16, 3 April 2023

Other information: Firefox 111, Win10.

Event Timeline

This was the case, but I thought we'd fixed it. The templates should be being sorted now.

When I look at the above page with RTP enabled, the template list looks like this (it looks correctly ordered to me):

Editing-World-of-Warcraft-Wikipedia.png (919×711 px, 269 KB)

I can reproduce it with safemode=1, so it's not a script or local JS (not that there is any of relevance). https://en.wikipedia.org/w/index.php?title=World_of_Warcraft&action=edit&safemode=1

Hmm nope, I still can't make it do it. :-( I wonder what the difference is! Is your first screenshot taken with non-live preview? i.e. with JS turned off? Because the difference in the header ("Templates used in this preview" templatesusedpreview vs "Pages transcluded onto the current version of this page" templatesused) seems odd as the latter isn't used in JS (as far as I can see). Is JS or API results being cached somewhere?

@GMikesell-WMF have you run into this issue?

@Samwilson I actually do too in Beta as seen from the screenshots below when toggling RTP.

OS: macOS 13.2
Browser: Chrome 112
Skin: Vector 2022
Test Link: https://en.wikipedia.beta.wmflabs.org/wiki/Beer

Non-RTP (Alphabetized list)RTP (Non-Alphabetized list)
T334441_NoRTP_Templates_Order.png (1×1 px, 254 KB)
T334441_RTP_Templates_Order.png (1×1 px, 433 KB)

Change 916157 had a related patch set uploaded (by Samwilson; author: Samwilson):

[mediawiki/core@master] preview: Maintain sort order of templates list in previews

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

It only happens (as far as I have found) when some templates need extra messages to be fetched, because fetching these takes time and the looping of the templates continues regardless. The above patch fixes this by processing the list items sequentially.

Change 916157 merged by jenkins-bot:

[mediawiki/core@master] preview: Maintain sort order of templates list in previews

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

When testing the patch, I've noticed that the sort order is also sometimes incorrect when there are transclusions from multiple namespaces, e.g. 'Template' and 'Module' (this is also apparent on the World of Warcraft article). This is a separate issue from the one you identified (and fixed).

I think this is a bug in the comparison function. The current code is:

			templatesAllInfo.sort( function ( t1, t2 ) {
				// Compare titles with the same rules of Title::compare() in PHP.
				return t1.title.getNamespaceId() !== t2.title.getNamespaceId() ?
					t1.title.getNamespaceId() > t2.title.getNamespaceId() :
					t1.title.getMain().localeCompare( t2.title.getMain() );
			} );

Using > seems wrong here. The callback to sort() should return -1 [or any negative number], 0 or 1 [or any positive number] depending on whether the first value is – respectively – less than, equal to or greater than the second value. But > returns only false or true, which will be converted to 0 or 1, which means that sometimes two namespaces will be treated as equal, despite being different.

The simplest fix is to use t1.title.getNamespaceId() - t2.title.getNamespaceId() instead. This only works for numbers and requires doing some math in your head to understand if you haven't ran into that before, but it's a common pattern, MDN documentation also recommends it: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort#examples

PHP has the <=> operator for this purpose, so the code in Title::compare() uses $a->getNamespace() <=> $b->getNamespace(), but that doesn't exist in JS.

Change 917411 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/core@master] preview: Fix sort order of different namespaces in templates list

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

With that fixed, I was seeing things in a reasonable order, but on a closer look, it was still different from the PHP version. Most noticeable, all uppercase letters are no longer sorted before all lowercase letters; i.e., where PHP code produces the order "Template:Cite Metacritic / Template:Cite journal / Template:Cite magazine", JS produces "Template:Cite journal / Template:Cite magazine / Template:Cite Metacritic". Special characters may also be ordered differently.

This is because localeCompare() is smart, and produces a reasonable alphabetical order in your browser's locale, while in PHP we use strcmp(), which just compares the bytes. I'm not sure if this is a bug or not. Arguably the results from the JS code are better, but they are different from the PHP results. Maybe that's okay?

(Worth noting that this also means that the sort order will depend on your browser's locale, rather than the wiki's; e.g. accented characters like "ó" may be sorted differently.)

To get the same results as PHP, we could compare the strings using < or > (there's no trick like - for numbers): t1.title.getMain() === t2.title.getMain() ? 0 : t1.title.getMain() < t2.title.getMain() ? -1 : 1. I'm not sure if this is desirable though.

I think it would be better to make the JS comparable to the PHP, and then explore changing/improving both later.

(I think I would consider the JS sort order noted above better (might take some getting used to), but we're definitely in the "what happens on other languages (fa, zh, he)? +- what happens with mixed language templates ("Module:Citation/CS1" with "Template:fa_cite_web")" category, and what those do in those scenarios will likely differ in more dramatic ways.)

Change 917411 merged by jenkins-bot:

[mediawiki/core@master] preview: Fix sort order of different namespaces in templates list

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

Thanks for the namespace fix. For the page names, it does sound like we should avoid localeCompare() for now, rather than switching the sorting used in PHP.

I'd thought that maybe we use the following in TemplatesOnThisPageFormatter:

$collator = collator_create( $this->context->getLanguage()->getCode() );
collator_sort( $collator, $templates );

But I don't think it's worth sorting differently here to everywhere else page titles are compared.

This comment was removed by Nardog.

Change 917435 had a related patch set uploaded (by Samwilson; author: Samwilson):

[mediawiki/core@master] preview: fix template list sorting

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

Change 917435 merged by jenkins-bot:

[mediawiki/core@master] preview: fix template list sorting

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

Sorting is now working as it should, I think, and this is ready for QA.

Testing notes: test with more than 50 templates, and some of them with custom restriction levels applied.

@Samwilson It seemed like they were in order but as you see in the screenshots below there are a few that are out of place for some reason for Templates and Modules. There was also the same order if it was RTP or Non-RTP.

OS: macOS 13.3
Browser: Chrome 112
Test Link: https://en.wikipedia.beta.wmflabs.org/w/index.php?title=World_of_Warcraft&action=edit

Templates - In RTP and Non-RTP

T334441_RTP_TemplateOrder.png (1×970 px, 204 KB)

Modules - In RTP and Non-RTP

T334441_RTP_Modules_Order.png (426×521 px, 65 KB)

@GMikesell-WMF The new "alphabetical" order after T334441#8835576 sorts all uppercase letters before all lowercase letters (that is, "A B C… a b c…" and not just "A a B b C c…"), matching what happens in the PHP code, so I think that's the expected result.

@matmarex Oh I see, that's interesting. Thanks for that!

@Samwilson - Are these fine below with a few languages I tested it on with some in English vs their own language?

OS: macOS 13.3
Browser: Chrome 112
Test Link: https://en.wikipedia.beta.wmflabs.org/w/index.php?title=World_of_Warcraft&action=edit

Tagalog/Italiano- Established editor protected and template editor protected are still in English

T334441_NoRTP_Templates_TagalogItaliano.png (1×1 px, 326 KB)

German/Japanese- Established editor protected and template editor protected are in their respective languages

T334441_NoRTP_Templates_GermanJapanese.png (1×1 px, 364 KB)

Italicized- Just curious why these two are in italics

T334441_NoRTP_Templates_Italicise.png (843×1 px, 186 KB)

Tagalog/Italiano- Established editor protected and template editor protected are still in English
German/Japanese- Established editor protected and template editor protected are in their respective languages

Good points! This is because these messages (restriction-level-extendedconfirmed and restriction-level-templateeditor) are defined in the WikimediaMessages extension and are not yet translated in all languages (for example, in German but not Italian).

Italicized- Just curious why these two are in italics

Italic means that it's a redirect. I've no idea how people are meant to know that! :-) I've added some info to https://www.mediawiki.org/wiki/Help:Templates#Organizing_templates

@Samwilson Ok that makes sense. Thank you for the explanation! Ok with the uppercase sorting before lowercase letters and your answers from above, I don't see anything else therefore I will move this to Done. Thank you all once again!