Page MenuHomePhabricator

Stray semicolons in RecentChanges, Watchlist, History and Contributions interface
Open, HighPublic5 Story Points

Description

See also: https://en.wikipedia.org/wiki/Wikipedia:Village_pump_(technical)#Problems_with_separators_and_semicolons_on_RecentChanges,_Watchlist,_History_and_Contributions

Visiting https://en.m.wikipedia.beta.wmflabs.org/wiki/Special:RecentChanges?hidebots=1&hidecategorization=1&limit=50&days=7&enhanced=1&urlversion=2


The semi colon only makes sense in Vector where all elements are inline but not in Minerva.

Instead of using a hardcoded semi colon, let's use

.mw-title:after { content: '; '; }

QA steps

QA Results

ACStatusDetails
1T233649#5646681
2T233649#5646681

QA Results (Nov 16, 2019)

Related Objects

Event Timeline

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

Instance of multiple extra semicolon/spaces appearing: history view that include a deleted version:

Even the one semicolon that appears in history view without deleted versions shouldn't be there.

Also in user contribs.

matej_suchanek renamed this task from [AMC] Stray semicolon in RecentChanges, Watchlist. History and Contributions interface to [AMC] Stray semicolon in RecentChanges, Watchlist, History and Contributions interface.Fri, Nov 8, 9:13 AM
matej_suchanek added a project: Regression.

Perhaps it is just enough to limit the selector to .mw-changeslist .mw-changeslist-date:before.

Wargo added a subscriber: Wargo.Fri, Nov 8, 11:00 AM
Huji updated the task description. (Show Details)Fri, Nov 8, 1:08 PM
Zoranzoki21 triaged this task as Normal priority.Fri, Nov 8, 1:55 PM

Can reproduce on srwiki:

Jdlrobson added a comment.EditedFri, Nov 8, 7:30 PM

@matmarex I can't find where you said this but I noticed your comment (whereever it was!) about semicolon-separator in some cases having an extraneous trailing space. I think having the space in the message is a symptom of bad separation between the presentation and content.
Special:LinksHere seems to be the only other user of the semicolon-separator. I'd like this page to also use pseudo elements in future, so I think the extraneous space needs to be removed in translations rather than from the CSS. It should not be part of this message.

Jdlrobson updated the task description. (Show Details)Fri, Nov 8, 7:35 PM
Jdlrobson updated the task description. (Show Details)

Change 549919 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Remove extraneous semicolons on the history page and DOCUMENT!

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

We'll need to run through the QA again once this latest patch has been merged so I've unchecked the boxes in the description.

Nirmos added a subscriber: Nirmos.Sat, Nov 9, 3:33 AM

regarding the checklist: "on watchlist page there should be a semicolon after title (pending better understanding on wiki)" - this was one of the complaints of "something broken" where this was not expected/wanted

Melos added a subscriber: Melos.Sat, Nov 9, 12:58 PM

How can this be a dupe, @Huji, if what I reported happens since aa9b5f17107c2c8b30b0765c82bd2a8019f2b935, which was deployed yesterday 0:00 UTC?

How can this be a dupe, @Huji, if what I reported happens since aa9b5f17107c2c8b30b0765c82bd2a8019f2b935, which was deployed yesterday 0:00 UTC?

I was attempting to close it as a duplicate too. Better to consolidate effort here. The QA would make sure it is anywhere it supposed to be and does not appear where it should not.

Ammarpad updated the task description. (Show Details)Sat, Nov 9, 4:53 PM
Huji added a comment.Sat, Nov 9, 4:53 PM

@Urbanecm we could keep it separate, and treat it as a side effect of this task, but like Ammarpad, I think it is best to keep the discussion consolidated in one task. In an effort to fix something on this task, a lot of regressions have been introduced. It is best to discuss them all here (and keep updating the QA steps in the task definition.

Ammarpad updated the task description. (Show Details)Sat, Nov 9, 4:54 PM

Okay, that makes sense.

@matmarex I can't find where you said this but I noticed your comment (whereever it was!) about semicolon-separator in some cases having an extraneous trailing space. I think having the space in the message is a symptom of bad separation between the presentation and content.

What there is presentation? Japanese and Chinese don't use space after (full-width) semicolon, and French seems to want space also before: https://translatewiki.net/wiki/Special:Translations?message=Semicolon-separator&namespace=8

Huji added a comment.Sat, Nov 9, 6:21 PM

FYI, the latest state we are in results in the addition of a Persian semicolon (right one) before (i.e. to the right of) the date string (wrong place) on fawiki, and an unnecessary space is also added after it:

Can we just get this last update pulled back - it obviously needs more discussion.

Fito added a subscriber: Fito.Sat, Nov 9, 10:46 PM
Draceane removed a subscriber: Draceane.Sun, Nov 10, 9:16 PM
Jdlrobson updated the task description. (Show Details)Mon, Nov 11, 4:41 PM
Jdlrobson updated the task description. (Show Details)Mon, Nov 11, 5:03 PM
Jdlrobson updated the task description. (Show Details)Mon, Nov 11, 5:07 PM

@matmarex I can't find where you said this but I noticed your comment (whereever it was!) about semicolon-separator in some cases having an extraneous trailing space. I think having the space in the message is a symptom of bad separation between the presentation and content.

What there is presentation? Japanese and Chinese don't use space after (full-width) semicolon, and French seems to want space also before: https://translatewiki.net/wiki/Special:Translations?message=Semicolon-separator&namespace=8

I'm not sure what you are asking here. Could you rephrase?
It's perfectly fine to have a space in the translated message. If there are more than one spaces they will be collapsed down to one. What I'm saying is that if you don't have a space the UI should separate the different bits of information in a watchlist/recent changes row.

I've tested https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/549919 pretty extensively and I'm pretty sure this covers all the edge cases we've highlighted. I've updated the QA steps and have been relying on these to confirm the new patch works as expected - please let me know if any edge cases are missing from those QA steps.

Jdlrobson raised the priority of this task from Normal to High.Mon, Nov 11, 5:15 PM
Pols12 added a subscriber: Pols12.Mon, Nov 11, 6:31 PM

Note on Special:Contributions with Vector, .mw-title class is got by Flow Discussion page title.

example:

04:09, 10 November 2019 (diff | hist) . . +26‎ . . Test ''topic'' - 20191110040911 on Talk:Flow QA;

This semicolon is unexpected; at least, it should not be linked.

Jdlrobson updated the task description. (Show Details)Mon, Nov 11, 8:52 PM

Regarding "but doing so makes the page inconsistent with the recent changes page " - wasn't this also just added? The semicolon is unnecessary on this page as well. And also it doesn't seem to be "after the title" but actually "before the date" which makes even less sense to precede a timestamp with "; ". These elements are already distinct.

And the QA steps 1&2 appear to contradict:
"The semicolon should not appear on Minerva RecentChanges"
followed by
" The semicolon should appear after any page title on Minerva RecentChanges"

How can these possibly both be true? Please take all this extra unwanted semicolons and toss them.

Xaosflux renamed this task from [AMC] Stray semicolon in RecentChanges, Watchlist, History and Contributions interface to [AMC] [And everything else] Stray semicolon in RecentChanges, Watchlist, History and Contributions interface.Tue, Nov 12, 2:15 AM
Xaosflux renamed this task from [AMC] [And everything else] Stray semicolon in RecentChanges, Watchlist, History and Contributions interface to Stray semicolons in RecentChanges, Watchlist, History and Contributions interface.
Jdlrobson updated the task description. (Show Details)Tue, Nov 12, 2:22 AM
Jdlrobson added a comment.EditedTue, Nov 12, 2:25 AM

But doing so makes the page inconsistent with the recent changes page

The semicolon on recent changes has always been in the interface. You can check out an old version of MediaWiki to confirm this.

And the QA steps 1&2 appear to contradict:

Thanks for pointing this out. The URL was correct but not the link label.

Please take all this extra unwanted semicolons and toss them.

@Xaosflux the ultimate goal here is to give a better experience to mobile users who have been at a disadvantage for many years. I understand it's frustrating but dealing with code that hasn't been touched in almost a decade and that lacks documentation of how it's supposed to work sometimes results in unexpected consequences. I'm trying my very best to resolve all these issues and improve the code for everyone. I appreciate your patience and understanding.

@Jdlrobson - the goal seems fine, but it has spilled completely outside of the "mobile user" UX here, if this was contained to mobile most of this discussion wouldn't be ongoing.

Zoranzoki21 added a comment.EditedTue, Nov 12, 5:05 PM

@Jdlrobson I think you can merge patch if you want.. This is screenshot from history page.

Special:Contribs look simular.

ovasileva set the point value for this task to 5.Tue, Nov 12, 6:32 PM

@ovasileva What "points" which you usually set means?

Change 549919 merged by jenkins-bot:
[mediawiki/core@master] Remove extraneous semicolons in unintended places and DOCUMENT!

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

Change 550559 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@wmf/1.35.0-wmf.5] Remove extraneous semicolons in unintended places and DOCUMENT!

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

completely outside of the "mobile user" UX here, if this was contained to mobile most of this discussion wouldn't be ongoing.

Mobile uses exactly the same HTML as desktop which is the challenge here. We've made dozens of changes in the past few months like this without disrupting desktop but I misjudged this one and unfortunately it didn't quite go too plan....!
Anyway It happens... but I'm afraid we will have to get more comfortable with these kind of short-term frustrations if we want to move the site forward technologically.

In terms of the fix....

The fix is up on the beta cluster now and I've run through the test cases and I'm not seeing any semicolons where they didn't use to be. If you have time, I'd really appreciate any editors checking the links in the description that it behaves as expected and super thanks in advance to @Zoranzoki21 for taking care of the SWAT. You are a star! <3

Zoranzoki21 updated the task description. (Show Details)Tue, Nov 12, 10:11 PM

completely outside of the "mobile user" UX here, if this was contained to mobile most of this discussion wouldn't be ongoing.

Mobile uses exactly the same HTML as desktop which is the challenge here. We've made dozens of changes in the past few months like this without disrupting desktop but I misjudged this one and unfortunately it didn't quite go too plan....!
Anyway It happens... but I'm afraid we will have to get more comfortable with these kind of short-term frustrations if we want to move the site forward technologically.

In terms of the fix....

The fix is up on the beta cluster now and I've run through the test cases and I'm not seeing any semicolons where they didn't use to be. If you have time, I'd really appreciate any editors checking the links in the description that it behaves as expected and super thanks in advance to @Zoranzoki21 for taking care of the SWAT. You are a star! <3

Oh thank you! Sure, I will do everything what I can to make WMF projects be good place. I checked too, everything looks good now.

Change 550559 merged by jenkins-bot:
[mediawiki/core@wmf/1.35.0-wmf.5] Remove extraneous semicolons in unintended places and DOCUMENT!

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

Mentioned in SAL (#wikimedia-operations) [2019-11-12T23:49:43Z] <catrope@deploy1001> Synchronized php-1.35.0-wmf.5/includes/changes/ChangesList.php: Remove extraneous semicolons (T233649), part 1 (duration: 00m 53s)

Mentioned in SAL (#wikimedia-operations) [2019-11-12T23:50:59Z] <catrope@deploy1001> Synchronized php-1.35.0-wmf.5/resources/src/mediawiki.interface.helpers.styles.less: Remove extraneous semicolons (T233649), part 2 (duration: 00m 52s)

The unexpected semicolons appear to now be gone from:
Special:Contributions
Page History

Are still on:
Special:RecentChanges
Special:Watchlist

Was there a reason these are needed on the last two pages ever discovered?

Those semi colons have always been on Watchlist and RecentChanges pages (you can visit older wikis to confirm). It's not clear if they are needed or were ever needed and if needed why they are semicolons and not one of the other visual separators.

I added some documentation in the code explaining that at a future date we might want to remove them altogether but in my opinion that should be part of a larger design review on desktop.

@ovasileva What "points" which you usually set means?

It's a general estimate of the relative risk associated to each task - it gives us a rough idea of difficulty and an even more vague idea of how long sometime might potentially take.

The unexpected semicolons appear to now be gone from:
Special:Contributions
Page History
Are still on:
Special:RecentChanges
Special:Watchlist
Was there a reason these are needed on the last two pages ever discovered?

I can't reproduce semicolon problems on Special:RecentChanges and Special:Watchlist...

Special:Watchlist

Special:RecentChanges

Jdlrobson reassigned this task from Jdlrobson to Edtadros.Wed, Nov 13, 6:23 PM

[off topic]

What "points" which you usually set means?

@Zoranzoki21: See Kanban and Scrum on https://www.mediawiki.org/wiki/Phabricator/Project_management (and please ask general questions that are unrelated to the specific topic of a task in support channels instead - thanks! :) )

[off topic]

What "points" which you usually set means?

@Zoranzoki21: See Kanban and Scrum on https://www.mediawiki.org/wiki/Phabricator/Project_management (and please ask general questions that are unrelated to the specific topic of a task in support channels instead - thanks! :) )

I got reply already, thank you!

Edtadros reassigned this task from Edtadros to Jdlrobson.Sun, Nov 17, 3:03 AM
Edtadros added a subscriber: Edtadros.

=== Test Result

Status: ❌ FAIL
OS: macOS Mojave
Browser: Chrome
Device: MBP
Emulated Device: iPhoneX

Test Artifact(s):

QA steps

✅ AC1: The semicolon should not appear on Minerva RecentChanges

❌ AC2: The semicolon should appear after any page title on Vector RecentChanges e.g. "(diff | hist) .. Page title; <time>"
No semicolons appear

❌ AC3: The semicolon should appear after any log entry e.g. "(Move log)" and outside the link Minerva RecentChanges e.g. "(Move log); <time>"
No semicolons appear

❌ AC4: Confirm on RecentChanges in Farsi language on Vector (uselang=fa) that the semicolon is translated.
No semicolons appear

✅ AC5: on history page in Vector there should be no semicolons

❓ AC6: make sure you are able to view suppressed revisions (ask for permission if you can't). Confirm that there are no semicolons on a history page where you are an admin and a revision has been suppressed by an admin - test url
I am uncertain what a suppressed revision looks like. Here is what I see.

✅ AC7: the history page for Flow should have no semicolons in the interface anywhere.

❌ AC8: on watchlist page there should be a semicolon after title (pending better understanding on wiki - https://en.wikipedia.org/wiki/Wikipedia:Village_pump_(technical)#Problems_with_separators_and_semicolons_on_RecentChanges,_Watchlist,_History_and_Contributions - one user so far has requested not to display the semicolon on the watchlist which wasn't there before, but doing so makes the page inconsistent with the recent changes page and adds additional maintenance cost. Treating this differently will require additional documentation so unless this need is explained better we will default to keeping it)
I wasn't sure if I was supposed to check the watchlist on beta or the link above. Both had no semicolons. Only the watchlist on beta is shown.

❓AC9: on Special:Contribs in Vector there is no semicolon after the text (del/undel) (for privileged users) originally reported in T237817. If you cannot see "(del/undel) " please ask someone to give you that permission.
I need permissions to see (del/undel).

✅ AC10: on Contributions page there is no semi colon after the titles of Flow topics e.g. eg. Talk:Flow QA

Edtadros updated the task description. (Show Details)Sun, Nov 17, 3:07 AM