WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#26316 closed defect (bug) (fixed)

Colour Scheme picker doesn't switch with RTL

Reported by: dd32 Owned by: nacin
Milestone: 3.8 Priority: normal
Severity: major Version:
Component: RTL Keywords:
Focuses: Cc:

Description

Requires #26315 to be fixed first so that the colors RTL stylesheets are generated.

When in RTL mode, the colour scheme picker doesn't automatically switch to the new colour scheme until the next page load. Haven't tracked it down, but it appears that css_url doesn't refer to a rtl stylesheet, although that doesn't seem like the cause of this.

Attachments (1)

26316.diff (2.4 KB) - added by nacin 4 years ago.

Download all attachments as: .zip

Change History (11)

#1 @nacin
4 years ago

  • Component changed from Administration to RTL

@nacin
4 years ago

#2 @nacin
4 years ago

The issue here is that RTL is handled by the script loader, while color schemes are a separate beast. The actual URL used for the color scheme doesn't match what is registered, thanks to all of our fancy juggling. This occurs in two places: wp_style_loader_src(), to define a URL for the 'colors' "meta" handle; and in WP_Styles::do_item(), which deals with RTL stuff.

This bug also means that the color picker only previews minified color files, never the development versions. Not a big deal, though.

I attached what I was playing with to debug this. Ideally, wp_admin_css_color() should accept the handle of a registered style instead of a URL. That way we wouldn't be guessing when it came to .min.css versus .css (or .css versus .dev.css), whether it supported RTL, whether the RTL was in-addition-to or should replace it, etc. This was a mess when we moved from .dev.css to .min.css (especially since .css went from being minified to raw) and MP6 makes it worse.

Alternatively, we might need to expose some helper methods on WP_Styles that can generate a URL outside the context of doing an item. That's not fun either.

FWIW, we didn't previously have an RTL version of colors stylesheets, instead opting for some .rtl classes. This means we could simply ignore the in-addition-to or replace issue, and assume that no other colors stylesheets have an RTL stylesheet. Indeed, in 3.7, colors-fresh.css wasn't defined as an RTL stylesheet, so a color scheme wouldn't have had support for -rtl.css out of the box. This does make things a bit easier here.

We still can't blindly replace .css with -rtl.css because it will break 3rd-party color schemes, but I think 3.8 was breaking them anyway in RTL (in general, not in preview) and in other contexts anyway. I will test with the bbPress color scheme.

#3 @nacin
4 years ago

  • Severity changed from normal to major

There's enough of a mess here to classify this as fairly major. I'm going to work on this.

#4 @ocean90
4 years ago

#26455 was marked as a duplicate.

#5 @ryelle
4 years ago

  • Cc kelly.dwan@… added

#6 @nacin
4 years ago

This got complicated quickly. Integrating WP_Styles is more trouble than it is worth, at least for the moment. It is easier to think of colors stylesheets as entirely independent from WP_Styles, at which point adding support for RTL and minified CSS is as easy as checking on registration of the color schemes.

#7 @nacin
4 years ago

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

In 26780:

Admin color schemes: Manually handle RTL and minified versions of the CSS files on registration.

This bypasses WP_Styles entirely, which is much simpler for the moment, given that color schemes bypass WP_Styles for plenty already. The script loader is told to stop thinking of colors.css as an RTL-ified file. The colors-fresh handle, used directly on the login screen, needed to be (even before this commit).

fixes #26316.

#8 @nacin
4 years ago

In 26781:

Tell the script loader that colors-fresh should be treated as an RTL style, as explained (but omitted) in [26780]. see #26316.

#9 @kpdesign
4 years ago

After updating to the latest revision (r26782) this AM, I had a notice display in the dashboard at the top of the WordPress News widget:

Notice: Use of undefined constant SCRIPT_DEBUG - assumed 'SCRIPT_DEBUG' in C:\xampp\htdocs\wordpress-svn\src\wp-includes\general-template.php on line 2119

That code was introduced in 26780.

It only appeared once. I added back the rest of the widgets for the dashboard screen in Screen Options, and the notice disappeared. I've logged out and back in, and the notice does not reappear, so can't reproduce the issue.

#10 @DrewAPicture
4 years ago

In 26786:

Avoid a notice by checking that SCRIPT_DEBUG is defined before evaluating it in register_admin_color_schemes().

See #26316.

Note: See TracTickets for help on using tickets.