WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#28478 closed defect (bug) (wontfix)

remove open sans font as a dependency

Reported by: norcross Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.8
Component: Script Loader Keywords: has-patch needs-testing
Focuses: ui, administration Cc:

Description

currently open sans is set as a dependency in multiple areas on the admin and in the admin bar. this causes an issue if someone runs wp_dequeue_style on open sans, as it will remove all CSS that sets it as a dependency. since its loading as an external file, some people may want to remove it for load times or any other reasons. it's a font, therefore I would argue it's a 'nice to have' and not a true dependency.

Attachments (2)

28478.diff (4.2 KB) - added by norcross 4 years ago.
removes the open sans dependencies and enqueues separately
28478.2.diff (4.2 KB) - added by MikeHansenMe 3 years ago.
Refreshed

Download all attachments as: .zip

Change History (21)

@norcross
4 years ago

removes the open sans dependencies and enqueues separately

#1 @norcross
4 years ago

  • Keywords has-patch dev-feedback 2nd-opinion needs-testing added

#2 @SergeyBiryukov
4 years ago

  • Component changed from Appearance to Script Loader

#3 @jtsternberg
4 years ago

+1. I suspect this was setup originally as a matter of convenience, but as a matter of principle it doesn't make sense to be a required dependency for the admin bar, as the bar works just fine w/o open-sans.

#4 @camdensegal
4 years ago

I think this makes sense, while convenient to have it automatically enqueued it definitely isn't a real dependency and causes more harm than good when wp_dequeue_style is used.

#5 @celloexpressions
3 years ago

  • Version changed from trunk to 3.8

#6 @wonderboymusic
3 years ago

  • Milestone changed from Awaiting Review to 4.2

This should get an up or down vote

#7 @tyxla
3 years ago

+1 from me. I've created some custom admin themes in my experience, and most of them had an other font used as a base font, which was requiring that Open Sans is also loaded (although not used). This would definitely be a good step forward to building more flexible and lighter custom admin themes.

@MikeHansenMe
3 years ago

Refreshed

#8 @MikeHansenMe
3 years ago

+1 from me on this as well.

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


3 years ago

#10 follow-up: @nacin
3 years ago

I think this makes it harder and more annoying for people to bring in open-sans when needed. This would work, no?

wp_deregister_style( 'open-sans' );
wp_register_style( 'open-sans', false );

#11 in reply to: ↑ 10 @jtsternberg
3 years ago

Replying to nacin:

I think this makes it harder and more annoying for people to bring in open-sans when needed. This would work, no?

Can you clarify? When needed where? It will still be enqueued in wp-admin by default. wp_enqueue_style( 'open-sans' ); is all that is needed to enqueue on the front-end (or wherever).

#12 @SergeyBiryukov
3 years ago

I think Nacin's point is that it's cleaner to keep the dependency than to enqueue the script separately in each file where it might be needed (as demonstrated by the patch).

If the goal is to disable Open Sans, re-registering it with an empty URL works fine and seems like the way to go.

Last edited 3 years ago by SergeyBiryukov (previous) (diff)

#13 @norcross
3 years ago

it may be a bit cleaner to register an empty URL, but the fact remains it's still an external font being loaded and isn't a true dependency in the same way jQuery is, for example. without jQuery, the scripts would break. without Open Sans, there's a slight visual change.

#14 @norcross
3 years ago

also, (correct me if I'm wrong) isn't Open Sans the only dependency that is an externally sourced file? it seems odd that we would have something in that capacity that we cannot gaurantee it being there. while I doubt Google is going anywhere, and I know font licensing is muddy, it's still an added bit of risk.

#15 @MikeHansenMe
3 years ago

I agree with @norcross here. In addition I think this would be somewhat strange to explain to someone who wanted to de-register it. First de-register the font then you would tell them to re-register it, but with a bool false instead of a string(which the codex says is required)? All they are trying to do is remove it, telling them to add something else seems confusing.

Last edited 3 years ago by MikeHansenMe (previous) (diff)

#16 @dd32
3 years ago

Being an external dependancy is another argument entirely (which was done to death when we added it), but it's still a dependancy and registering it as such seems like the correct way to go to me (ie. as we currently do it).

28478.2.diff shows that this IS a dependancy, simply based on the locations that it then needs to be added in.

I'm personally behind the argument that re-registering it with a empty url should be the correct way of disabling it.
There's two ways to do that:

  • deregister/register it as false
  • register it as false before core does, add_action( 'wp_default_styles', function( &$styles ) { $styles->add( 'open-sans', false ); }, 9 ); should probably do it (untested)

If the register method is too hard, which realistically I don't think it is for altering a major part of the admin styling, then introducing an explicit filter seems like the better way (although you can already use the gettext filter, it's way too heavy-handed for this)

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


3 years ago

#18 @DrewAPicture
3 years ago

  • Keywords dev-feedback 2nd-opinion removed
  • Milestone 4.2 deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Per the consensus between @nacin and @dd32, the recommendation for now is to deregister the style and re-register with an empty/false value. See comment:10. Closing as wontfix.

#19 @norcross
3 years ago

I disagree with the logic and decision about this, but it isn't a hill worth dying on.

Note: See TracTickets for help on using tickets.