Fix spacing problem of paragraphs around lists by luixxiul · Pull Request #10305 · matrix-org/matrix-react-sdk

luixxiul · 2023-03-07T11:58:41Z

This PR intends to fix a spacing problem of paragraphs (p) around lists (ul and ol) inside .markdown-body.

	Before	After
IRC
Modern
Modern (compact): unchanged
Bubble

Signed-off-by: Suguru Hirahara luixxiul@users.noreply.github.com

type: defect

Checklist

Tests written for new code (and old code if feasible)
Linter and other CI checks pass
Sign-off given on the changes (see CONTRIBUTING.md)

Here's what your changelog entry will look like:

🐛 Bug Fixes

Fix spacing problem of paragraphs around lists (#10305). Fixes Markdown: There is a strange spacing problem when using the "list". element-hq/element-web#24602. Contributed by @luixxiul.

Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>

luixxiul · 2023-03-07T12:15:02Z

res/css/views/rooms/_EventTile.pcss

-            .mx_EventTile_content .markdown-body {
-                p,
-                ul,
-                ol,


I am not sure if the margin-bottom for ul and ol on compact modern layout has been expected, as the other layouts do not have margin-bottom for them.

luixxiul · 2023-03-07T12:24:50Z

cypress/e2e/timeline/timeline.spec.ts

+            // Check block margin values of paragraphs and lists on compact modern layout
+            cy.setSettingValue("useCompactLayout", null, SettingLevel.DEVICE, true);
+            cy.get(".mx_EventTile[data-layout=group] .markdown-body").within(() => {
+                cy.get("ul").should("have.css", "margin-block", "0px 4px");


If ul and ol margin is in fact not expected here, this needs to be replaced as well. https://github.com/matrix-org/matrix-react-sdk/pull/10305/files#r1127766737

richvdh

Thanks for looking at this. A few questions and comments below.

Generally: as I understand it, this works by setting a margin-top for <p> elements which follow a list. What is the reason for doing that, rather than setting a margin-bottom on lists, to bring them into line with the other block elements? I'm worried that with all these overrides, we're making the CSS very hard to reason about, and that there may be other situations that this won't fix.

The problematic margin-bottom override appears to have been introduced in #9871, though I don't entirely follow why it was required. As the author of that PR, @alunturner can you shed any light on it?

res/css/views/rooms/_EventTile.pcss

cypress/e2e/timeline/timeline.spec.ts

richvdh · 2023-03-08T12:04:49Z

res/css/views/rooms/_EventTile.pcss

+            :is(ul, ol) + p {
+                margin-block: $spacing-16;
+            }


this seems to be directly overriding the settings at lines 647-650. Do we really need both settings?

Are you suggesting that ul + p and ol + p would be covered by :is(blockquote > p, ol, ul) ?

Erm, I'm mostly asking the same question as in the review summary:

Generally: as I understand it, this works by setting a margin-top for <p> elements which follow a list. What is the reason for doing that, rather than setting a margin-bottom on lists, to bring them into line with the other block elements? I'm worried that with all these overrides, we're making the CSS very hard to reason about, and that there may be other situations that this won't fix.

richvdh · 2023-03-08T12:06:59Z

res/css/views/rooms/_EventTile.pcss

@@ -648,6 +648,32 @@ $left-gutter: 64px;
            margin-top: 0;


(I don't really know why we need a margin-top here. As far as I can tell, it's the default anyway)

richvdh · 2023-03-08T12:07:10Z

res/css/views/rooms/_EventTile.pcss

+            /* Paragraph below ul or ol */
+            :is(ul, ol) + p {
+                margin-block: $spacing-4;
+            }


again, how does this interact with the settings at lines 647-650?

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>

luixxiul · 2023-03-08T14:49:08Z

Closing for now as setting up a well-thought test set for lists should be done at first.

artcodespace · 2023-03-08T17:29:37Z

Hello

Thanks for looking at this. A few questions and comments below.

Generally: as I understand it, this works by setting a margin-top for <p> elements which follow a list. What is the reason for doing that, rather than setting a margin-bottom on lists, to bring them into line with the other block elements? I'm worried that with all these overrides, we're making the CSS very hard to reason about, and that there may be other situations that this won't fix.

The problematic margin-bottom override appears to have been introduced in #9871, though I don't entirely follow why it was required. As the author of that PR, @alunturner can you shed any light on it?

Hello!

This work was carried out before I was aware that we were having clashes in the timeline when trying to apply styling purely to the output of the wysiwyg composer. That specific override was put in to handle consecutive list display from the rich text editor, but if it breaks other things, I think it's sensible to be taken out. This is exactly what I did in #10071 to revert an unseen issue with paragraph spacing caused by wysiwyg work.

luixxiul added 12 commits March 7, 2023 20:41

Add margin-top to paragraph below ul or ol

39ca0e3

Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>

Add margin to the list on compact modern layout

b96df96

Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>

Sort style blocks

613040f

Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>

Ensure the same value is applied to both top and bottom margin

2525350

Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>

Edit comments

d9ebe6e

Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>

Add a test - Send lists with paragraphs as a message

0578f2c

Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>

Check block margin on modern (compact) layout

2e8156a

Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>

Check block margin on IRC layout

b34692f

Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>

Check block margin on bubble layout

c4d13ef

Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>

Use functions

951a210

Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>

Add percySnapshotElement

09f5cfd

Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>

Edit a comment

dbe57d6

Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>

github-actions bot added Z-Community-PR Issue is solved by a community member's PR T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems labels Mar 7, 2023

github-actions bot deployed to Netlify March 7, 2023 12:06 View deployment

luixxiul commented Mar 7, 2023

View reviewed changes

Merge branch 'develop' into fix-margin-list

96be6f7

luixxiul commented Mar 7, 2023

View reviewed changes

github-actions bot deployed to Netlify March 7, 2023 12:30 View deployment

luixxiul marked this pull request as ready for review March 7, 2023 12:56

luixxiul requested a review from a team as a code owner March 7, 2023 12:56

luixxiul requested review from justjanne and richvdh March 7, 2023 12:56

richvdh suggested changes Mar 8, 2023

View reviewed changes

Update cypress/e2e/timeline/timeline.spec.ts

934e752

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

github-actions bot deployed to Netlify March 8, 2023 13:48 View deployment

Merge branch 'develop' into fix-margin-list

7ac5c6c

richvdh self-requested a review March 8, 2023 14:24

Revert SCSS nesting

98adc11

Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>

github-actions bot deployed to Netlify March 8, 2023 14:25 View deployment

Merge the functions

3aef940

Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>

github-actions bot deployed to Netlify March 8, 2023 14:37 View deployment

luixxiul closed this Mar 8, 2023

Uh oh!

Conversation

luixxiul commented Mar 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

🐛 Bug Fixes

Uh oh!

luixxiul Mar 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luixxiul Mar 7, 2023

Choose a reason for hiding this comment

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

richvdh Mar 8, 2023

Choose a reason for hiding this comment

Uh oh!

luixxiul Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richvdh Mar 8, 2023

Choose a reason for hiding this comment

Uh oh!

richvdh Mar 8, 2023

Choose a reason for hiding this comment

Uh oh!

luixxiul Mar 8, 2023

Choose a reason for hiding this comment

Uh oh!

richvdh Mar 8, 2023

Choose a reason for hiding this comment

Uh oh!

luixxiul Mar 8, 2023

Choose a reason for hiding this comment

Uh oh!

luixxiul commented Mar 8, 2023

Uh oh!

artcodespace commented Mar 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

luixxiul commented Mar 7, 2023 •

edited

Loading

luixxiul Mar 7, 2023 •

edited

Loading

luixxiul Mar 8, 2023 •

edited

Loading