Page MenuHomePhabricator

MobileDiff with inconsistent linebreaks within words
Closed, ResolvedPublic0 Estimated Story Points

Description

Since a few days (?) there are linebreaks/wordwrap within single words which make the diff view unreadable.

Example: https://de.m.wikipedia.org/wiki/Special:Mobilediff/167609483

EDIT: This behaviour is inconsistent. Generally, in Minerva word-wrap:break-word is used to avoid overflow of long lines. That would work fine here as well, but for some reasons MobileDiff uses a strange combination of hyphens:auto (mw-mf-diffarea; generally fine, though there could be some misunderstandings about added/deleted hyphens or dashes in the content) and word-wrap:break-all. I do not see any good reason to use word-wrap:break-all, as it will unnecessarily justify the content of the diff while making the text very difficult to read. The simple and rather obvious solution to this would be to use

#mw-mf-diffview .mw-diff-inline-added ins, #mw-mf-diffview .mw-diff-inline-changed ins, #mw-mf-diffview .mw-diff-inline-deleted del, #mw-mf-diffview .mw-diff-inline-changed del { word-wrap: break-word; }

(instead of the current word-wrap: break-all;). Ideally, I suggest to make it consistent with the rest of the diff (thus, either use hyphens:auto; for the entire area or only use word-wrap: break-word;).

EDIT 2: Chrome does not support word-wrap or hyphens:auto. Instead, word-break:break-word needs to be used.

QA steps

Shrink window and make sure the word Benutzer Diskussion:Andif1|Diskussion breaks after the word "Benutzer" not within the second word.

https://de.m.wikipedia.org/wiki/Special:Mobilediff/167609483
https://en.m.wikipedia.beta.wmflabs.org/wiki/Special:MobileDiff/530215

QA Results -Beta

ACStatusDetails
1T171726#7579305

QA Results - Prod

ACStatusDetails
1T171726#7579307 and T171726#7630640

Related Objects

StatusSubtypeAssignedTask
Declineddchen
OpenNone
OpenNone
DuplicateNone
OpenFeatureNone
OpenBUG REPORTNone
OpenNone
StalledNone
OpenFeatureNone
DuplicateNone
ResolvedNone
OpenNone
OpenNone
OpenFeatureNone
OpenNone
ResolvedNone
ResolvedNone
ResolvedHalfak
OpenNone
OpenNone
OpenFeatureNone
StalledNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
ResolvedPetrb
OpenNone
OpenNone
DeclinedNone
OpenBUG REPORTNone
OpenNone
ResolvedJdlrobson

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Jdlrobson raised the priority of this task from Low to Needs Triage.Oct 25 2021, 9:36 PM

Again, why break-all? That should have never been there in the first place, anywhere else the strategy for mobile is to use either hyphens:auto or word-break. The issue here is mostly the inconsistency, even between the changed lines and the context lines; but also the lack of browser support for certain functions needed here. Right now, the changed lines need a word-break:break-word for Chrome to correctly handle the diff (again, word-wrap is not supported there).

If the recent change introduced only a word-wrap, the solution could be to simply add a word-break for Chrome at the same place.

If the recent change introduced only a word-wrap, the solution could be to simply add a word-break for Chrome at the same place.

I just suggested break-all as I believe that's the default browser style. I was wrong. It's "normal". Updated my comment.

Yes, but either way, word-wrap won’t fix it for Chrome, so I’m not sure how this addresses the most recent bug (these properties are incredibly annoying, but I have come to accept that they just need to be used in combination everywhere and all the time).

As anonymus said, word-wrap is not supported by Chrome. The correct property would be word-break. Specifically, it looks like Chrome needs word-break: break-word.

Yes, as it is done anywhere else in Minerva, as far as I am aware of. MobileDiff has had this really strange approach with break-all for some time now, so I’m not surprised that it is broken.

I please you all to fix this issue soon!! I'm using my watchlist in mobile view every day and it's really annoying to see the text in a diff view incompletely and to have to scroll also horizontally.

@Stegosaurus_Rex is it ok now ? I can't tell, it's hard to tell. I always advise anyone to include screenshots in their reports for visual problems, that is much easier than to describe things in words when things change all the time.

No, still the same. Just follow the links I posted above, eg this one (broken in both Safari and Chrome).

Got it (make sure you zoom in a lot). This is not due to hypens or break word or any of that. It's because the diff is wrapped in a table and tables adapt their size to their contents (before contents are forced to wrap).

But wasn’t it a table before already? The problem is fairly new (October).

Change 743575 had a related patch set uploaded (by TheDJ; author: TheDJ):

[mediawiki/extensions/MobileFrontend@master] Fix diff viewport overflow caused by table layout

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

Thanks for the patch! Seems good to me, but it still misses word-break:break-word for Chrome; word-wrap only works on other browsers.

But wasn’t it a table before already? The problem is fairly new (October).

I'm pretty sure it was always broken like that. That's probably why this ticket kept getting comments since 2017. it was never fixed in the right way.

word-break:break-word for Chrome; word-wrap only works on other browsers.

As far as I know, that is not true. If officially might have never supported it, but it has always worked as far as i know (overflow-wrap is just the newer name for it) and we use it all over MediaWiki, so i'm sure that if it was broken, we would hear about it (and setting it with the inspector in the desktop version works just fine)

Last time I checked, all over MediaWiki word-wrap:break-word was always used together with word-break:break-word. I do the same whenever I add this to templates on-wiki, as I use Chrome and Safari, and without the double styles it never behaves consistently between the two! There might have been a very recent change in that, so I will do some more testing, but I’m pretty sure both lines are still needed.

Positive, my browser now seems to work just fine with word-wrap! This was definitely not always the case (see eg https://stackoverflow.com/questions/22785191/chrome-paragraph-word-wrap-break-word) and in the Chrome inspector it is still marked as wrong (unlike word-break or overflow-wrap, see screenshot), but I guess the patch will work everywhere then!

Screenshot Chrome inspector word-wrap 2021-12-05.jpg (286×451 px, 27 KB)

But wasn’t it a table before already? The problem is fairly new (October).

I'm pretty sure it was always broken like that. That's probably why this ticket kept getting comments since 2017. it was never fixed in the right way.

No, this specific problem only started coming up when the font type / line-height of the mobile diff also changed, so there must have been some change in the styles of MobileDiff in October. But I guess the patch will fix it, so we don't need to explore the exact timeline.

Change 743575 merged by jenkins-bot:

[mediawiki/extensions/MobileFrontend@master] Fix diff viewport overflow caused by table layout

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

Jdlrobson triaged this task as Medium priority.Dec 6 2021, 10:54 PM
Jdlrobson set the point value for this task to 0.

Looks like this causes a regression in wmf12 on tablet/desktop:
https://en.m.wikipedia.beta.wmflabs.org/wiki/Special:MobileDiff/530215

Screen Shot 2021-12-07 at 2.28.01 PM.png (1×2 px, 258 KB)

I will either revert the patch tomorrow OR deploy a fix if one is available

Weird… this is on my iPad

CE2157F5-0E2D-4657-A41A-C9FF1DBBA7C5.jpeg (1×2 px, 597 KB)

Ah this is:

@media screen and (min-width: 720px)
.content table tbody {
    display: table-row-group;
}

making yet another override.... and for some reason the behaviour of Chrome and Safari differ with this.

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

[mediawiki/skins/MinervaNeue@master] Fixes desktop regression on diff page

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

Something like this perhaps? Good it's not impacting iPad. Only > 720px.

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

https://patchdemo.wmflabs.org/wikis/8f11c27329/w/

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

https://patchdemo.wmflabs.org/wikis/b07185b23f/w/

Change 745329 had a related patch set uploaded (by TheDJ; author: TheDJ):

[mediawiki/extensions/MobileFrontend@master] Set higher specificity

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

i was thinking more like this (untested, i can't seem to figure out how to get this mode to be active on my localhost).

Also. I'm wondering why we use bottom padding in this view instead of margins... it's causing the margin collapse with the table not to take effect, adding effectively a double 'margin' above the table.

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

https://patchdemo.wmflabs.org/wikis/f72ae6e043/w/

Change 745288 abandoned by Jdlrobson:

[mediawiki/skins/MinervaNeue@master] Fixes desktop regression on diff page

Reason:

See https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MobileFrontend/+/745329

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

Change 745329 merged by jenkins-bot:

[mediawiki/extensions/MobileFrontend@master] Set higher specificity

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

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

[mediawiki/extensions/MobileFrontend@wmf/1.38.0-wmf.12] Set higher specificity

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

Change 745241 abandoned by Jdlrobson:

[mediawiki/extensions/MobileFrontend@wmf/1.38.0-wmf.12] Set higher specificity

Reason:

I don't have time to backport this today. I checked with Olga and given views to Minerva diffs at desktop resolution are quite low in the grand scheme of things this can probably wait until next train.

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

Test Result - Beta

Status: ✅ PASS
Environment: beta
OS: macOS Monterey
Browser: Chrome
Device: MBP
Emulated Device:NA

Test Artifact(s):

QA Steps

✅ AC1: Shrink window and make sure the word Benutzer Diskussion:Andif1|Diskussion breaks after the word "Benutzer" not within the second word.

Screen Shot 2021-12-18 at 1.45.52 PM.png (992×499 px, 85 KB)

Edtadros added a subscriber: Edtadros.

Test Result - Prod

Status: ✅ PASS
Environment: dewiki
OS: macOS Monterey
Browser: Chrome
Device: MBP
Emulated Device:NA

Test Artifact(s):

QA Steps

✅ AC1: Shrink window and make sure the word Benutzer Diskussion:Andif1|Diskussion breaks after the word "Benutzer" not within the second word.

Screen Shot 2021-12-18 at 1.46.27 PM.png (990×499 px, 133 KB)

Today I found another broken MobileDiff: https://de.m.wikipedia.org/wiki/Spezial:Mobiler_Unterschied/219031629 The inserted part overflows on smaller screens (both in Chrome and in Safari) because of the long IPv6. Word-breaks are apparently not applied. I am not sure if this is still related to the fixes from this task, in any case there should be word-breaks for long lines. I have been able to fix it for myself by adding .diff td div { word-break: break-word; } to my common.css, which I find confusing, as this used to be a mere Chrome fix and shouldn't have been necessary at all; but now it works for both Chrome and Safari. Any ideas?

Hmm. you are right. It seems that word-break: break-word; has different behaviour here from overflow-wrap and word-wrap when applied to inline-blocks instead of inline elements.....

wow. wonder if this is a browser bug, or some very obscure web specification thing they'd figure no one would notice...

This also explains the confusion higher up. The inline-blocks are only used for the ins/del words, so sometimes it worked for ppl with word-wrap/overflow-wrap and sometimes only if there was also word-break (because the word to break was being added/deleted). I'm gonna add a note to MDN about this.

Change 753565 had a related patch set uploaded (by TheDJ; author: TheDJ):

[mediawiki/extensions/MobileFrontend@master] Use break-word to wrap on word boundaries inside inline-blocks

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

I've requested wikidiff2 on patchdemo so that we can collaboratively test this one and make sure we end up with the desired result:
https://github.com/MatmaRex/patchdemo/issues/414

@XanonymusX the patch from @TheDJ provides the following diffs. Could you take a look and tell me if it's an improvement? Feel free to try out other diffs on that wiki by creating/editing pages.

https://patchdemo.wmflabs.org/wikis/3596c71251/wiki/Special:MobileDiff/38
https://patchdemo.wmflabs.org/wikis/3596c71251/wiki/Special:MobileDiff/34

ovasileva added a subscriber: ovasileva.

I have tried it in Chrome and in Safari, it looks good to me! Long lines are now wrapping consistently, and only when actually needed. Thanks! Still not a fan of automatic hyphenation in the diff, but I guess it's acceptable.

Change 753565 merged by jenkins-bot:

[mediawiki/extensions/MobileFrontend@master] Use break-word to wrap on word boundaries inside inline-blocks

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

@bwang shared an image in prod where it was hyphenating in Prod. I checked a few different devices. Here's a video showing a range of dimensions and the hyphenation.

@Edtadros Your video also seems to be showing the hyphenation happening, should this task be moved to QA to prod or needs more work? I'm unsure why it seems fixed in beta but not prod

Your video also seems to be showing the hyphenation happening

@bwang I'm pretty sure this is fine. The issue originally raised here was about /when/ and /where/ the hyphenation occurs. @TheDJ and @XanonymusX can you review the video in T171726#7630640 and confirm that's working as expected?

Marking as resolved. @XanonymusX and @TheDJ please feel free to open a new task if there's any remaining issues here.

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

https://patchdemo.wmflabs.org/wikis/b07185b23f/w/

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

https://patchdemo.wmflabs.org/wikis/8f11c27329/w/

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

https://patchdemo.wmflabs.org/wikis/f72ae6e043/w/