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

Fix maskable icon setting behaviour based on site icon #770

Merged
merged 5 commits into from
Apr 23, 2022
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 38 additions & 11 deletions tests/test-class-wp-web-app-manifest.php
Original file line number Diff line number Diff line change
@@ -245,6 +245,7 @@ public function test_get_manifest() {
$this->mock_site_icon();
$blogname = 'PWA & Test "First" and \'second\' and “third”';
update_option( 'blogname', $blogname );
update_option( 'site_icon_maskable', false );
$actual_manifest = $this->instance->get_manifest();

// Verify that there are now entities.
@@ -264,16 +265,41 @@ public function test_get_manifest() {
$this->assertEquals( $expected_manifest, $actual_manifest );

// Check that icon purpose is `any maskable` if site icon is maskable.
$actual_manifest = $this->instance->get_manifest();
$this->assertEquals( $expected_manifest, $actual_manifest );
$purposes = array();
foreach ( $actual_manifest['icons'] as $icon ) {
if ( ! isset( $purposes[ $icon['purpose'] ] ) ) {
$purposes[ $icon['purpose'] ] = 0;
} else {
$purposes[ $icon['purpose'] ]++;
}
}
$this->assertEquals(
array(
'any' => 1,
),
$purposes
);

// Make sure maskable is properly checked.
update_option( 'site_icon_maskable', true );
$actual_manifest = $this->instance->get_manifest();
$expected_manifest['icons'] = array_map(
function ( $icon ) {
$icon['purpose'] = 'any maskable';
return $icon;
},
$expected_manifest['icons']
$actual_manifest = $this->instance->get_manifest();
$purposes = array();
foreach ( $actual_manifest['icons'] as $icon ) {
if ( ! isset( $purposes[ $icon['purpose'] ] ) ) {
$purposes[ $icon['purpose'] ] = 0;
} else {
$purposes[ $icon['purpose'] ]++;
}
}
$this->assertEquals(
array(
'any' => 1,
'maskable' => 1,
),
$purposes
);
$this->assertEquals( $expected_manifest, $actual_manifest );

// Check that long names do not automatically copy to short name.
$blogname = str_repeat( 'x', 13 );
@@ -482,9 +508,10 @@ public function test_get_icons() {
$expected_icons = array();
foreach ( $this->instance->default_manifest_icon_sizes as $size ) {
$expected_icons[] = array(
'src' => $this->expected_site_icon_img_url,
'sizes' => sprintf( '%1$dx%1$d', $size ),
'type' => self::MIME_TYPE,
'src' => $this->expected_site_icon_img_url,
'sizes' => sprintf( '%1$dx%1$d', $size ),
'type' => self::MIME_TYPE,
'purpose' => 'any',
);
}
$this->assertEquals( $expected_icons, $this->instance->get_icons() );
7 changes: 7 additions & 0 deletions wp-admin/js/customize-controls-site-icon-pwa.js
Original file line number Diff line number Diff line change
@@ -31,6 +31,13 @@ wp.customize(
// Update active state whenever the site_icon setting changes.
siteIconSetting.bind(updateActive);

// Change the site_icon_maskable if site_icon is not set.
siteIconSetting.bind((newSiteIconValue) => {
if (!newSiteIconValue) {
siteIconMaskableSetting(false);
}
});

Comment on lines +34 to +40
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does capture @pooja-muchandikar's comment in #304 (comment):

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

However, I didn't think this was necessarily a bug. It's just whether one thinks of the maskable property being linked to the icon, or if maskable is something that you would persist across icon changes.

Copy link
Collaborator Author

@thelovekesh thelovekesh Apr 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, I didn't think this was necessarily a bug.

Agreed, rather it is just a check to ensure that by default we are assuming that an icon is maskable.

If we had to save maskable property in an icon meta then we will be able to guess it correctly, for an icon maskable property should be true or not, but saving maskable property in meta could be overkill IMO.

/**
* Validate site icons for its presence and size.
*/
11 changes: 7 additions & 4 deletions wp-includes/class-wp-customize-manager.php
Original file line number Diff line number Diff line change
@@ -35,10 +35,13 @@ function pwa_customize_register_site_icon_maskable( WP_Customize_Manager $wp_cus
$wp_customize->add_control(
'site_icon_maskable',
array(
'type' => 'checkbox',
'section' => 'title_tagline',
'label' => __( 'Maskable icon', 'pwa' ),
'priority' => $site_icon_control->priority + 1,
'type' => 'checkbox',
'section' => 'title_tagline',
'label' => __( 'Maskable icon', 'pwa' ),
'priority' => $site_icon_control->priority + 1,
'active_callback' => function() use ( $wp_customize ) {
return (bool) $wp_customize->get_setting( 'site_icon' )->value();
},
thelovekesh marked this conversation as resolved.
Show resolved Hide resolved
)
);
}
14 changes: 8 additions & 6 deletions wp-includes/class-wp-web-app-manifest.php
Original file line number Diff line number Diff line change
@@ -504,16 +504,18 @@ public function get_icons() {
$src = get_site_icon_url( $size );
if ( $src ) {
$icon = array(
'src' => $src,
'sizes' => sprintf( '%1$dx%1$d', $size ),
'type' => $mime_type,
'purpose' => 'any',
'src' => $src,
'sizes' => sprintf( '%1$dx%1$d', $size ),
'type' => $mime_type,
);

$icons[] = $icon;

if ( $maskable ) {
$icon['purpose'] = 'any maskable';
$icon['purpose'] = 'maskable';
$icons[] = $icon;
}

$icons[] = $icon;
}
}
return $icons;