WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 6 months ago

#46363 closed task (blessed) (fixed)

Images in about.php should be served locally, not from s.w.org

Reported by: pputzer Owned by: garrett-eclipse
Milestone: 5.6 Priority: normal
Severity: normal Version:
Component: Help/About Keywords: has-screenshots has-patch commit
Focuses: css, privacy Cc:

Description

As part of the ongoing efforts to reduce the external resources loaded by a vanilla WordPress install to zero, the images used in the about.php page should be included in the ZIP archive instead of being served from https.//s.w.org/images. Since these are SVG images, the download size should not increase significantly. Serving the files locally is beneficial both for reasons of privacy and speed.

Attachments (17)

freedoms-2x.png (461.1 KB) - added by garrett-eclipse 6 months ago.
2x Freedoms image used in CSS
freedoms.png (109.4 KB) - added by garrett-eclipse 6 months ago.
Freedoms Image (Not used in CSS)
46363.diff (884 bytes) - added by garrett-eclipse 6 months ago.
Initial Patch to introduce the freedoms image locally inside core.
46363.alt.diff (1.3 KB) - added by garrett-eclipse 6 months ago.
Alt patch to also include the freedoms.png main image even though it's not used in the css.
Screen Shot 2020-10-07 at 1.50.33 PM.png (772.6 KB) - added by garrett-eclipse 6 months ago.
Screen illustrating it's locally hosted.
Screen Shot 2020-10-16 at 6.20.47 PM.png (1.0 MB) - added by hellofromTonya 6 months ago.
Image served locally
46363.2.diff (1.6 KB) - added by garrett-eclipse 6 months ago.
Initial patch for reducing image file size by switching to 2 columns until 420px and then going to single column leaving us with a 308px image meaning 2x is 616px (or 600 if we round down)
freedoms.2.png (105.8 KB) - added by garrett-eclipse 6 months ago.
Original 1x image can now be used as the 2x image as we reduced the max image size by half. This is the original on s.w.org run through ImageOptim for a further improvement.
38f828cc13d3e54e30857f0c26814f13.gif (5.1 MB) - added by garrett-eclipse 6 months ago.
Showing the responsive approach dropping to two column before 1 column at a lower breakpoint means we can reduce the max size needed for the images.
Screen Shot 2020-10-20 at 9.48.35 PM.png (155.3 KB) - added by garrett-eclipse 6 months ago.
Inspect illustration showing new max image size when at single column 420px screen wide.
46363.3.diff (1.7 KB) - added by garrett-eclipse 6 months ago.
Refresh to introduce a .med-2-columns class to avoid any back-compat issues on changing the existing .has-4-columns class.
46363.3-firefox.gif (8.2 MB) - added by hellofromTonya 6 months ago.
Showing responsive styling in Firefox with patch 46363.3.diff applied
46363.3-safari.gif (9.4 MB) - added by hellofromTonya 6 months ago.
Showing responsive styling in Safari with patch 46363.3.diff applied
Screen Shot 2020-10-26 at 1.12.16 AM.png (724.9 KB) - added by hellofromTonya 6 months ago.
With patch 46363.3.diff, showing HTML with med-2-columns class attribute and image served locally.
46363.4.diff (1.6 KB) - added by garrett-eclipse 6 months ago.
Refresh of the second diff to move into the 480 breakpoint. And avoid the .mid-2-columns that the third diff had. This is just awaiting a resized image.
46363.5.diff (1.8 KB) - added by ryelle 6 months ago.
freedoms.3.png (349.3 KB) - added by ryelle 6 months ago.

Change History (38)

This ticket was mentioned in Slack in #core-privacy by pepe. View the logs.


2 years ago

#2 @johnbillion
2 years ago

  • Focuses performance removed
  • Milestone changed from Awaiting Review to Future Release
  • Type changed from defect (bug) to enhancement
  • Version 5.1 deleted

#3 @mukesh27
2 years ago

  • Component changed from Privacy to Help/About

#4 @pputzer
2 years ago

  • Focuses privacy added

This ticket was mentioned in Slack in #meta by garrett-eclipse. View the logs.


6 months ago

@garrett-eclipse
6 months ago

2x Freedoms image used in CSS

@garrett-eclipse
6 months ago

Freedoms Image (Not used in CSS)

@garrett-eclipse
6 months ago

Initial Patch to introduce the freedoms image locally inside core.

@garrett-eclipse
6 months ago

Alt patch to also include the freedoms.png main image even though it's not used in the css.

@garrett-eclipse
6 months ago

Screen illustrating it's locally hosted.

#6 @garrett-eclipse
6 months ago

  • Keywords has-patch needs-testing has-screenshots added
  • Milestone changed from Future Release to 5.6
  • Owner set to garrett-eclipse
  • Status changed from new to accepted

From a related discussion on meta Slack and #45395 let's try to get this in for 5.6.

I've uploaded the Freedoms image used in freedoms.php and updated the css to use it as found in 46363.diff. I also provided 46363.alt.diff in case we along with freedoms-2x.png want to include freedoms.png in core.

Currently with 5.5 the about.php only provides videos via CDN so we'll want to discuss them, they're probably too large to host with core. But maybe in 5.6 about page #51415 we can use gifs instead.

@hellofromTonya
6 months ago

Image served locally

#7 @hellofromTonya
6 months ago

  • Keywords commit added; needs-testing removed

Tested. Yes, it serves locally as expected.

Committer note: The 2 images with this patch will need to be included in the src/wp-admin/images/ folder.

#8 @SergeyBiryukov
6 months ago

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

In 49219:

Help/About: Serve the image on the Freedoms page locally.

Serving the files locally is beneficial for reasons of privacy and speed.

Props garrett-eclipse, pputzer, hellofromTonya.
Fixes #46363.

#9 in reply to: ↑ description @SergeyBiryukov
6 months ago

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

Replying to pputzer:

Since these are SVG images, the download size should not increase significantly.

Didn't want to block the commit, but the new freedoms-2x.png image is ~460 KB, which is more than all the other images in wp-admin/images together. Is there anything we can do to optimize it?

#10 @SergeyBiryukov
6 months ago

  • Type changed from enhancement to task (blessed)

#11 @garrett-eclipse
6 months ago

For sure, aside from just optimizing the image we can revisit the breakpoints so we go first to two columns and then to single column. This means we won't need as large an image as the most space it'll fill should be reduced.
We could also break it out into 4 images but not sure if that is a benefit as their total weight would be the same but now they'd need 4 requests.

I'll explore the 2 column approach once Beta has landed here.

@garrett-eclipse
6 months ago

Initial patch for reducing image file size by switching to 2 columns until 420px and then going to single column leaving us with a 308px image meaning 2x is 616px (or 600 if we round down)

@garrett-eclipse
6 months ago

Original 1x image can now be used as the 2x image as we reduced the max image size by half. This is the original on s.w.org run through ImageOptim for a further improvement.

@garrett-eclipse
6 months ago

Showing the responsive approach dropping to two column before 1 column at a lower breakpoint means we can reduce the max size needed for the images.

@garrett-eclipse
6 months ago

Inspect illustration showing new max image size when at single column 420px screen wide.

#12 @garrett-eclipse
6 months ago

  • Focuses css added
  • Keywords has-patch needs-testing dev-feedback added

I've uploaded an initial patch 46363.2.diff to illustrate the concept I had on reducing the image size.

In fact in the freedoms.2.png has cut that size into a quarter (~460kb down to now ~108kb) by reducing the max image width into half it's original.

A few notes as this is only an initial patch and could be improved;

  1. I tweaked the existing .has-4-columns class to achieve this. I fear this is or will be used for another implementation where they wanted the original responsive behaviour. To resolve this we could maybe introduce a new .has-2-mid-columns class to apply our styles specifically.
  2. I changed the name from freedoms-2x.png to just freedoms.png in the patch, but the image I uploaded since there was another prior was renamed to freedoms.2.png so when downloading please rename.
  3. I introduced a new breakpoint at 420px after testing and found that kept the images when 2 column still looking nice and achieved the exact 1/2 of our original size allowing me to just use the pre-existing 1x version of the freedoms image off the CDN. Note: I only found that coincidence after I'd found a breakpoint I was happy with sizing on.

CC @SergeyBiryukov

#13 @garrett-eclipse
6 months ago

  • Keywords needs-design-feedback needs-refresh added

Adding needs-design-feedback to confirm the direction of using 2 columns on medium sizes and double-check the breakpoints. I've added needs-refresh as mentioned in my last comment for back-compat I believe we'll want to introduce a new class instead of adding a breakpoint to the existing to avoid changing other sections.

This ticket was mentioned in Slack in #core-css by kirstyburgoine. View the logs.


6 months ago

@garrett-eclipse
6 months ago

Refresh to introduce a .med-2-columns class to avoid any back-compat issues on changing the existing .has-4-columns class.

#15 @garrett-eclipse
6 months ago

  • Keywords dev-feedback needs-design-feedback needs-refresh removed
  • Status changed from reopened to accepted

In 46363.3.diff I've refreshed the patch to ensure we don't introduce any back-compat issues by updating the .has-4-columns class.

Aside from testing this is looking pretty good.

@hellofromTonya
6 months ago

Showing responsive styling in Firefox with patch 46363.3.diff applied

@hellofromTonya
6 months ago

Showing responsive styling in Safari with patch 46363.3.diff applied

@hellofromTonya
6 months ago

With patch 46363.3.diff, showing HTML with med-2-columns class attribute and image served locally.

#16 @hellofromTonya
6 months ago

  • Keywords commit added; needs-testing removed

Tested on Firefox and Safari. Results:

  • .med-2-columns class attribute is applied
  • responsive styling behaves as expected
  • image is served locally

This ticket was mentioned in Slack in #core by garrett-eclipse. View the logs.


6 months ago

#18 @ryelle
6 months ago

I think it would be safe to drop has-4-columns to two columns at that middle size by default, rather than introducing a new class — I'm actually surprised it's not already doing that. I don't remember having any specific reason for dropping directly to a single column, and the 4-column design is usually only used for text (developer notes).

Version 0, edited 6 months ago by ryelle (next)

@garrett-eclipse
6 months ago

Refresh of the second diff to move into the 480 breakpoint. And avoid the .mid-2-columns that the third diff had. This is just awaiting a resized image.

@ryelle
6 months ago

@ryelle
6 months ago

#19 @ryelle
6 months ago

46363.5.diff iterates on the previous patch to fix the second row of items on IE, and uses the 480px breakpoint for switching to single column. I also got image resizing sorted out, so now this 800px wide freedoms.png image (350 KB) can replace the freedoms-2x.png (460 KB).

To summarize the patch itself, it introduces a breakpoint into the 4-column layout, so that we don't have a large jump from 4 columns down to a much wider single column at 782px. The 2 column layout exists from 782px to 480px, when it drops down to the single column. This means we can shrink the image used on the freedoms page, which saves about 110KB.

This ticket was mentioned in Slack in #core by helen. View the logs.


6 months ago

#21 @helen
6 months ago

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

In 49479:

About: Optimize freedoms sprite and add 2 column layout.

Props garrett-eclipse, ryelle.
Fixes #46363.

Note: See TracTickets for help on using tickets.