WordPress.org

Make WordPress Core

Opened 14 months ago

Closed 5 months ago

#23684 closed defect (bug) (fixed)

wp-admin CSS style conflicts with jQuery-UI breaking form elements in Firefox

Reported by: Colin84 Owned by: helen
Milestone: 3.8 Priority: high
Severity: major Version: 3.5
Component: Administration Keywords: has-patch needs-testing
Focuses: ui Cc:

Description (last modified by SergeyBiryukov)

A default CSS style in the wordpress admin interface, which is loaded through load-styles.php either under the name wp-admin or buttons (both of which are inaccessible for removal as they are not listed in the WP_Styles object) interferes with jQuery-ui in such a way that any radio buttons (and possibly other form elements) that are styled with jQuery-ui in the admin interface (such as with a plugin) cause the browser window to jump to the top of the page when clicked. This behavious occurs with the current Wordpress version 3.5.1 using the latest Firefox 19.0 on linux (haven't tested in windows yet). After spending a few hours tracking down the offending code I narrowed it down to this statement in the admin CSS:

.screen-reader-text,.screen-reader-text span,.ui-helper-hidden-accessible {
position:absolute;
left:-1000em;
top:-1000em;
height:1px;
width:1px;
overflow:hidden;
}

The .ui-helper-hidden-accessible class is what is causing the conflict, specifically the "top:-1000em;" statement; due to the negative positioning values when a button is clicked the browser tries to focus on something styled with the ui-helper-hidden-accessible class, which is of course way outside the limits of the browser window. In order to correct the error I had to override it with the following in another stylesheet declared after load-styles.php:

.screen-reader-text,.screen-reader-text span,.ui-helper-hidden-accessible {
position:fixed;
left:1em;
top:1em;
height:1px;
width:1px;
overflow:hidden;
display:none;
}

We should not have to override default styles like this when making plugins, especially when we are using a js library (jQuery-ui) that is included with wordpress. Either the ui-helper-hidden-accessible class needs to be renamed or the negative values removed and replaced with a display:none; statement to fix this bug.

Attachments (2)

css-conflicts.23684.diff (969 bytes) - added by codebykat 12 months ago.
patch to fix css conflicts #23684
css-conflicts-keeping-ui-helper-hidden-accessible.23684.diff (868 bytes) - added by codebykat 9 months ago.
Moves to clip but retains the ui-helper-hidden-accessible class.

Download all attachments as: .zip

Change History (23)

comment:1 follow-up: SergeyBiryukov14 months ago

  • Keywords ui-focus added
  • Milestone changed from Awaiting Review to 3.6
  • Version changed from 3.5.1 to 3.5

Introduced in [22285] (for #22166).

Could you provide a piece of code to reproduce the issue?

comment:2 in reply to: ↑ 1 Colin8414 months ago

Replying to SergeyBiryukov:

Introduced in [22285] (for #22166).

Could you provide a piece of code to reproduce the issue?

I have uploaded a test page which showcases the issue here -> http://colinhunt.com/wp-bug/

Scroll to the bottom and click on the radio buttons to reproduce the bug. The bug is reproducible in the latest Firefox version 19.0 for Linux. Probably in Windows too but haven't tested it there yet. When you view the source code, wp-admin-styles.css is the file with the offending wordpress styles as loaded with wp-admin/load-styles.php?c=0&dir=ltr&load=wp-admin,buttons&ver=3.5.1 in the wordpress admin sections.

comment:3 Colin8414 months ago

I have just confirmed that this bug is present and reproducible in Firefox versions 13.0.1 to 19.0 as well as in Internet Explorer 9 under Windows 7.

Last edited 14 months ago by Colin84 (previous) (diff)

comment:4 follow-up: helen14 months ago

display: none defeats the purpose of having accessibility text; please don't do that in your theme or plugin. I'd look into what jQuery UI does in their CSS themes and adjust to that, which we should also do for core.

comment:5 SergeyBiryukov14 months ago

  • Description modified (diff)

comment:6 in reply to: ↑ 4 Colin8414 months ago

Replying to helen:

display: none defeats the purpose of having accessibility text; please don't do that in your theme or plugin. I'd look into what jQuery UI does in their CSS themes and adjust to that, which we should also do for core.

OK, I've figured out that just changing the line top:-1000em; to top: auto; in the wordpress CSS file will also fix the problem, at least in Firefox. This shouldn't affect anything for accessibility and there is really no need to set the elements to left: -1000em AND top: -1000em; moving them that far left already pushes it off the screen.

codebykat12 months ago

patch to fix css conflicts #23684

comment:7 codebykat12 months ago

  • Keywords has-patch added

This seems to have been introduced in October: http://core.trac.wordpress.org/browser/trunk/wp-admin/css/wp-admin.css?rev=22285 as a fix for #22166

It looks like "display: none;" is not a valid solution here, as it would prevent the text from being read by screen-readers.

As of version 1.8.7, jQuery UI has used a clipping rectangle, not off-screen positioning, to hide content:

/trunk/wp-includes/css/jquery-ui-dialog.css:
   17: .ui-helper-hidden-accessible {
   18  	border: 0;
   19  	clip: rect(0 0 0 0);

Since I don't see anything in the commit logs to indicate we're intentionally overriding jQueryUI's behavior here, I'm leaning toward using the clipping rectangle, both for the .ui-helper-hidden-accessible class and for the .screen-reader-text classes. Have found a few articles saying the clipping rectangle does prevent just the sort of issue reported here (e.g. this one on snook.ca ).

The more cautious solution, of course, would be to simply remove our overrides of the jQueryUI class and leave .screen-reader-text as is... but I think it's worth keeping the styles consistent, unless we have a good reason not to.

Side note: There are no uses of the class "ui-helper-hidden-accessible" in the current codebase (it seems everything is using screen-reader-text), so that override will *only* affect inserted content, e.g. plugins.

Attaching a patch that updates the styles for .screen-reader-text to match jQueryUI, and removes the overrides of .ui-helper-hidden-accessible.

Last edited 12 months ago by SergeyBiryukov (previous) (diff)

comment:8 kovshenin12 months ago

I've been able to reproduce this in Firefox 20.0, here's a snippet that helps. css-conflicts.23684.diff looks fine and fixes the issue for me, though probably needs some testing in IE to make sure we don't accidentally reveal any screen reader text.

comment:9 SergeyBiryukov12 months ago

  • Keywords needs-testing added

comment:10 nacin10 months ago

  • Milestone changed from 3.6 to Future Release

.ui-helper-hidden-accessible is specifically a jQuery UI class, and we specified it in [22285] because the jQuery UI CSS is not always loaded when jQuery UI itself is loaded. So css-conflicts.23684.diff should keep it.

There is likely a storied history for why .screen-reader-text uses the technique it currently uses. I'm not against changing it, but punting on this for now.

comment:11 codebykat10 months ago

@nacin Can you explain to me why we think .ui-helper-hidden-accessible is important, even though we're not using that class anywhere? Is it a concern about plugins that might be adding stuff to the admin dashboard, and if so, shouldn't it be the plugin's responsibility to load jQuery UI CSS if it uses it?

(Honest question, I'm not being snarky. I'm new to all of this.)

The story, as far as I've been able to determine (from our commit logs and jQuery UI changelog), is that we originally copied what jQuery UI was doing, and then jQuery UI changed the way they do this (possibly because of this same bug). I would argue that we should continue following jQuery's lead on this.

comment:12 nacin10 months ago

.ui-helper-hidden-accessible is a CSS class that is actually used in jQuery UI. It was added in 1.9, and is mostly used in autocomplete. See #22166.

We added that class to our existing .screen-reader-text declaration because the structural CSS that jQuery UI expects to be loaded is not usually/always loaded on WordPress admin screens. So, for example, autocomplete is used, but wp-includes/css/jquery-ui-dialog.css is not enqueued.

I wasn't questioning the CSS properties in css-conflicts.23684.diff. (I'll let others weigh in on whether we should move to clip.) Only that .ui-helper-hidden-accessible has gone missing.

comment:13 follow-up: codebykat10 months ago

Ah, okay, that makes sense. Should I submit a diff that retains the .ui-helper-hidden-accessible class but uses the updated styles?

comment:14 in reply to: ↑ 13 helen10 months ago

Replying to codebykat:

Ah, okay, that makes sense. Should I submit a diff that retains the .ui-helper-hidden-accessible class but uses the updated styles?

Sure. I would guess that whatever jQuery UI uses is more up-to-date.

comment:15 rfgoetz9 months ago

  • Cc bob@… added

codebykat9 months ago

Moves to clip but retains the ui-helper-hidden-accessible class.

comment:16 helen6 months ago

  • Milestone changed from Future Release to 3.8

Haven't tested this yet, but we need to fix this.

comment:17 helen5 months ago

#25940 was marked as a duplicate.

comment:18 matt5 months ago

  • Priority changed from normal to high
  • Severity changed from normal to major

comment:19 SergeyBiryukov5 months ago

Should be handled along with #26107.

comment:20 helen5 months ago

Voiceover doesn't seem to read links hidden with clip values of 0 unless I set a height, which then re-introduces the behavior in #26107 - 1px seems to work, though.

Would really like some testable code (in WP, please) for this. The demo page doesn't seem to exist anymore - the dangers of having something externally hosted.

comment:21 helen5 months ago

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

In 26602:

Utilize a more widely-adopted CSS method for hiding screen reader text, as seen in projects like jQuery UI and HTML5 Boilerplate. props codebykat for the initial patch. fixes #23684, #26107.

Note: See TracTickets for help on using tickets.