Make WordPress Core

Opened 5 years ago

Closed 2 years ago

Last modified 2 years ago

#46600 closed defect (bug) (fixed)

Twenty Nineteen: Flex order on comment form causes contents of comment_form_after to appear above the form

Reported by: garrett-eclipse's profile garrett-eclipse Owned by: audrasjb's profile audrasjb
Milestone: 6.0 Priority: normal
Severity: normal Version: 5.0
Component: Bundled Theme Keywords: has-patch has-dev-note
Focuses: accessibility, css Cc:

Description

In the Twenty Nineteen theme the comment form design uses flexbox order CSS properties.

When contents are injected via the comment_form_after they're expected to appear below the form, for instance the Akisment privacy message. Due to the flex order the contents instead appears before the form.

Attaching screenshots

Attachments (5)

Screen Shot 2019-03-22 at 12.16.01 AM.png (77.5 KB) - added by garrett-eclipse 5 years ago.
Akismet content injected via comment_form_after appearing above comments form
Screen Shot 2019-03-22 at 12.16.24 AM.png (147.3 KB) - added by garrett-eclipse 5 years ago.
Inspect screen showing the dom and css affecting the content order.
46600.diff (1.9 KB) - added by sabernhardt 2 years ago.
creating a new container class to avoid flex order, hiding that container in print styles, moving heading before the form (in the markup)
removing-flex-order.png (677.3 KB) - added by sabernhardt 2 years ago.
46600-pr2431-override-flex.png (704.1 KB) - added by sabernhardt 2 years ago.

Download all attachments as: .zip

Change History (19)

@garrett-eclipse
5 years ago

Akismet content injected via comment_form_after appearing above comments form

@garrett-eclipse
5 years ago

Inspect screen showing the dom and css affecting the content order.

#1 @sabernhardt
3 years ago

  • Focuses accessibility css added; ui removed
  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

The order has been like this since the initial commit, so I do not understand why it was done this way. But making the DOM match the visual order is important, with or without the comment_form_after content.

Version 0, edited 3 years ago by sabernhardt (next)

@sabernhardt
2 years ago

creating a new container class to avoid flex order, hiding that container in print styles, moving heading before the form (in the markup)

#2 @sabernhardt
2 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Future Release to 6.0

Adding a message also hides the "Leave a comment" heading, and 46600.diff shows it again.

The patch uses a new class to stop using CSS flex, but any child theme with the old markup still would have the older styles. And this new container is hidden in the print stylesheet.

The patch does not match the styles perfectly when no messages are added. The 1rem top margin for the short line above the heading does not apply (it's included in the wrapper's margin), though I'm not convinced that the extra space is necessary.

Last edited 2 years ago by sabernhardt (previous) (diff)

#3 @audrasjb
2 years ago

  • Keywords dev-feedback added

@sabernhardt thanks, the patch looks great to me but I'm only wondering about replacing class comment-form-flex with comment-form-wrapper, since it would introduce a breaking change for people who customized this container. For the sake of backward compatibility, could we keep the previous class?

#4 @sabernhardt
2 years ago

  • Keywords needs-patch added; has-patch dev-feedback removed

I thought to switch classes for the structure, but did not account for several other possibilities (changing text or background color, adding a border, etc.).

It probably should add a new class and keep the existing class, which requires a bit more CSS editing.

This ticket was mentioned in PR #2431 on WordPress/wordpress-develop by sabernhardt.


2 years ago
#5

  • Keywords has-patch added; needs-patch removed

Fix the DOM order in the comment form, and add a new .comment-form-wrapper class to change flex display to block.

Trac ticket: https://core.trac.wordpress.org/ticket/46600

#6 @sabernhardt
2 years ago

  • Keywords needs-refresh added

The print styles apparently do not need editing with this way. Both .comment-form-flex and .comment-form-wrapper have the same level of specificity.

#7 @sabernhardt
2 years ago

  • Keywords needs-refresh removed

The pull request is updated, including the RTL stylesheet and correcting an HTML comment for another container.

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


2 years ago

#9 @sabernhardt
2 years ago

  • Keywords needs-testing added

#10 @audrasjb
2 years ago

  • Keywords assigned-for-commit commit needs-dev-note added

Thank you @sabernhardt for the changes.
The pull request looks good to me know.
Self assigning for commit and adding needs-dev-note keyword so this new class could be referenced on a Bundled Themes dev note or in the Misc changes dev note 📗

#11 @audrasjb
2 years ago

  • Owner set to audrasjb
  • Status changed from new to accepted

#12 @audrasjb
2 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 52993:

Twenty Nineteen: Override flex order in comment form.

This change updates Twenty Nineteen bundled theme to fix the DOM order in the comment form, and to add a new .comment-form-wrapper class to change flex display to block for this specific area.

Props garrett-eclipse, audrasjb, sabernhardt.
Fixes #46600.

#14 @sabernhardt
2 years ago

  • Keywords has-dev-note added; needs-testing assigned-for-commit commit needs-dev-note removed

The field guide mentions this change.

If someone wants to write about this in a Bundled Themes note as well, it could be worth adding how child themes that replace the parent style.css and/or comments.php would retain the flex order.

Note: See TracTickets for help on using tickets.