Page MenuHomePhabricator

Codex message box should reset margins on child nodes
Closed, ResolvedPublic3 Estimated Story PointsBUG REPORT

Description

Follow up to T326587

Vector is now using the Codex message box CSS component. It is common for messages to contain p elements (see example) and these often have default margins (in Vector 2022 styled by .vector-body p. Since messages are wrapped in mw-parser-output and these messages are often within the content area, the only way Vector 2022 can address this is by introducing a rule cdx-message__content p { margin: 0;} however it seems more useful that this is done inside Codex itself.

This may also be a problem for other elements e.g. heading tags, but I'm currently not seeing any specific examples of where those are being used.

We may also want to consider a set of Wikimedia-styles shipped inside MediaWiki core e.g. .cdx-message__content .mw-parser-output > * { margin: 0;}

Example:

Event Timeline

Would it be OK if we restricted this to the margins at the very top/bottom of the Message? For example, in the Dialog component, we zero out the top margin/padding on the first child of the dialog body, and the bottom margin/padding on the last child. I suggest we do the same thing here. That should resolve the alignment issue that you're reporting, while not messing up the internal spacing of a multi-paragraph message too much.

(Also, screenshots of the Russian Wikipedia example linked in the task description are below, for ease of access:)

image.png (354×134 px, 10 KB)
image.png (354×134 px, 13 KB)

Yeh I think that would be fine. Please note that first-child may not work here if the parser is involved as it outputs mw-parser-output. Not sure if margin collapsing would go into effect there? 🤔

Hmm you're right that that won't work in this case, because of the <div class="mw-content-ltr"> wrapper. Given that, maybe a better solution would be to add rules like .cdx-message__content .mw-content-ltr p:first-child { margin-top: 0; } in a Codex overrides stylesheet in MediaWiki somewhere?

CCiufo-WMF moved this task from Inbox to Needs Refinement on the Design-System-Team board.

Change 938293 had a related patch set uploaded (by Eric Gardner; author: Eric Gardner):

[design/codex@main] Message: Remove top and bottom margins from first/last content children

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

I've added a basic patch that zeroes out top and bottom margins/padding for first-child and last-child of the message content. I think that this is a reasonable measure to take to ensure that basic usage of the Message component (placing a paragraph or a heading element inside) doesn't lead to a wonky layout. Here are the actual styles I'm proposing:

.cdx-message__content > *:first-child {
    margin-top: 0;
    padding-top: 0;
}

.cdx-message__content > *:last-child {
    margin-bottom: 0;
    padding-bottom: 0;
}

I think that overriding .vector-body or .mw-content styles is something that should happen at the MediaWiki level, not within Codex itself.

I think we should do a few things before proceeding:

  • Research how other component libraries handle this. It doesn't look like Vuetify (or Material Design) has any kind of reset, and they just recommend using plain text rather than markup in message content.
  • Decide if these resets could cause issues for users who actually want to add margin/padding for some reason. I think we should hesitate to add styles that apply to all content in case we may be doing something that users don't expect. The resets for the Dialog header make more sense to me as an exception, since we are explicitly including an <h2> in the markup and can expect that most sites will apply padding or margin styles to that element. Since message should be succinct and don't require p or h tags, I don't think it's as critical that we add a reset.
  • Decide if these resets are worth adding specific and potentially non-performant selectors. It does look like we could target individual selectors of parser output within MediaWiki, rather than using a wildcard selector, or if we want to do this in Codex we could just target p and h tags.
  • If we determine that the overrides are needed, decide whether they belong in Codex or MediaWiki. This will depend on what our users actually need/expect and what styles we want to apply (generic or MediaWiki-specific).

If we decide not to add resets to the Codex Message component, we could instead update our docs to recommend the user of plain text in message content, emphasizing that message content is meant to be succinct (e.g. they typically should not be multiple paragraphs).

  • Decide if these resets could cause issues for users who actually want to add margin/padding for some reason. I think we should hesitate to add styles that apply to all content in case we may be doing something that users don't expect. The resets for the Dialog header make more sense to me as an exception, since we are explicitly including an <h2> in the markup and can expect that most sites will apply padding or margin styles to that element. Since message should be succinct and don't require p or h tags, I don't think it's as critical that we add a reset.

I think it's reasonable to apply the same approach that we took in the Dialog component. People might put a <p> tag or a heading tag inside of a message, and those elements might pick up global styling that could disrupt the layout of the message boxes.

In terms of performance of the selectors, what kind of impact are we talking about here? Do we mitigate any of these concerns by limiting the wildcard to first and last child? If even this kind of style rule is a problem, then do we need to reconsider the dialog styles as well?

It may be worth revisiting the Dialog styles, but I do think that the cases here are distinct: for Dialog, we aimed to remove margin and padding from any content within the dialog body, which could include things like images or whole layouts. For Message, we are only concerned about p and h tags, as far as I know, so we don't need such broad resets in this case.

It's also worth mentioning that :first-child will not be sufficient for cases of parser output in MW (see Jon's comment above). This is another reason not to assume too much when adding styles to Codex, and instead to allow applications to add their own resets or overrides.

Update: DST engineers discussed this and there were some points made that alleviate my concerns:

  1. The proposed selectors shouldn't be a performance issue
  2. We can implement general resets in Codex and MediaWiki-specific ones in MediaWiki
  3. The risk of allowing an unexpected margin/padding on a p or h tag to break the layout is probably greater than the risk of imposing reset styles that some users may not expect

I'm least certain about the third point, especially since these resets do not seem to be common in other libraries, but am willing to support the resets if we think people will generally want them.

Change 938293 merged by jenkins-bot:

[design/codex@main] Message: Remove top and bottom margins from first/last content children

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

Change 939386 had a related patch set uploaded (by Jforrester; author: Eric Gardner):

[mediawiki/core@master] Update Codex from v0.14.0 to v0.15.0

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

Change 939386 merged by jenkins-bot:

[mediawiki/core@master] Update Codex from v0.14.0 to v0.15.0

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

In addition to the horror already added by Gerrit 869274:

.cdx-message__content>* {
    font-size: var(--font-size-medium, 1rem);
    line-height: var(--line-height-medium, 1.625rem)
}

The selectors added by this change:

.cdx-message__content>*:first-child {
    margin-top: 0;
    padding-top: 0
}
.cdx-message__content>*:last-child {
    margin-bottom: 0;
    padding-bottom: 0
}

These selectors are terrible. CSS selectors are evaluated right-to-left, so this ends up evaluating every single element on the page!

Selectors have a runtime cost like anything else: universal selectors force a full DOM walk on every style recalculation, and any unrelated DOM mutation elsewhere on the page can trigger it, adding real reflow/repaint cost — especially on the large DOMs typical of wiki article pages.

And on top of that, these rulesets are currently duplicated (once as a CSS asset and once inline via <style> in the <head>), so in total there are now six "star selectors" being evaluated…