WordPress.org

Make WordPress Core

Opened 3 months ago

Last modified 10 days ago

#42724 new defect (bug)

Options Media page hides breaks on desktop

Reported by: garrett-eclipse Owned by:
Milestone: 4.9.5 Priority: normal
Severity: normal Version: 4.9
Component: Media Keywords: has-patch needs-refresh
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 (5)

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

Download all attachments as: .zip

Change History (24)

#1 @dd32
3 months 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.


2 months ago

@obenland
2 months ago

#3 @obenland
2 months 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.


2 months ago

#5 @garrett-eclipse
2 months 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
5 weeks ago

Screenshot from @garrett-eclipse

#6 @dd32
5 weeks 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.


4 weeks ago

#8 @afercia
4 weeks 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.


3 weeks ago

#10 @williampatton
3 weeks 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.


2 weeks ago

#12 @melchoyce
2 weeks ago

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

#13 @afercia
2 weeks 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
2 weeks 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
2 weeks ago

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

#15 @RavanH
2 weeks 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
13 days 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
11 days ago

  • Milestone changed from 4.9.4 to 4.9.5

Bumping, 4.9.4 has been released.

@RavanH
10 days ago

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

#18 @RavanH
10 days 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
10 days 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 10 days ago by williampatton (previous) (diff)
Note: See TracTickets for help on using tickets.