Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Facilitate indicating the site icon as being maskable #304

Closed
westonruter opened this issue Jul 5, 2020 · 18 comments · Fixed by #698 or #770
Closed

Facilitate indicating the site icon as being maskable #304

westonruter opened this issue Jul 5, 2020 · 18 comments · Fixed by #698 or #770
Milestone

Comments

@westonruter
Copy link
Collaborator

As reported on the support forum, when doing a Lighthouse PWA audit, the web app manifest generated by the plugin currently gets a warning:

Manifest doesn't have a maskable icon

Read more about this on web.dev: Manifest doesn't have a maskable icon.

A site owner can fix this problem by just adding the purpose property to all of the icons:

add_filter( 'web_app_manifest', function( $manifest ) {
	$manifest['icons'] = array_map(
		function ( $icon ) {
			if ( ! isset( $icon['purpose'] ) ) {
				$icon['purpose'] = 'any maskable';
			}
			return $icon;
		},
		$manifest['icons']
	);
	return $manifest;
} );

The plugin could also automatically add this maskable property. But these both assume that the icon is actually maskable. Is this good to assume? What if we added a new checkbox after where the Site Icon is supplied in the Customizer to let the user indicate it is maskable:

image

Related: #246

@westonruter westonruter added the web-app-manifest Web App Manifests label Jul 5, 2020
@fellyph
Copy link

fellyph commented Jul 6, 2020

I saw this same issue this weekend using chrome canary, if there is no customisation on the panel should it include it maskable by default?

@westonruter
Copy link
Collaborator Author

I don't think it should be maskable by default because then it could crop the icon undesirably.

@fellyph
Copy link

fellyph commented Jul 6, 2020

You are right, the checkbox is the best option.

@valendesigns
Copy link

valendesigns commented Jul 6, 2020 via email

@crm911
Copy link

crm911 commented Aug 7, 2020

//A site owner can fix this problem by just adding the purpose property to all of the icons://

Sorry, I am not a developer. Where exactly does that go? Somewhere inside pwa_manifest.json?

@westonruter
Copy link
Collaborator Author

It can go in your theme's functions.php file.

@crm911
Copy link

crm911 commented Aug 7, 2020

Thank you. This did it for me.

A tip for others - always flush the cache. :)

@westonruter
Copy link
Collaborator Author

westonruter commented Feb 4, 2022

This is a bit more complicated now with Full Site Editing because the Site Icon is now managed in the Block Editor, not the Customizer.

Update: Per below in #304 (comment) I think we should skip any FSE-specific integration for now given the Customizer is still linked to for setting the Site Icon.

@westonruter westonruter added this to the 0.7 milestone Feb 8, 2022
@westonruter
Copy link
Collaborator Author

This is something else that comes up over and over again, so we need a UI to indicate the icon is maskable. If not using a block-based theme, the Site Icon control in the Customizer is where the UI should live per my screenshot above. Otherwise, if using a block-based theme when the Customizer is not available, I'm not entirely clear where the UI should be located. I suppose it would be an extension to the Site Logo block?

@thelovekesh
Copy link
Collaborator

thelovekesh commented Feb 9, 2022

@westonruter I have taken this up. Related to the scenario of issue I and @ankitrox discussed the possibilities to make this fully compatible in FSE. Here are some key points from the discussion:

Customizer

FSE

  • Target block will be the Site Logo block.
  • Check if the user has enabled Use as site icon toggle.
  • If yes, then show the toggle for the maskable icon.
  • While saving the block set we will also save it for customizer as this icon will also be saved for customizer setting.

@westonruter
Copy link
Collaborator Author

When the checkbox for maskable icon

It would be a nice touch if clicking this checkbox would apply a mask to the icon so the user can see what effect it has. This would in part limit the need for having a link to explain what maskable icons are.

  • While saving the block set we will also save it for customizer as this icon will also be saved for customizer setting.

I'm not too familiar with how FSE site icon is managed. The non-FSE Custom Logo is stored as a theme_mod and the Customizer saves it as such. In contrast, the Site Icon is stored as an option. I recall that in FSE the Custom Logo is now stored as an option instead. It's not clear to me how the Custom Logo, however, maps to the Site Icon in FSE. In particular, the logo may be completely inappropriate for use as an icon. The logo may be rectangular and be visually rich, whereas a site icon needs to be square and able to be legible at very small sizes. It also needs to be a PNG, as you noted.

In looking further, I saw the WP Tavern article about this and:

If they are different images or if the user does not use a logo, the only built-in way to change the site icon directly is through the customize.php URL trick mentioned above. The Site Logo block also adds a link to the customizer option.

image

So the ability to set the maskable flag would still be available for FSE themes, if they click that “Site Icon settings” link. To me it seems like there should be a non-Customizer way to set the Site Icon when using FSE.

All this to say: I think it's probably good to just skip the FSE integration for now, given that the user can (and probably should) use the Customizer for this. Presumably the UI for managing the Site Icon will evolve in the FSE Site Editor, so probably best to not spend too much time on it right now.

@ankitrox
Copy link
Collaborator

It would be a nice touch if clicking this checkbox would apply a mask to the icon so the user can see what effect it has. This would in part limit the need for having a link to explain what maskable icons are.

Currently when we upload icon, it is already rounded corner giving the impression that it is masked. If we apply above-mentioned strategy, we may need to make the icon square shaped by default. When checkbox for maskable icon is checked, we can add border-radius to icon.

Does this sound fine @westonruter ?

@westonruter
Copy link
Collaborator Author

The rounded corners are good, but they're not round enough. It should instead apply the “minimum safe zone” as described at https://web.dev/maskable-icon/#are-my-current-icons-ready

@thelovekesh
Copy link
Collaborator

@westonruter Showing icon in minimum safe zone when checking Maskable icon will work and the user can have an Idea when to use this setting. Check the below demos and let me know if it works.

non-maskable-icon.mp4
maskable-icon.mp4

@westonruter
Copy link
Collaborator Author

Yes, that looks good!

@pooja-muchandikar
Copy link

pooja-muchandikar commented Apr 19, 2022

@thelovekesh

Following are some observations related to Site Icon being Maskable.

  • Maskable checkbox is in place but when the checkbox is checked from customizer, an error is shown on manifest, ideally it should show any error. Also the same error is displayed twice any reason?
    image

image


  • If site icon is not set, Maskable Icon setting is visible, it shouldn't be visible. It should be visible only if site icon is set
    image

  • If Site icon was set and set as maskable and then removed the site icon, Maskable Icon setting remains checked.
Customize_.Quality.Assurance.PWA.Site.Just.another.WordPress.site.mp4

Test Environment
WordPress v5.9.3
PWA v0.7.0-alpha-52d5096-20220419T054300Z
Theme 2021

cc: @westonruter

@westonruter
Copy link
Collaborator Author

  • Maskable checkbox is in place but when the checkbox is checked from customizer, an error is shown on manifest, ideally it should show any error.

Very interesting. I hadn't seen this warning before. I see it also reported here:

In the latter case, the fix was just to duplicate the icon and give one maskable and the other any: SuperPWA/super-progressive-web-apps@ca128da

But this presumes that the chosen icon actually is suitable for masking and non-masking. If we want to be extra sure for the user, we could duplicate the preview image to appear alongside the checkbox and only show it and give it a masking when the checkbox is selected, leaving the original preview as-is without any masking. But that may be excessive. In general I'd think that if an icon is maskable then it should be presumed to work without masking as well.

  • If Site icon was set and set as maskable and then removed the site icon, Maskable Icon setting remains checked.

Yes, it makes sense that the field should be hidden when no icon is selected. Good catch.

@pooja-muchandikar
Copy link

QA Passed ✅

Reference: #770 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants