Page MenuHomePhabricator

[Regression] Galleries are not displaying correctly in VisualEditor (one image per row with large gaps, rather than multiple images per row) due to wrapping CSS
Closed, ResolvedPublic1 Story Points

Description

See the user-created screenshot: http://img24.cz/images/77346777291945590436.jpg

Steps to reproduce:

  1. Open any page containing a gallery in VisualEditor, or go to Insert > More > Gallery and add one.
  1. See the mess.

Related Objects

Event Timeline

Whatamidoing-WMF raised the priority of this task from to Needs Triage.
Whatamidoing-WMF updated the task description. (Show Details)
Whatamidoing-WMF added a subscriber: Whatamidoing-WMF.
Restricted Application added subscribers: StudiesWorld, Aklapper. · View Herald TranscriptDec 15 2015, 6:55 PM
TheDJ added a subscriber: TheDJ.Dec 15 2015, 7:47 PM

This is due to: .ve-ce-documentNode { white-space: pre-wrap; }

Jdforrester-WMF renamed this task from Galleries are not displaying correctly in VisualEditor (one image per row with large gaps, rather than multiple images per row) to [Regression] Galleries are not displaying correctly in VisualEditor (one image per row with large gaps, rather than multiple images per row) due to wrapping CSS.Dec 15 2015, 8:02 PM
Jdforrester-WMF triaged this task as High priority.
Jdforrester-WMF moved this task from To Triage to TR0: Interrupt on the VisualEditor board.
Jdforrester-WMF set Security to None.
Jdforrester-WMF edited a custom field.
TheDJ added a comment.Dec 15 2015, 9:44 PM


Attaching the linked screenshot, since linked screenshots have a tendency to disappear over time :)

matmarex added a comment.EditedDec 15 2015, 10:16 PM

Interesting. The solution is just:

ul.gallery {
    white-space: normal;
}

But I'm not sure if we should add that in core, or in VE VE-MW.

Interesting. The solution is just:

ul.gallery {
    white-space: normal;
}

But I'm not sure if we should add that in core, or in VE.

Or VE-MW?

The real problem that we are applying a CSS change to the whole document that doesn't exist in read mode. We could end up playing whack-a-mole with endless extensions that exhibit the same problem.

A better solution is to target our original hack more specifically - or remove it all together.

I do think that white-space: pre-wrap is a much better solution to the problem of displaying several spaces in a row that peppering the document with &nbsp; that are stripped on save. Perhaps we could get away with only applying the style to p or something (I'm sure there are edge cases of other nodes containing text content to consider). Or perhaps we should make the result of <gallery> tags not contain newlines between <li></li> tags (which is why newlines appear with pre-wrap).

The CSS may be a neater solution but unless we can apply it to just the bits we want its a non-starter.

Change 259488 had a related patch set uploaded (by Esanders):
Move white-space: pre-wrap hack to ContentBranchNode

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

Change 259488 merged by jenkins-bot:
Move white-space:pre-wrap hack to ContentBranchNode

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

TheDJ added a comment.Dec 19 2015, 9:40 PM

Another effect of this CSS on https://en.wikipedia.org/wiki/Hockenheim?veaction=edit
As can be seen in the attached screenshots, the image map is chased outside of the infobox for some reason.

Not sure what it does icw the new patch.

TheDJ added a comment.Dec 19 2015, 9:52 PM

And another case with hlist's in this particular case inside navboxes: https://en.wikipedia.org/wiki/Liparoo,_Victoria?veaction=edit

Both these examples will be fixed by the latest patch as they aren't in paragraphs but, @matmarex, this convinces me that the CSS solution is unsustainable. I can guarantee there are inline templates and extensions out there that are getting broken by this fix. We just haven't found them yet.

It's basically impossible to interfere with generic content styles without breaking things.

Change 260432 had a related patch set uploaded (by Alex Monk):
Move white-space:pre-wrap hack to ContentBranchNode

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

Ehh… but we control the HTML on the editing surface. Can't we just .replace( /\n/g, ' ' ) in GeneratedContentNode or something to avoid this?

Change 260432 merged by jenkins-bot:
Move white-space:pre-wrap hack to ContentBranchNode

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

matmarex closed this task as Resolved.Dec 22 2015, 1:47 AM
matmarex claimed this task.

(Either way, the issues found so far should be fixed now.)

matmarex reassigned this task from matmarex to Esanders.Dec 22 2015, 1:47 AM
matmarex edited projects, added VisualEditor-MediaWiki; removed Patch-For-Review.

The fix was backported and deployed to all Wikimedia wikis today.

Catrope removed a subscriber: Catrope.Jan 15 2016, 10:41 PM