Page MenuHomePhabricator

Excessive paragraph padding in MF editor save dialog
Closed, ResolvedPublic

Description

A selector introduced a few months ago in https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/496547/ adds 1em padding to all paragraphs inside a .panel. This rule seems overly generic and leads to this in the save panel:

Before this patch the appearance was much more sensible:

Event Timeline

Esanders created this task.Aug 10 2019, 4:27 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 10 2019, 4:27 PM

Change 529472 had a related patch set uploaded (by Esanders; owner: Esanders):
[mediawiki/extensions/MobileFrontend@master] Revert "Generalise a CSS rule for paragraphs within panels"

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

Jdlrobson added a comment.EditedAug 12 2019, 3:33 PM

Why is .summary-request a p tag when it is styled more like a heading? Can we not just change the tag for an h2? If the license padding if not necessary or useful we should probably not use the panel class here and use editor-panel or editor-overlay-panel instead. It doesn't seem useful to share styles with unrelated things such as talk overlay, language overlay, beta opt in panel, category overlay and AbuseFilterPanel.

Change 531324 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Editor save panel elements should not be paragraphs

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

In both this and T217102, it looks like the desired behaviour is to have a panel with fixed padding all the way around, in which case it makes more sense to apply the padding to the whole panel, rather than each individual paragraph, regardless of if we use h2/p/div for the individual content items.

Indeed in the example given in T217102 (the talk page add-topic form), the whole panel would have padding of 12px, were it not for the fact it overridden by the rule .add-topic-form .panel { padding: 0; }. This leads me to believe that that is a special case where the padding is being moved from the containing panel to the inner elements (so that the edge-to-edge horizontal borders can be drawn), in which case the .panel > p rule should only to this particular form.

Here is the form in question:

The alignment form is a result of some inconsistently targeted selectors:

  • The three panels have no padding due to a rule targeted at .add-topic-form .panel (as mentioned aboved)
  • The license field gets a padding of 1em from .panel > p
  • The subject and message fields get a padding of 16px 1em from a .panel input, .panel textarea rule (note no child selector)
  • The borders of both fields are removed by a rules specific to this form: .add-topic-form .panel input/textarea:only-child

This should probably be cleaned up.

It appears from both naming and usage that .panel is supposed to be a generic container with padding. I don't think it makes sense to generically give paragraphs extra horizontal padding within them.

Change 531324 abandoned by Jdlrobson:
Editor save panel elements should not be paragraphs

Reason:
Folded into https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/ /531522/2

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

Change 531522 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Simplify panel styles

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

So we have three patches here:

  1. https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MobileFrontend/+/529472 Revert "Generalise a CSS rule for paragraphs within panels"
    • Supposedly fixes the issue, but it doesn't rebase cleanly, and apparently the patch being reverted is so old that it doesn't work anymore with current MediaWiki.
  2. https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MobileFrontend/+/531324 Editor save panel elements should not be paragraphs
    • Indeed fixes the issue.
  3. https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MobileFrontend/+/531522 Simplify panel styles
    • This is a superset of patch #2 that also makes tons of other changes.

I am not really comfortable merging all of the changes from #3, so I'm going to restore #2 and merge it. This shouldn't cause any problems for anyone and we'll be able to close this task faster.

Change 531324 restored by Bartosz Dziewoński:
Editor save panel elements should not be paragraphs

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

Change 529472 abandoned by Bartosz Dziewoński:
Revert "Generalise a CSS rule for paragraphs within panels"

Reason:
Superseded by https://gerrit.wikimedia.org/r/531324

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

WFM. We can open a new task for cleaning up the Panel code.

Change 531324 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Editor save panel elements should not be paragraphs

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

matmarex moved this task from Incoming to QA on the VisualEditor (Current work) board.

Change 531522 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Simplify panel styles

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

Change 531522 abandoned by Jdlrobson:
Simplify panel styles

Reason:
Too risky for now.

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

ppelberg closed this task as Resolved.Sep 20 2019, 11:02 PM
ppelberg claimed this task.
DannyS712 added a subscriber: DannyS712.

[batch] remove patch for review tag from resolved tasks