Make WordPress Core

Opened 17 years ago

Closed 17 years ago

Last modified 35 hours ago

#12010 closed enhancement (invalid)

Insert Audio with FILE URL does not work correctly

Reported by: wpmuguru Owned by:
Priority: normal Milestone:
Component: Multisite Version: 2.8.4
Severity: normal Keywords: reporter-feedback has-patch has-unit-tests
Cc: Focuses:

Description

MU Trac Ticket: http://trac.mu.wordpress.org/ticket/958

When inserting audio into a page with the File Url option the result is: <a href=" http://jmcginnis.bandzilla.net/files/2009/04/03-plateau.mp3" rel='attachment wp-att-20'>Plateau</a>

After a save the result is: <a href=' http://jmcginnis.bandzilla.net/home/03-plateau/' rel='attachment wp-att-20'>Plateau</a>

As you can see it turns into a attachment link.

Change History (52)

#1 @wpmuguru
17 years ago

Can someone confirm whether this is still a problem?

#2 @wpmuguru
17 years ago

  • Keywords needs-feedback added

#3 @freddyware
17 years ago

  • Type defect (bug)enhancement

I am pretty sure this is the intended behaviour. What is WordPress supposed to do with audio files, embed them?

#4 @wpmuguru
17 years ago

  • Resolutionfixed
  • Status newclosed

Works ok in trunk. Closing pending someone having an issue.

#5 @nacin
17 years ago

  • Keywords reporter-feedback added; needs-feedback removed
  • Milestone Unassigned

#6 @nacin
17 years ago

  • Resolution fixed
  • Status closedreopened

#7 @nacin
17 years ago

  • Resolutioninvalid
  • Status reopenedclosed

Re-open with steps to reproduce any unintended behavior.

#8 @jkershner
16 years ago

Resolved in 2.9.2

This ticket was mentioned in PR #12270 on WordPress/wordpress-develop by @fushar.


3 months ago
#9

  • Keywords has-patch has-unit-tests added

Trac ticket:

[!WARNING]
This PR is stacked on top of #12010. Please only review the new commit(s).

## Use of AI Tools

@fushar commented on PR #12270:


3 months ago
#10

cc-ing (maybe) interested parties :) @t-hamano @jasmussen @tyxla @scruffian @lucasmendes-design

@JeffPaul commented on PR #12270:


3 months ago
#11

These all appear to be solid like-for-like replacements, though I feel least supportive of the Sites icon replacement on multisite. Perhaps a new icon could/should be generated there or review other icons for something besides a 2x2 table sort of icon? Of the existing icons, the one that feels like what I'd recommend otherwise is one that looks like a "two child node branched to the right" (_sorry GitHub is giving me problems trying to upload a screenshot from my cell connection_).

@fushar commented on PR #12270:


3 months ago
#12

@jeffpaul Thanks for checking!

I feel least supportive of the Sites icon replacement on multisite.

Yep, as noted in the PR description, I'm not sure about this as well.

Of the existing icons, the one that feels like what I'd recommend otherwise is one that looks like a "two child node branched to the right"

You mean core/blockMeta? Indeed this looks more fitting. 😄

https://github.com/user-attachments/assets/e6c65b06-d961-4f6a-bd8c-e2f55a17a9b4

But my worry is that the icon slug is not related to "site" at all. I would expect there is something like core/sites icon. Perhaps it's better if we create a new such icon. cc: @jasmussen, @lucasmendes-design (I know you're all on vacation, feel free to respond when you're back.)

Another idea is to use this icon, to represent a "site switcher":

https://github.com/user-attachments/assets/57dc6934-dfbd-4e92-a988-7be0c6bc36e7

@tyxla commented on PR #12270:


3 months ago
#13

A few things from my end as I'm looking at this work:

  • This is exciting and a much-awaited overhaul of icons, love it!
  • It has the potential to have an impact on many plugins, plugin infrastructures and products that rely on the current icon set - something to be mindful about
  • I don't see enough feedback from @WordPress/gutenberg-design on this, and I believe this is a hard requirement
  • I believe it's a bit late in the 7.1 cycle for this to move forward, but it makes sense to get it to the finish line on time for landing early in the 7.2 cycle. It's paramount to give plugins and extenders enough time for testing and adopting the necessary changes.
  • An angle that doesn't seem explored is trying this out as a Gutenberg experiment - not only to allow for safe iteration, but also as yet another way to get useful feedback.

@JeffPaul commented on PR #12270:


3 months ago
#14

@fushar yes the blockMeta was the icon I was trying to reference, though I'm not quite as concerned with the slug for it (not certain that the slug is referenced or rendered anywhere in the UI and if not then there's no end user confusion that could happen). I'm not much in favor of the chevronUpDown option for Sites, in that case would just stick with the 2x2 table-looking icon I suppose but in any case would like a better one for that icon if possible (and if as @tyxla notes this is too late for 7.1 then we've got some time to iterate on the Sites icon).

@fushar commented on PR #12270:


3 months ago
#15

An angle that doesn't seem explored is trying this out as a Gutenberg experiment

Thanks for the suggestion. I spent a good chunk of my day today trying to make this as a Gutenberg experiment.

With the help of Claude, I (we) came up with this PR: https://github.com/WordPress/gutenberg/pull/79588. Now, I understand that the changes are complicated, but I can't simplify it any further. Reasons:

  • I need to render the icons that are not yet public. wp_get_icon() won't allow me to render such icons, so I need to replicate the implementation for now.
  • There's no way to hook into admin menu icon. Basically I really need this new logic in Core, which can't be extended via hooks. Hence the experiment needs to hijack the output of the admin menu HTML and replace dashicons with SVG before it gets rendered.
  • The CSS needs to duplicate the admin color scheme to achieve hover effect in the admin bar. In this Core change, I can just update the _admin.scss directly, which can't be replicated in experiments.

Am I approaching this correctly? Or do you have better idea for a cleaner approach? Thanks!

@fcoveram commented on PR #12270:


3 months ago
#16

It looks great ✨ I noticed a few things that I'm not fully sure belong to the PR scope.

### Icon size

In most cases, the icon size is 22px, but the W symbol is 24px.

22px 24px
https://github.com/user-attachments/assets/2cea23a8-e9f4-4c32-a62f-e8820657331a https://github.com/user-attachments/assets/3919ffc6-a3e1-459e-a5ce-860d47095c92

Not sure if this is intentional or not.

### Color on the frontend

On the frontend, the color at 0.6 looks slightly lighter. I know it is consistent, but at 0.85 looks visually more similar to the current. This is not a blocker but sharing it for thoughts.

### "Sites" icon

Regarding the icon for Sites, I agree that blockMeta works well until having a dedicated icon.

---

I've been testing it on Arc, Chrome, and Safari and on multiple viewports. And they all look good ⭐

@tyxla commented on PR #12270:


3 months ago
#17

Am I approaching this correctly? Or do you have better idea for a cleaner approach?

Hmm, this seems correct to me. There are side ways for achieving some of the things you mentioned, but not for others, unless we want to be super hacky. A bummer that this part of the menu is not as extensible as usual. Thank you for trying, though.

Another way to get more attention on this would be to post a call for testing on the https://make.wordpress.org/core blog.

@mcsf commented on PR #12270:


3 months ago
#18

Am I approaching this correctly? Or do you have better idea for a cleaner approach? Thanks!

I'd say so, at least for the purposes of a Gutenberg experiment. I left a comment or two in that PR.

@fushar commented on PR #12270:


3 months ago
#19

yes the blockMeta was the icon I was trying to reference

@jeffpaul: I agree that it looks better than a 2x2 grid. For the purpose of demonstration, I updated this PR to use that for the time being:

https://github.com/user-attachments/assets/a1c24c8d-ab14-4aca-8fae-d0e48e1f58ed

@fushar commented on PR #12270:


3 months ago
#20

@fcoveram: Thanks for the thorough testing!

In most cases, the icon size is 22px, but the W symbol is 24px.
Not sure if this is intentional or not.

Yeah, it is intentional. I'm not sure if it's the best way though. It's to achieve the same look and feel with the existing W dashicon. See the above image comparison, pasted here again:

https://github.com/user-attachments/assets/65924104-8433-44c3-9fce-7aa8a48a9972

Right now, if we use 22px, then the W logo is too small, because there's outer gap in the SVG. Maybe we need to fill the entire SVG box without any outer gap.

On the frontend, the color at 0.6 looks slightly lighter. I know it is consistent, but at 0.85 looks visually more similar to the current.

Yeah agree that 0.85 looks better. But I'm not sure if we can just update it only for the SVG icon. It's part of the Fresh color scheme; maybe we should do a separate PR to update the scheme 😄

@fushar commented on PR #12270:


3 months ago
#21

All right everyone, thanks for taking a look. I want to summarize where we're at with this PR:

  • To use currently non-public icons, we need the public: false flag as discussed in https://github.com/WordPress/gutenberg/pull/79451#issuecomment-4797981729.
    • @t-hamano, is this something that you're currently pursuing? How confident are we with the solution?
  • This PR adds support to use registered icons in left sidebar menu items. See this diff. To move forward, I need a review on this as it's modifying a public hook (admin_menu).
    • Also, maybe we should at least ship this part first in 7.1 so that plugins can start using it?
    • If we do that, it will be beneficial to ship the CSS changes for rending SVG icons in admin bar / menu.
  • We need to iterate on My Sites icon. I can ask for help for this. @jasmussen / @lucasmendes-design should be interested.
  • As suggested, we should publish a public Make post as a heads-up to the community and plugin developers. In the post, we should suggest the default (core) icon set for the admin, and how plugins can use SVG icons in the admin menu (the public API change I mentioned above).

It feels like there's interest to push at least some parts of it in 7.1, and we're not that sure if we can achieve everything by 7.1. @t-hamano, what's your opinion on this? It seems you have the most context for all this icon API discussion 😄

cc: @tyxla, @mcsf, @scruffian.

@wildworks commented on PR #12270:


3 months ago
#22

It feels like there's interest to push at least some parts of it in 7.1, and we're not that sure if we can achieve everything by 7.1. @t-hamano, what's your opinion on this? It seems you have the most context for all this icon API discussion 😄

Leveraging the icon API for the dashboard icons is a great idea, and introducing a public:false flag shouldn't be difficult. However, I noticed that core icons might be unregistered by consumers.

add_action( 'init', function () {
    wp_unregister_icon_collection( 'core' );
}, 20 );
add_action( 'init', function () {
    wp_unregister_icon( 'core/info' );
}, 20 );

In this case, what happens to the core menu icons registered by the icon API? This is my biggest concern. Here are the ideas I can come up with at the moment:

  • Allow file paths for menu_icon. This ensures that icons are rendered even if they are unregistered from the icon registry, as the file itself will still exist within the core.
  • Allow SVG strings for menu_icon. This might be a larger undertaking, requiring a proper SVG processing pipeline.
  • Introduce a "built-in" collection to house icons used in dashboards and protect the unregisteredion of this collection or its icons would enhance robustness.

Do you have any other ideas?

@fushar commented on PR #12270:


3 months ago
#23

Thanks for taking a look.

I noticed that core icons might be unregistered by consumers.

Hmm interesting question. I would say, the last option (consider core/* as built-in icon set which can't be unregistered) looks the most sensible to me. It's like having a fixed set of dashicons-* slugs that consumers can always use currently.

Otherwise, we will also need another way to get the SVG string for the admin bar icons as well, because in this PR I also use wp_get_icon() to prepend the icon to the title 🤔

@mcsf commented on PR #12270:


3 months ago
#24

However, I noticed that core icons might be unregistered by consumers.

This is a good point.

  • Allow file paths for menu_icon. This ensures that icons are rendered even if they are unregistered from the icon registry, as the file itself will still exist within the core.

One of the problems here is that it makes it more difficult to iterate on icon designs in Gutenberg, since those icons will no longer override those in wp-includes.

It also may or may not add some work for certain WP installations that rely on CDNs for core assets...? This is unfamiliar territory for me.

  • Allow SVG strings for menu_icon. This might be a larger undertaking, requiring a proper SVG processing pipeline.

Sounds risky and heavy-handed, as you point out yourself.

  • Introduce a "built-in" collection to house icons used in dashboards and protect the unregisteredion of this collection or its icons would enhance robustness.

Maybe, but it could make things frustrating for certain admins. For example, if — say, for editorial purposes — they'd like to keep a very narrow set of available icons for the Icon block. I don't think it's unheard of to prevent deregistering a core provision, but we almost always allow it (e.g. wp_deregister_script, unregister_block_type...). (Side note: we can always explore a separate mechanism for the Icon block so that third parties can filter its list of available icons.)

There's a fourth option, loosely in response to the one above:

Allowing core icons (and core collection) to be deregistered, so that they will not show up in listings, but embedding a fallback somewhere (WP_Icons_Registry::get_registered_icon? wp_get_icon?) so that certain core icons will still render.
I don't exactly like the idea, but I thought I'd share it anyway.

Another option is to honour admins' decision to deregister things, and have a fallback "no-icon" icon. In other words, make admins and extenders responsible for the questionable decision to deregister core icons: they can do it, but the admin will look weird with placeholder icons.

@wildworks commented on PR #12270:


3 months ago
#25

Another option is to honour admins' decision to deregister things, and have a fallback "no-icon" icon. In other words, make admins and extenders responsible for the questionable decision to deregister core icons: they can do it, but the admin will look weird with placeholder icons.

Is it possible to extend this idea further and display the relevant dashicon as a fallback if the SVG icon is not registered?

@keoshi commented on PR #12270:


3 months ago
#26

One point to add to @fcoveram comment here:

In most cases, the icon size is 22px, but the W symbol is 24px.

All @wordpress/icons icons were designed with a 24×24px area in mind and all/most are optimized to be visualized at that size. Certainly in non-retina displays. Not sure if a deal breaker or not, so pinging @jasmussen to get your thoughts.

@tyxla commented on PR #12270:


3 months ago
#27

Hmm interesting question. I would say, the last option (consider core/* as built-in icon set which can't be unregistered) looks the most sensible to me. It's like having a fixed set of dashicons-* slugs that consumers can always use currently.

This might make sense since this is how post types work - you can't unregister_post_type() a built-in post type. But for that to work, post types also maintain a _builtin flag.

@tyxla commented on PR #12270:


3 months ago
#28

A high-level flag from me: the fact that we are still asking and answering fundamental API/design questions (registration/unregistration/compatibility) less than a couple of weeks before beta is a clear sign that a feature might need more time to get feature-complete, thoroughly thought out, and polished enough for a WordPress release. For some of that work, we might want to get broader feedback through a Make post, too, and that, on its own, will require some time to get meaningful feedback.

@Joen commented on PR #12270:


2 months ago
#29

Noting that I designed a "Sites" icon here: https://github.com/WordPress/gutenberg/pull/80094

@mcsf commented on PR #12270:


2 months ago
#30

Is it possible to extend this idea further and display the relevant dashicon as a fallback if the SVG icon is not registered?

I think this would defeat the purpose of introducing SVG icons to finally move away from Dashicons! Then we'd have to support both systems forever.

I like Marin's argument below. Currently it seems like the wisest choice to me:

This might make sense since this is how post types work - you can't unregister_post_type() a built-in post type. But for that to work, post types also maintain a _builtin flag. Seems like this is yet another "flag" to consider for icons.

@Joen commented on PR #12270:


13 days ago
#31

Coming back to this one, the Verse icon is growing on me for posts. I also tried the cog for settings, the single image for media, the single page for pages, and in https://github.com/WordPress/gutenberg/issues/81274 I tried a new "tool" icon that could eventually benefit this effort.

https://github.com/user-attachments/assets/90a76fe4-e974-4da6-b837-7e5ff0958e57

@fushar commented on PR #12270:


12 days ago
#32

All right, after the dust settled for 7.1, I rebased updated the PR as follows:

  • Use default 24px size for the icons
  • Use the new core/sites for My Sites.
  • Use core/verse for Posts.
  • Use core/cog for Settings.

I updated the screenshots in this PR to reflect the latest.

We still need to decide how to use the currently-private icons; it's still unresolved discussion.

@fushar commented on PR #12270:


12 days ago
#33

Also, if we want to make this a Gutenberg experiment, we need to at least ship this diff in Core, to allow using core icons in the sidebar. Curious to hear what others think.

https://github.com/user-attachments/assets/cece69fe-2ba3-450c-978e-e84d5579d4d2

@Joen commented on PR #12270:


12 days ago
#34

Not a strong opinion, but I don't think this should be a Gutenberg experiment. It seems worth merging early in the cycle, and then evaluating feedback from that.

@afercia commented on PR #12270:


12 days ago
#35

When running npm install on this branch, it hangs for minutes for me with this message. Is it just me?

npm warn deprecated svgo@1.3.2: This SVGO version is no longer supported. Upgrade to v2.x.x.
npm notice This endpoint is being retired. Use the bulk advisory endpoint instead. See the following docs for more info: https://api-docs.npmjs.com/#tag/Audit

@ugyensupport commented on PR #12270:


12 days ago
#36

Tested locally against a WP 7.1 (trunk-level) install, both commits applied.

The PR's code works correctly against core alone — verified via wp eval that wp_get_icon('core/tool') and the other 11 new icons all return proper SVG markup, and confirmed visually (WP logo, Media, Pages, Appearance, Plugins, Tools, Settings all render the new inline SVGs correctly in the admin bar and sidebar, desktop and mobile breakpoints). The core/people no-avatar fallback and the mirrored search icon also behave as described in the PR.

Heads-up for anyone else testing locally with the Gutenberg plugin active, though: Gutenberg ships its own WP_Icons_Registry_Gutenberg shim (lib/class-wp-icons-registry-gutenberg.php) that replaces core's WP_Icons_Registry singleton with a registry pre-loaded from Gutenberg's own (currently stale) icon set. Core's _wp_register_default_icons() still runs, but registration of this PR's 12 new icons silently no-ops via _doing_it_wrong( 'WP_Icons_Registry_Gutenberg::register', 'Icon is already registered.' ) wherever the name collides with something Gutenberg already claimed under core/*. Net effect with Gutenberg active: wp_get_icon() returns '' for all 12 new icons, so the WP logo/Media/Pages/Appearance/Settings icons all render blank, and the sidebar's <img> fallback path (wp-admin/menu-header.php) additionally throws a console error per item (net::ERR_NAME_NOT_RESOLVED @ http://core/tool, etc., since esc_url('core/tool') resolves as a protocol-relative host).

This isn't a bug in the PR — deactivating Gutenberg makes every icon render correctly immediately — but since most contributors run Gutenberg locally, it's easy to mistake for one. Worth a note somewhere (or something Gutenberg's own compat shim will need to catch up on before/when this lands).

Screenshots (before / after / after-with-Gutenberg-active-showing-the-blank-icons) and full write-up: available on request, not attached here to keep this comment focused.

🤖 Generated with Claude Code

@afercia commented on PR #12270:


12 days ago
#37

A note on the wp_get_icon() function: the handling of the focusable="false" can be entirely removed. That attribute was originally introduced as a workaround for an Internet Explorer bug. IE made any SVG icon within a link focusable so there were two tab stops when tabbing through linked buttons that contained an icon.
Given IE is no longer supported, as far as I can tell the focusable="false" attrirbute is no longer necessary.
Cc @joedolson

@ugyensupport commented on PR #12270:


12 days ago
#38

Before/after screenshots from the local testing above (Gutenberg deactivated, so the PR's icons render as intended):

Before (Dashicons)
https://raw.githubusercontent.com/dugyen/pr12270-icon-test-screenshots/1b98c039e5eeb8e1a4b992cd6c16025adf10b978/pr12270-before-desktop.png

After (@wordpress/icons SVGs)
https://raw.githubusercontent.com/dugyen/pr12270-icon-test-screenshots/1b98c039e5eeb8e1a4b992cd6c16025adf10b978/pr12270-after-desktop.png

🤖 Generated with Claude Code

@Joen commented on PR #12270:


9 days ago
#39

Noting that I'm responding here to some feedback on the core post, just to consolidate it all in one place. I will follow up and link to this comment from the comments.

The feedback fell in two categories:

  1. which icon to best use for their dashicon counterparts
  2. optical balance: some being bigger, some being smaller.

For this PR I think we should update the icons to what agreement is found as far as which icons to use, pin vs. verse, etc. As for the actual optical balance changes, upon agreement I will make a PR to the componentry source and we can then follow up shortly after with a new PR here. It's important to know the context and the followup, even if we do it in two phases.

I tried to address as much as possible in the mockup below:

Set Mockup

| https://github.com/user-attachments/assets/d3b919da-63bd-4996-bd29-2cae874be5ad | https://github.com/user-attachments/assets/eed65a96-1ca1-4ab2-a996-0a934367afae |

  • Testing a new larger "brush" icon, closer to its original sibling counterpart.
  • Gallery icon instead of "image" for Media.
  • New optically balanced Pin icon for Posts
  • New optically balanced comment icon for comments.
  • Visually updated Tools icon, stroke based
  • Updated Settings icon

I personally think this one gets us closer as far as balance, though @mrwweb makes a good point that is notably visible in the bottom group of icons:

The icons themselves feel like what they are—an assemblage of icons designed for different purposes through the WP UI

There's an aspect of the icons having organically built up separately, as also tracked here. As part of the effort to convert to stroke-based, however, small improvements have been made to a number of icons specifically to address the points raised there, and extending that set for this collection of main admin icons feels valid. For me what stands out is that there are many diagonal icons in the utility section that causes a bit of a mix. Here's an attempt at addressing this head on:

Set Mockup

| https://github.com/user-attachments/assets/11724501-2f83-48ec-a298-ab4137e08f1d | https://github.com/user-attachments/assets/571d1074-f3f4-4942-b338-101859eb2209 |

  • The Plugins icon is now adjusted, polished, and tipped 45 degrees.
  • There's a new "Media" icon, that's closer to the source material.

The rework of the plugins icon makes it immediately feel like it belongs next to tools and appearance.

I'm not too sure about the new Media icon: I honestly prefer using something simpler like just a single "image" icon. But there is some value in recognition that's worth acknowledging.

---

In defence of the "Verse": the following I want to acknowledge as good feedback:

re: the posts icon – I think the iconography is too similar to the updated appearance icon. And while the Verse block certainly isn’t an everyday block for most sites, it feels odd to reuse it for the original post type. I didn’t do a full audit, but I wonder about other icon purposes overlapping that might create confusion, even if infrequently.

Perhaps with a larger brush, the Verse icon can be reintroduced as it no longer looks similar. Nevertheless, the post type point is worth noting. I think of this as the quill pen icon, that happens to be used for the Verse block, and thus the connection between block and type didn't occur to me.

WordPress may be the last bastion of the open web. Its users write, without arbitrary limitation from closed platforms, never locked in. There's some beauty to that which feels like poetry. So for me using the quill pen icon is more a matter of bringing that character into WordPress—code is poetry, after all.

In the process of redrawing icons to be stroke-based, I also tried some stress tests to validate the point of stroke-based, namely that you can contextually set the weight:

https://github.com/user-attachments/assets/e1b2a3e0-b226-472f-aa1e-c02f74da54a6

It's likely not a UI we'll use anywhere—or could it become part of a mobile effort? In any case, next to "Page", the quill pen just looks beautiful to me.

In any case, I'll be delighted for WordPress to get fresh energy on the icon side. For now, the main action item on these mockups is to agree on: which icons should we use? If we find agreement on those, I will follow up on the icon design changes based on that agreement and we can make a new PR. Is that fair?

---

To note, a suggestion was to use the "styles" icon for the Apperance section. I tried that below, and I think it could work if also replacing some of the other utility icons.

<details><summary>View</summary>

https://github.com/user-attachments/assets/7bc10dcb-422b-4986-83e0-8a769088238c

</details>

@mrwweb commented on PR #12270:


8 days ago
#40

@jasmussen These changes all feel both very responsive to the overall feedback and like really meaningful improvements!

The plugins icon feels especially better cleaned up and tilted 45 degrees! I like the direction of the new Media Library icon, though there's something about the music notes that is maybe slightly too heavy. (That's a super nit pick, and it feels much more cohesive than the old one). I also like the settings icon change that was suggested. Really nice work!

I like your points in defense of the quill icon, writing, and the open web. I think there's something there. The new distinctive paintbrush icon solves what felt like the bigger of the two issues with the quill not being distinctive enough. Personally, I've never loved the pin icon either, so I like your idea of a change for Posts.

I haven't thought about this deeply, but I wonder if a fountain pen icon could potentially have the same meaning without any risk of ambiguity? (Pencil is presumably still reserved for editing.) Here's a noun project icon purely as a directional composition idea that could use a 45 degree angle.

@JeffPaul commented on PR #12270:


8 days ago
#41

In any case, next to "Page", the quill pen just looks beautiful to me.

In any case, I'll be delighted for WordPress to get fresh energy on the icon side.

Totally agree, beautiful!

@fushar How does the before/after look for an RTL site?

@Joen commented on PR #12270:


8 days ago
#42

Thank you all for the feedback. Noting that for now I've updated the "after" image (including a tiny slipstreamed tweak to make the musical note in "media" ever so slightly smaller) on the general make/core post.

In general I pledge to follow up on any details that emerge from conversation in followups—the fountain pen idea is a good one—though perhaps before I go much deeper it's better to give additional feedback a chance to land, and for Ashar to update this PR with the latest choices. I will then separately start to prep the new icons for inclusion in the library. Sound good?

@fushar commented on PR #12270:


8 days ago
#43

@fushar How does the before/after look for an RTL site?

@jeffpaul It looks like this 🙂

BeforeAfter
https://github.com/user-attachments/assets/b38542a9-5e99-4bbe-ace5-54444498dab2 https://github.com/user-attachments/assets/e6d02d91-66c5-49a6-a58e-a9d410f06543

@fushar commented on PR #12270:


8 days ago
#44

and for Ashar to update this PR with the latest choices.

@jasmussen I updated this PR (and the screenshots) with core/pin for Posts, and core/settings for Settings!

I'll try to revive the Gutenberg PRs which are necessary to get the non-public icons in for this PR.

@Joen commented on PR #12270:


8 days ago
#45

Thanks. I will also follow up ASAP on the new and adjusted icons shown in this thread.

@fushar commented on PR #12270:


6 days ago
#46

For anyone following along, the latest state of the PR can be tested using this special Playground link: https://playground.wordpress.net/?core-pr=12270&gutenberg-pr=79451 🙂

@ugyensupport commented on PR #12270:


6 days ago
#47

Re-tested this via the Playground link in the description (core PR + Gutenberg PR 79451), since core alone can't render this without the icon backport.

Verified working:

  • Sidebar icons (Dashboard, Posts, Media, Pages, Comments, Appearance, Plugins, Users, Tools, Settings, Collapse) render as real inline <svg>, not broken/missing glyphs — confirmed via DOM inspection, not just visually.
  • Admin bar icons (WP logo, Site Name/home, Menu toggle, Search, +New, Comments) render with the correct icon per item.
  • Hover/current-item highlighting still recolors the SVG (fill: currentColor + hover rules).
  • Folded/collapsed sidebar mode renders correctly, icon-only.
  • No console errors or 404s tied to icons; dashicons.css still loads fine as a residual dependency for the parts not yet converted.

One thing I suspected from reading the diff but disproved by testing: the CSS deletes several #wpadminbar #wp-admin-bar-<item> > .ab-item::before { width: 52px; height: 46px; ... } mobile (≤782px) rules with no obvious SVG replacement, which looked like a mobile-icon-sizing regression on first read. Checking computed styles at a 375px viewport showed it's fine — a pre-existing generic rule (#wpadminbar .ab-icon, #wpadminbar .ab-icon.svg-icon { width: 52px; height: 46px; ... }) already covers the new .ab-icon elements, so the deleted per-ID rules were dead code once the icon moved from ::before onto a real element. No action needed.

Suggestions (non-blocking):

  1. Dead CSS in the mobile media query: admin-bar.css still applies font: 40px/1 dashicons !important; to the new .ab-icon.svg-icon selector even though SVG sizing is fully controlled by the separate svg { width/height } rule added right after it. Harmless, but worth trimming for clarity.
  2. Redundant aria-hidden: _wp_admin_bar_icon(), the collapse-button span, and the .wp-menu-image wrapper in menu-header.php all set aria-hidden="true" on the wrapper *and* wp_get_icon() already sets it on the inner <svg> by default. Not wrong, just duplicate.
  3. Not exercised in this pass: RTL layout, multisite/network-admin sidebar, and the "has site icon" branch of the admin bar (my test site had no site icon and wasn't multisite). Worth a manual pass on those before merge.

Traced the icon-selection logic (menu.php's $builtin/WP_Icons_Registry::is_registered() branch, wp_admin_bar_site_menu()'s home-vs-dashboard choice, the dashicons-/data:image/svg+xml/registered-icon fallback chain in _wp_menu_output()) against the PR's before/after table and it's consistent — no logic bugs found. PHPUnit/QUnit fixture updates correctly match the new icon slugs.

Nice work overall — only minor cleanup opportunities, nothing blocking.

@fushar commented on PR #12270:


6 days ago
#48

Something is not right, it seems the above Playground link is not serving the latest state of this PR (the Posts sidebar is showing blank instead of pin). Let me think...

@wildworks commented on PR #12270:


41 hours ago
#49

@fushar, thank you for working on this, and I apologize for the delayed reply.

The biggest concern with moving this PR forward is that the core icon collection could be unregistered by consumers. If a consumer were to use code like the following—for example, to hide core icons in the Icon block—all the dashboard menu icons would disappear. I don't think the dashboard icon should disappear under any circumstances.

add_action( 'init', function () {
        wp_unregister_icon_collection( 'core' );
}, 20 );

https://github.com/user-attachments/assets/a1ab23b0-9f32-422a-9b85-949df5b846c9

Perhaps we should add a non-deletable _builtin icon collection, and any icons related to the dashboard should be included in that collection. This idea has already been proposed in https://github.com/WordPress/wordpress-develop/pull/12270#issuecomment-4864984947. This approach is similar to how post types and post taxonomies are handled.

@ugyensupport commented on PR #12270:


39 hours ago
#50

@t-hamano I re-tested this against the Playground link in the PR description (core PR #12270 + gutenberg#79451) and can confirm the concern — it's a clean, one-line reproduction:

add_action( 'init', function () {
        wp_unregister_icon_collection( 'core' );
}, 20 );

Before: wp_get_icon( 'core/pin' ) returns valid SVG, 99 icons registered under the core collection.
After the snippet runs: wp_get_icon( 'core/pin' ) returns '', 0 collections and 0 icons remain registered. Every admin-bar and sidebar icon disappears on /wp-admin/ — WP logo, +New, Comments, avatar, Dashboard, Posts, Media, Pages, Comments, Appearance, Plugins, Users, Tools, Settings, all gone. Removing the mu-plugin restores them immediately, confirming it's the unregister call.

Root cause is in the already-merged registry classes, not in this diff: WP_Icon_Collections_Registry::unregister() (class-wp-icon-collections-registry.php) and WP_Icons_Registry::unregister() (class-wp-icons-registry.php) have no protection for the core collection/icons, and this PR is what makes core chrome newly depend on that same collection.

+1 on the _builtin approach from #12270 (comment), mirrored on `unregister_post_type()`. Concretely, in icons.php/class-wp-icon-collections-registry.php/class-wp-icons-registry.php:

  1. Mark the core collection _builtin => true when _wp_register_default_icon_collections() registers it — but keep that flag internal (not part of the public label/description $allowed_keys in WP_Icon_Collections_Registry::register()), so a plugin can't declare its *own* collection built-in and make it un-removable too.
  2. Guard both unregister() methods:
    // WP_Icon_Collections_Registry::unregister()
       if ( ! empty( $this->registered_collections[ $collection_slug ]['_builtin'] ) ) {
           _doing_it_wrong( __METHOD__, __( 'Unregistering a built-in icon collection is not allowed.' ), '7.1.0' );
           return false;
       }
    
// WP_Icons_Registry::unregister()
   $collection = WP_Icon_Collections_Registry::get_instance()->get_registered( $this->registered_icons[ $icon_name ]['collection'] );
   if ( ! empty( $collection['_builtin'] ) ) {
       _doing_it_wrong( __METHOD__, __( 'Unregistering a built-in icon is not allowed.' ), '7.1.0' );
       return false;
   }

The second guard matters on its own — without it a consumer can still pick off individual core/* icons one at a time via wp_unregister_icon() even with the collection protected.

  1. Add coverage in tests/phpunit/tests/icons/wpIconCollectionsRegistry.php and wpIconsRegistry.php asserting both unregister calls return false and leave the registry untouched for core, plus a case that a plugin can't smuggle _builtin => true onto its own collection.

Since the registry classes are already in trunk, this would need its own small companion PR/ticket rather than living in this diff — happy to help put that together if useful. Screenshots of the before/after repro available if helpful.

@ugyensupport commented on PR #12270:


39 hours ago
#51

Screenshots from a repro of the above, on the Playground link in the PR description (core PR #12270 + gutenberg#79451):

Before — normal dashboard, all icons render:
https://github.com/user-attachments/assets/bf0cb176-b6f8-4127-992e-0db10f21057c

After — same site, after an mu-plugin runs the exact snippet quoted above (wp_unregister_icon_collection('core') on init, priority 20). Every admin-bar and sidebar icon disappears — WP logo, +New, Comments, avatar, and the entire left menu:
https://github.com/user-attachments/assets/0bca9f1e-2775-4726-8487-3add48026872

Removing the mu-plugin restores them immediately, confirming it's the unregister call.

@mrwweb commented on PR #12270:


35 hours ago
#52

Does preventing unregistering the icon in the admin also prevent unregistering the Core icon set in the Icon Block? If so, I think that would be a problem for a lot of customized sites.

Glancing at the snippets above would seem to suggest it applies to all contexts. Sorry that I don't have time to test this right now, but I just want to make sure. It'll still be important that developers can provide and limit icons in the Icon block to a single custom set that does not include the Core set.

Note: See TracTickets for help on using tickets.