Page MenuHomePhabricator

Fix unintended vertical spacing behavior in Minerva and Vector
Open, HighPublic

Description

Background

Followup to https://phabricator.wikimedia.org/T360917

User story

As a user of a wiki I want the whitespace to be consistent and per the intended design. As an editor the markup and spacing of the page should be implemented in an expected way.

Requirements

  • The padding-bottom solution taken in previous tasks for Minerva and Vector is replaced with the solution in the POC patch linked to this ticket

Impacted projects

All projects

Acceptance criteria

  • Spacing between paragraph and other elements is according to the design

Event Timeline

Change #1021533 had a related patch set uploaded (by Bernard Wang; author: Bernard Wang):

[mediawiki/skins/Vector@master] Update <p> spacing to only use margins, update list elements to have consistent top and bottom margins

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

Jdlrobson subscribed.

Since we're so close to branch cut let's hold off on merging this till after it has happened (@ovasileva we should also estimate this tomorrow - when we estimated it before it was a 5 so I think if we take this on this sprint we should discuss that risk).

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

Change #1025463 had a related patch set uploaded (by Bernard Wang; author: Bernard Wang):

[mediawiki/skins/MinervaNeue@master] Update <p> spacing to only use margins

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

Change #1025463 had a related patch set uploaded (by Bernard Wang; author: Bernard Wang):

[mediawiki/skins/MinervaNeue@master] Update <p> spacing to only use margins

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

Just so this isn't lost:

An issue @Jdrewniak pointed out with this approach (and technically any approach that relies on sibling selectors) is that we wont have the correct spacing when elements are floated.

Right, unfortunately p + p will immediately distort the appearance of a huge share of articles, and in a noticeable way. Just to be clear: if you apply a fix only to images, something like p + figure + p, you will soon notice that there are also floating tables, floating divs, floating divs preceded by a <link> element from TemplateStyles, groups of floating images, non-floating images, hidden elements, div-nested paragraphs, etc. etc. This is volunteers' turf, not developers'; I don't think it's feasible to find any meaningful compromise here and not find yourself (and volunteers) hunting for the endless exceptions for eternity to come.

While I was developing Convenient Discussions, floating elements were a hell of a troublemaker when I was trying to figure out the visual boundaries of users' comments (so, floating elements had to be ignored). The solution I adopted eventually was to just use an algorithm that parses each and every stylesheet existing on the page and look for float set to left or right in them. But you can't do that in pure CSS.

and technically any approach that relies on sibling selectors

Yet there are cases that are clearly anomalies (e.g. p + figure + ul when the paragraph describes the content of the list that follows) and cases that are a rule rather than an exception.

So unless you have a viable solution to this, I believe p + p { margin-top: 1em; } to be a solution of the same level of harmfulness as padding-bottom: 0.5em.

Maybe negative margins would be a better solution after all?

instead of using padding-bottom: 0.5em for p , you could use margin-bottom: 1em and a negative margin for the next sibling in exceptional cases, e.g. p + ul, p + ol { margin-top: -0.5em }, together with :first-child / :last-child pseudo-classes as required

This one I saw utilized by experienced web designers (e.g. Aegea blog engine, see an example list), but it may come with its drawbacks as well: :last-child can be a floating or hidden element just as well, which would result in p:last-child { margin-bottom: 0.5em } not being applied and 1em bottom margin for the last paragraph. But currently it seems way less invasive to me.

You have to research and think about this very carefully. Whichever solution you settle on, its consequences are gonna haunt developers and volunteers for years. Please don't rush with releasing it as you did with padding-bottom: 0.5em.

And one more consideration: Styles should be applied not as smart ways to achieve this or that visual appearance, but based on a general idea of what you're trying to do.

  • Do you want paragraphs to have 0.5em margins, except for the special case where it immediately follows another paragraph?
    • And follows visually and not in terms of the DOM – which seems unattainable with the current state of CSS.
    • But is this case really that special? You will find out that paragraphs are not the only elements that can contain text not delimited by borders (how about p + dl, p + div > p:first-child and p + table where the table has a caption instead of a head?). Or, even more importantly, that by increasing spacing between paragraphs, you have affected the general feel of Vector's design, which would necessiate changes in margins of other elements (and other margins of paragraphs) as well. This consideration seems underestimated to me. E.g. p + h3, p + h4 will have the top indentation smaller than that of paragraphs which is inconsistent.
  • Or do you want paragraphs to have a consistent greater bottom indentation, except for the special cases where it 1) is immediately followed by a list and 2) is at the end of a container?

If you start with the wrong general idea, at first it might look good, but with time everything will start falling apart.

Jdlrobson lowered the priority of this task from High to Medium.Tue, May 14, 6:30 PM
ovasileva raised the priority of this task from Medium to High.Thu, May 16, 6:19 PM