Make WordPress Core

Opened 8 years ago

Closed 2 years ago

#35717 closed defect (bug) (fixed)

Audit the .icon16 selectors in admin-menu.css

Reported by: afercia's profile afercia Owned by: desrosj's profile desrosj
Milestone: 6.1 Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch
Focuses: ui, css Cc:

Description

Noticed while investigating the admin stylesheets for color contrast, looks like the .icon16 selectors in admin-menu.css are no more used. I'm not 100% sure and I'd really appreciate some feedback though.

As far as I see, the .icon16 selectors were used only for the Welcome Panel at some point, while the admin menu always used its own selectors. It seems to me they can be safely removed.

Some history:
Introduced in WordPress 3.3 to reuse the admin menu icons on the Welcome Screen, see [19163] and [19197].

The icons were then removed with the Welcome Panel redesign for WP 3.5, see [22018] and #21368.

The selectors were still present in the MP6 stylesheets, see [26072], but no more used in any screen? If so, it would be a nice opportunity to do some cleaning.

Attachments (3)

35717.0.patch (2.8 KB) - added by mmaumio 8 years ago.
Removed the .icon16 class from wp-admin/admin-menu.css
35717.diff (2.2 KB) - added by desrosj 2 years ago.
Screen Shot 2022-07-20 at 10.46.51.png (117.0 KB) - added by desrosj 2 years ago.

Download all attachments as: .zip

Change History (14)

@mmaumio
8 years ago

Removed the .icon16 class from wp-admin/admin-menu.css

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


3 years ago

#2 @isabel_brison
3 years ago

  • Focuses css added
  • Keywords dev-feedback removed

The icon16 selector is no longer used in Core, but a few plugins still use it: https://wpdirectory.net/search/01F0APW51X955Z64064W4SQXBZ

It would be great to work out a deprecation policy for CSS so that 3rd party code can gradually move off these unused selectors.

@mmaumio : regarding your patch, the aim is to remove the CSS rules targeting icon16 altogether, not just the selector. However, we can only do that once we have come up with a way to address the deprecation issue. All the same, thanks for your effort!

#3 @sabernhardt
3 years ago

Checking for spaces and/or quotes narrows down the plugin directory search a bit more. A few of those matches are still not classes, and the top two plugins contain the same link.

The directory search also found one theme, but that's quite outdated and the icon link is not necessary when the link URLs are visible anyway.

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

This ticket was mentioned in Slack in #core-css by sabernhardt. View the logs.


3 years ago

This ticket was mentioned in Slack in #core-css by sabernhardt. View the logs.


2 years ago

#6 @desrosj
2 years ago

  • Keywords close has-patch added; needs-patch removed
  • Milestone set to 6.1

I came across this one going through open tickets missing a milestone.

Normally, I advocate for leaving code whenever there's the potential for a site to break, but I think enough time has passed and this one is acceptable to remove.

Some notes from my investigation:

  • The one theme using the .icon16 class only has 50 installs. And when the relevant CSS is removed nothing actually "breaks". The icons just disappear.
  • This is admin only facing, should not affect regular site visitors.
  • A repeat search with updated results shows more matches, but fewer total active installs.
  • The results show a lot of false positives and instances where plugins are using that class for something unrelated.

Moving this to the 6.1 milestone and giving a bit more time for additional thoughts to be added.

Last edited 2 years ago by desrosj (previous) (diff)

@desrosj
2 years ago

#7 @audrasjb
2 years ago

  • Keywords commit added; close removed

Removing close keyword :)

The patch still applies cleanly against trunk.
Considering @desrosj’s research, +1 for removing these useless CSS declarations 👍

#8 @desrosj
2 years ago

  • Owner set to desrosj
  • Status changed from new to assigned

Thanks! Adding close was definitely an accident there.

#9 @desrosj
2 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 53731:

Administration: Remove unused CSS selectors related to old format menu icons.

This removes all CSS definitions related to the .icon16 class. Research shows that current ecosystem usage of this class is extremely minimal.

This selector was used for the Welcome Panel and Welcome Screen in WordPress 3.3 (see [19163] and [19197]. The icons were removed for WordPress 3.5 in [22018], but the related CSS remained, and were also included when the MP6 admin re-skinning was merged in [26072].

Props afercia, mmaumio, isabel_brison, sabernhardt, audrasjb, desrosj.
Fixes #35717.

#10 @desrosj
2 years ago

  • Keywords commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening because I noticed some weirdness in Trac and Making WordPress Core after this commit. Though it's not clear how this is related just yet, as the class does not appear to be used anywhere on .org.

Tickets page 2 days ago: https://web.archive.org/web/20220718083419/https://make.wordpress.org/core/reports/.

Tickets page today (see screenshot, as pulled into the Tickets overlay on Trac).

#11 @desrosj
2 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

Reclosing. The issue above was coincidental and unrelated.

Note: See TracTickets for help on using tickets.