Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#42724 closed defect (bug) (fixed)

Options Media page hides breaks on desktop

Reported by: garrett-eclipse's profile garrett-eclipse Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 4.9.5 Priority: normal
Severity: normal Version: 4.9
Component: Media Keywords: has-patch needs-testing has-screenshots
Focuses: accessibility Cc:

Description

Hello,

With 4.9 some css was introduced which is specific to the options-media.php and disables breaks on that page for desktop, and then they become visible on mobile.

This broke the layout of Easy Fancybox and potentially other plugins that added content to the options-media page and any that had breaks in their code.

Screen;
https://i.imgur.com/Ccoe40P.png

Ticket w/ Easy Fancybox - https://wordpress.org/support/topic/latest-loses-style-breaks-on-setting-page/
*Developer worked around the issue in 1.6.2

CSS of issue appears in wp-admin/css/common.css;

.options-media-php br {
	display: none;
}

@media screen and (max-width: 375px) {
	.options-media-php input[type="number"][name*="_size_"] {
		margin: 5px 0;
	}
	.options-media-php label[for*="_size_h"]:before {
		content: '';
		display: block;
	}
	.options-media-php br {
		display: block;
	}
}

Reference: https://github.com/WordPress/WordPress/blob/f4e974057ea98ad98b1d8f7173f35bda2e5ef1c3/wp-admin/css/common.css#L3934

I understand that's for responsive styling, but can a class of mobile-break be applied to <br> tags we want to have this css affect and then be more specific to avoid conflicts.

Appreciated,
Cheers

Attachments (9)

42724.diff (4.4 KB) - added by obenland 6 years ago.
42724.png (685.9 KB) - added by SergeyBiryukov 6 years ago.
Screenshot from @garrett-eclipse
media-settings-responsive-issue.png (143.6 KB) - added by melchoyce 6 years ago.
Schermafdruk van 2018-02-03 22-49-04.png (26.8 KB) - added by RavanH 6 years ago.
Breaks with display:block on large screens (french translation)
42724-2.diff (2.2 KB) - added by RavanH 6 years ago.
Patch proposing non-hidden line breaks and added fieldset/legend for accessibility
42724.2.diff (2.6 KB) - added by afercia 6 years ago.
42724-01,png.png (88.5 KB) - added by afercia 6 years ago.
42724-02.png (108.8 KB) - added by afercia 6 years ago.
42724-03.png (182.8 KB) - added by afercia 6 years ago.

Download all attachments as: .zip

Change History (43)

#1 @dd32
6 years ago

  • Milestone changed from Awaiting Review to 4.9.2

Introduced with [41836] / #34539 cc @melchoyce

Marking as 4.9.2 for visibility purposes only.

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


6 years ago

@obenland
6 years ago

#3 @obenland
6 years ago

  • Keywords has-patch needs-testing added

@garrett-eclipse Could you test 42724.diff to see if it fixes it for you?

@melchoyce Could you review 42724.diff to make sure it maintains what #34539 tried to achieve?

@afercia I changed the label structure here, could you let me know if this still works for a11y?

This ticket was mentioned in Slack in #core-media by garrett-eclipse. View the logs.


6 years ago

#5 @garrett-eclipse
6 years ago

  • Keywords needs-testing removed

Thanks @obenland I've tested this patch w/ the older version of Easy Fancybox that held the conflict and can see it working now. As well I didn't see any other unforeseen issues arise so I think you're good to go mate. Appreciate the patch.

@SergeyBiryukov
6 years ago

Screenshot from @garrett-eclipse

#6 @dd32
6 years ago

  • Milestone changed from 4.9.2 to 4.9.3

Bumping to 4.9.3 due to 4.9.2s release

This ticket was mentioned in Slack in #core by desrosj. View the logs.


6 years ago

#8 @afercia
6 years ago

  • Focuses accessibility added

@obenland sorry for late reply. No, that wouldn't be good for accessibility. Labels should never both use a for attribute and wrap the associated elements. Either they wrap their element or use a for/id association. And, as per the WordPress accessibility standards, new code should use a for/id association:
https://make.wordpress.org/core/handbook/best-practices/coding-standards/accessibility-coding-standards/#labeling

Existing code uses a mixture of explicitly and implicitly labeled fields, but all new code must use an explicitly associated <label> element (using for/id attributes and not wrapping the form control).

This ticket was mentioned in Slack in #core by williampatton. View the logs.


6 years ago

#10 @williampatton
6 years ago

  • Milestone changed from 4.9.3 to 4.9.4

Moving to next release milestone.

This ticket was mentioned in Slack in #core-media by mike. View the logs.


6 years ago

#12 @melchoyce
6 years ago

Latest patch also has a design issue. See media-settings-responsive-issue.png.

#13 @afercia
6 years ago

  • Keywords needs-refresh added

A few considerations:

  • The whole goal of forms.css is trying to have a consistent, reusable, set of CSS rules that can be applied everywhere. Ideally, there shouldn't be exceptions or special cases.
  • The breakpoint max-width: 375px that comes from #34539 seems a "magic number" to me. Where's the math behind it? Also, it's not used anywhere else in core. Worth reminding translations in other languages can use longer, sometimes much longer, strings and in this case the breakpoint should happen before 375 pixels.
  • Aligning labels inline with input fields is difficult. While it may be acceptable on large screens, I'd rather consider to always stack labels and fields vertically on small screens, even the ones with the small-text class (larger fields already stack).
  • The checkboxes "line wrap" issue: the current markup uses labels that wrap the checkboxes and it's basically impossible to avoid line wrapping. Using not-wrapping labels, as recommended by the Accessibility coding standards, would allow to style the checkbox and the label independently and solve the issue.

#14 @RavanH
6 years ago

Hi @afercia thanks for these considerations. Question: does the consistency/reusability rule not apply to common.css then? The rules that hide/show these break tags seem pretty much a special case as they only apply to the Settings > Media page...

IMHO, these break tags do not belong there and should not be abused like that for responsive display purposes. It should be handled as is done on other settings pages. Take for example the Settings > Reading admin page where the select fields for front and blog page are displayed as inline-block (meaning horizontally) on desktop but as block (horizontally) on small screens due to this rule:

@media screen and (max-width: 782px) {
  #profile-page .form-table textarea, .form-table span.description, .form-table td input[type=text], .form-table td input[type=password], .form-table td input[type=email], .form-table td select, .form-table td textarea {
    ...
    display: block;
    ...
  }
}

I understand this is a bit different case as it involves only one label/input pair per line, not two label/input pairs like on Settings > Media but still, a simpler more elegant solution than hidden break tags should be possible...

@RavanH
6 years ago

Breaks with display:block on large screens (french translation)

#15 @RavanH
6 years ago

Or, why not just not hide these break tags at all? What's wrong with the width and height fields showing on new lines no matter the screen size? (See attached screen shot)

It's the default on any other admin page, right?

#16 @afercia
6 years ago

@RavanH that's what I meant when I suggested these labels / fields should be "stacked vertically", at least on small screens. I don't think the <br> trick is a viable approach. Thanks for the screenshot in French :) French is one of the languages that uses longer strings here (see screenshot below) and it's clear the 375 pixels breakpoint doesn't work well with longer strings:

https://cldup.com/8M67BPfTGe.png

However, not all the small-text fields should be display: block. Some of them need to be inline with text, see for example the General Settings or the DIscussion Settings:

https://cldup.com/-VwniAVz9q.png

🙈

I think there's the need of a simple way to distinguish which fields should be stacked and which ones not, maybe a new CSS class?

Worth noting all the Settings pages show their age and they would need some refactoring. That's one of the goals of the (unofficial) Settings API Enhanced project but it's definitely out of the scope of this ticket.

#17 @dd32
6 years ago

  • Milestone changed from 4.9.4 to 4.9.5

Bumping, 4.9.4 has been released.

@RavanH
6 years ago

Patch proposing non-hidden line breaks and added fieldset/legend for accessibility

#18 @RavanH
6 years ago

Patch https://core.trac.wordpress.org/attachment/ticket/42724/42724-2.diff removes the "CSS trickery" as mentioned in (and introduced by) https://core.trac.wordpress.org/changeset/41836 with arbitrary break breakpoint at 375px, making line breaks behave as line breaks again. Also added fieldset/legend for accessibility.

Fields are now stacked vertically while still aligning horizontally with their label on all screen sizes. Consistent with other settings pages.

#19 @williampatton
6 years ago

EDIT: ignore this comment, I was looking at the old patch.

In the latest patch I see that labels are still wrapping the elements even though they have a for value. Labels should either have a for or wrap the element - but not both. Or does the addition of the filedset/legend make this a moot point?

I think the latest WP accessibility guidelines say to use the for method for labeling inputs.

Last edited 6 years ago by williampatton (previous) (diff)

This ticket was mentioned in Slack in #core-media by mike. View the logs.


6 years ago

This ticket was mentioned in Slack in #core-media by desrosj. View the logs.


6 years ago

This ticket was mentioned in Slack in #core-media by mike. View the logs.


6 years ago

This ticket was mentioned in Slack in #core by danieltj. View the logs.


6 years ago

This ticket was mentioned in Slack in #core-media by mike. View the logs.


6 years ago

#25 @kirasong
6 years ago

  • Keywords needs-testing added; needs-refresh removed

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


6 years ago

This ticket was mentioned in Slack in #core-media by afercia. View the logs.


6 years ago

#28 @afercia
6 years ago

I'd suggest to wrap the new fieldset just around the two size inputs, leaving the checkbox outside of the fieldset. This would be a bit moe correct from an accessibility perspective, because the legend "Thumbnail size" refers only to the two size fields.
I think it can also help to solve the checkbox alignment issue. Patch incoming.

@afercia
6 years ago

#29 @afercia
6 years ago

  • Keywords has-screenshots added

42724.2.diff follows the previous approach, making the pairs of label + input always stacked vertically, also on bigger screens. This introduces a visual change, but I think it aligns nicely and it's the simplest approach.

It also uses a min-width on the labels to make the inputs align vertically, to some extent. I've tested with several languages and seems to me it's good enough. If in a particular language one of these labels happen to be very long, nothing will break: the input field will just be a bit shifted on the right. The min-width approach is already used in the general settings page.

The CSS selectors target only the "size" labels and fields. Properties are simple enough. @RavanH I've tested with your plugin and haven't noticed any breakage. Please double check when you have a chance :)

The patch also adds a missing fieldset + legend (thanks @RavanH!) to wrap the two "Thumbnail size" fields, leaving the checkbox out of the fieldset. See screenshot below, in English and French.

@afercia
6 years ago

@afercia
6 years ago

@afercia
6 years ago

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


6 years ago

#31 @desrosj
6 years ago

Just wanted to note that the plugin patched the options display in version 1.6.2. When testing, make sure to use version 1.6.1 of the plugin.

@afercia 42724.2.diff works great for me. The plugin's affected version also was fixed, and after updating to their most recent version, it stayed fixed. Looks great to me!

#32 @audrasjb
6 years ago

Hello,
I can confirm this patch looks great on my side.

#33 @SergeyBiryukov
6 years ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 42864:

Media: On Media Settings screen, make the pairs of labels and inputs always stacked vertically, on both mobile and desktop screens.

Make <br /> tags on the screen behave as line breaks again after [41836]. Add a missing fieldset + legend for better accessibility.

Props afercia, RavanH, obenland, garrett-eclipse.
Fixes #42724. See #34539.

#34 @SergeyBiryukov
6 years ago

In 42865:

Media: On Media Settings screen, make the pairs of labels and inputs always stacked vertically, on both mobile and desktop screens.

Make <br /> tags on the screen behave as line breaks again after [41836]. Add a missing fieldset + legend for better accessibility.

Props afercia, RavanH, obenland, garrett-eclipse.
Merges [42864] to the 4.9 branch.
Fixes #42724. See #34539.

Note: See TracTickets for help on using tickets.