WordPress.org

Make WordPress Core

Opened 2 months ago

Closed 2 months ago

#47183 closed defect (bug) (fixed)

Console warning with Dashicons font and input type="radio"

Reported by: aduth Owned by: desrosj
Milestone: 5.2.1 Priority: normal
Severity: normal Version: 5.2
Component: Administration Keywords: has-patch fixed-major
Focuses: ui Cc:

Description

Previous Slack conversations:

Possibly related to (introduced by?): r44940 (#41074)

As of WordPress 5.2, viewing any admin page where a checked radio button is present will display a warning in the developer tools console:

Failed to decode downloaded font: http://editor.test/wp-includes/fonts/dashicons.eot?50db0456fde2a241f005968eede3f987
OTS parsing error: invalid version tag

Steps to reproduce:

  1. Open your browser's developer tools console
  2. Navigate to /wp-admin/export.php
  3. Observe warning in console

Additional debugging information from @talldanwp:

I’ve narrowed it down to being triggered by radio inputs. They use a pseudo-element to display the filled circle when checked:

https://github.com/WordPress/wordpress-develop/blob/master/src/wp-admin/css/forms.css#L141
My current thinking is that the warning is being shown because of content: "\2022";. The pseudo-element is inheriting dashicons as the font family, but ‘\2022’ isn’t a valid dashicons code point. This is reproducible in wp-admin and within Gutenberg (post visibility in Gutenberg, screen options on the posts screen in wp-admin).

It doesn’t actually seem to cause any problems in the UI. I’m not entirely sure why this is all of a sudden an issue, either. The CSS has been unchanged for 4-5 years.

Attachments (1)

47183.patch (564 bytes) - added by aduth 2 months ago.

Download all attachments as: .zip

Change History (17)

#1 @aduth
2 months ago

As noted by @afercia in Slack, the warning occurs only in Google Chrome (in my case, v74.0.3729.131).

#2 @afercia
2 months ago

I see two possible reasons for this (Chrome-only) warning:

Granted, when a browser doesn't find a font-family, it should fallback to the one used on the ancestors and ultimately to the OS/browser default, so the query param might play a role anyways.

@aduth
2 months ago

#3 @aduth
2 months ago

Related Dashicons pull request which appears to be a remedy for this warning:

https://github.com/WordPress/dashicons/pull/360

47183.patch can serve for testing. There may be additional changes pending a response to my comment about unexpected file changes.

#4 @aduth
2 months ago

  • Keywords has-patch added

#5 @SergeyBiryukov
2 months ago

  • Milestone changed from Awaiting Review to 5.2.1

#6 @superpoincare
2 months ago

I have seen some tickets which remove stylesheets for IE8 and below since it's no longer supported.

eot files are for ie8 and below. So we could remove any reference to it altogether.

The patch still has a reference to the eot file.

#7 @timph
2 months ago

I ran into this on the frontend of a site when logged in as admin. I didn't check other roles, but probably still the same result if the wpadminbar is displayed. I don't have any radio buttons as far as I can tell anywhere. Maybe this is just because of the search icon on the input field though? Not sure.

I tried a fresh install using twenty-nineteen, and https://github.com/WPTRT/theme-unit-test imported:
https://i.imgur.com/kO1sktn.png

As a note, it's pretty wacky that it that this gets triggered on resize too, which is seen above in ss with the 100 warnings in console.

Browser: Chrome Version 70.0.3538.77 (Official Build) (64-bit)
OS: Ubuntu 18.04.2 LTS

Just deleting the line locally in sources tab fixed it, and I applied 47183.patch to the test install and no longer am receiving the warnings.

I agree with @superpoincare though that we should not be enablers for old IE versions and remove the .eot references and file for dashicons completely. Many themes also enqueue dashicons on the frontend for icons used inside the theme, so I'd assume the same would apply for non-logged in users in those situations. It reduces bundle size, and prevents this situation from happening again, so seems like a win/win kind of scenario.

#8 @desrosj
2 months ago

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

#9 @desrosj
2 months ago

  • Keywords commit added

47183.patch looks good to me. This was merged upstream in the PR @aduth mentioned.

I worked with @ianbelanger on some IE testing. EOT format is not just for IE 8 and below. It is actually supported through IE 11 (which Core still supports). In our testing, we also found that IE 11 defaults to using the EOT format.

If we were to remove EOT from Dashicons, this would need to be discussed and happen upstream in the GitHub repository. Even then, the EOT file would need to persist in Core for some time for backwards compatibility.

#10 @desrosj
2 months ago

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

In 45310:

Administration: Remove duplicate font-face declaration in Dashicons CSS.

This was causing console warnings in some browsers.

Props aduth, joen, afercia, timph, ianbelanger.
Fixes #47183.

#11 @desrosj
2 months ago

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

Reopening for backport consideration.

#12 @desrosj
2 months ago

  • Keywords fixed-major added

#13 @superpoincare
2 months ago

@desrosj

IE 11 supports woff but it chooses eot because of the syntax. Ideally the syntax should be written so that IE 11 chooses woff over eot as woff is considered an upgrade to eot.

Zach Leat here says the syntax should have woff2 and woff only:

https://calendar.perfplanet.com/2016/no-font-face-bulletproof-syntax/

He also argues that data-uri is anti-pattern and hence we should use woff2 and woff files, not base64 encodes in css:

https://www.zachleat.com/web/web-font-data-uris/

Last edited 2 months ago by superpoincare (previous) (diff)

#14 @desrosj
2 months ago

@superpoincare I am not against the idea. Sorry if my last comment came across that way. However, that change is outside the scope of this ticket. The goal here is to fix the console warning caused by the duplicate font-face.

Also, as I mentioned, the Dashicons font is maintained in GitHub. That is the correct place to propose a change like this as the build process and files in that repository will need to be updated with any changes before they can be introduced to Core.

Additionally, this is a change that should probably be included in a major release and accompanied by a note in the release documentation so that developers managing sites that require support for older browsers are able to adjust accordingly.

I recommend opening an issue on the Dashicons repository detailing your proposal to drop EOT with some research and reasoning so it can be properly evaluated.

#15 @superpoincare
2 months ago

@desrosj

That makes perfect sense. True, the present goal is to get rid of the error message. Will open an issue at Github.

#16 @desrosj
2 months ago

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

In 45312:

Administration: Remove duplicate font-face declaration in Dashicons CSS.

This was causing console warnings in some browsers.

Merges [45310] to the 5.2 branch.

Props aduth, joen, afercia, timph, ianbelanger.
Fixes #47183.

Note: See TracTickets for help on using tickets.