Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#54208 closed defect (bug) (fixed)

Twenty Twenty-One: Social icons have duplicate height and width values

Reported by: max-dayala's profile Max dAyala Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.9 Priority: normal
Severity: normal Version: 5.8.1
Component: Bundled Theme Keywords: good-first-bug has-screenshots has-patch commit
Focuses: Cc:

Description

This applies to the twenty twenty-one bundled theme.

Refer to the class:

Twenty_Twenty_One_SVG_Icons

in the file:

class-twenty-twenty-one-icons.php

The header of the file says that the svg icons should not have a height or width specified. This is because the height and width are added later by the get_svg() function.

The social icons, however, have the height and width specified in the svg definitions. This means that the height and width attributes end up appearing twice in the final string representing the svg element. This is obviously invalid XML, although browsers will still parse it.

Here is example output obtained from viewing the source of a web page in the browser:

<svg class="svg-icon" width="24" height="24" aria-hidden="true" role="img" focusable="false" width="24" height="24" viewBox="0 0 24 24" version="1.1" xmlns="http://www.w3.org/2000/svg">

Note that the user interface icons in the same source file do not have the height and width specified. These appear to be correct.

Hence, unless there is a good reason not to, the height and width should be removed from the social icon svg definitions to avoid confusion and potential problems when people create child or custom themes based on these svg definitions.

Attachments (4)

social_icons.png (31.5 KB) - added by Laxman Prajapati 3 years ago.
secondary-navigation-svg.png (38.6 KB) - added by sabernhardt 3 years ago.
SVG social icons in secondary navigation
54208.patch (90.9 KB) - added by Laxman Prajapati 3 years ago.
Static height and width removed from svg icons.
54208-1.png (160.8 KB) - added by Laxman Prajapati 3 years ago.
Screenshot for the same.

Download all attachments as: .zip

Change History (14)

#1 @Presskopp
3 years ago

  • Summary changed from Social icons have duplicate height and width values to Twenty Twenty-One: Social icons have duplicate height and width values

#2 @sabernhardt
3 years ago

  • Keywords needs-patch good-first-bug added
  • Milestone changed from Awaiting Review to 5.9

#3 @Laxman Prajapati
3 years ago

Hi @max-dayala

Thanks for the ticket and welcome to WordPress Trac!

1) Could you please share a screenshot of view-source of the browser OR "Inspect Elements"?
2) Please confirm the file name with the directory path of it?

We can't fine this file in "twentytwentyone" theme on WordPress 5.8.1 version.

We have added the social icons from wordpress admin on one page and it's working fine from my view. Please find attached screenshot with the single time value of the height and width in the svg icon.

Thanks!

#4 @Laxman Prajapati
3 years ago

  • Keywords has-screenshots added

@sabernhardt
3 years ago

SVG social icons in secondary navigation

#5 @Laxman Prajapati
3 years ago

What are the step you follow to add svg icons in secondary navigation menu?

Please describe me?

#6 @sabernhardt
3 years ago

To reproduce the error:

  1. Go to Appearance > Menus and create a new menu.
  2. Assign it to the Secondary menu location.
  3. Add custom links to your menu, including some with various social URLs. For example, you could use WordPress.com and/or WordPress.org.
  4. Save menu.
  5. Visit the site and view source for the footer markup.
  6. Observe that the width and height attributes are included twice for each SVG in the footer menu links. (In Firefox, the duplicate attributes are highlighted in red.)

The Secondary menu's $social_icons array needs editing in classes/class-twenty-twenty-one-svg-icons.php.

The primary navigation's open and close buttons do not have the duplicate attributes because the $icons array does not include the width and height for those graphics.

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

@Laxman Prajapati
3 years ago

Static height and width removed from svg icons.

#7 @Laxman Prajapati
3 years ago

Hi @sabernhardt

Thanks for details and clarification for the same.

We have removed the static height and width from the svg icons and after that we have checked with the social svg icons with header,content and footer, It's working fine from my side.

Thanks!

#8 @Laxman Prajapati
3 years ago

  • Keywords has-patch added; needs-patch removed

@Laxman Prajapati
3 years ago

Screenshot for the same.

#9 @mukesh27
3 years ago

  • Keywords commit added

Thanks for the patch.

54208.patch working fine for me.

Marking this ticket for commit.

#10 @SergeyBiryukov
3 years ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 51893:

Twenty Twenty-One: Remove duplicate width and height values from social icons.

These values are added dynamically by the Twenty_Twenty_One_SVG_Icons::get_svg() method and are not needed in the source array.

Follow-up to [49216].

Props max-dayala, laxman-prajapati, sabernhardt, Presskopp, mukesh27.
Fixes #54208.

Note: See TracTickets for help on using tickets.