Make WordPress Core

Opened 6 years ago

Closed 4 years ago

#46363 closed task (blessed) (fixed)

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

Reported by: pputzer's profile pputzer Owned by: garrett-eclipse's profile 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 4 years ago.
2x Freedoms image used in CSS
freedoms.png (109.4 KB) - added by garrett-eclipse 4 years ago.
Freedoms Image (Not used in CSS)
46363.diff (884 bytes) - added by garrett-eclipse 4 years ago.
Initial Patch to introduce the freedoms image locally inside core.
46363.alt.diff (1.3 KB) - added by garrett-eclipse 4 years 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 4 years ago.
Screen illustrating it's locally hosted.
Screen Shot 2020-10-16 at 6.20.47 PM.png (1.0 MB) - added by hellofromTonya 4 years ago.
Image served locally
46363.2.diff (1.6 KB) - added by garrett-eclipse 4 years 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 4 years 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 4 years 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 4 years 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 4 years 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 4 years ago.
Showing responsive styling in Firefox with patch 46363.3.diff applied
46363.3-safari.gif (9.4 MB) - added by hellofromTonya 4 years 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 4 years 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 4 years 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 4 years ago.
freedoms.3.png (349.3 KB) - added by ryelle 4 years ago.

Change History (38)

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


6 years ago

#2 @johnbillion
6 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
6 years ago

  • Component changed from Privacy to Help/About

#4 @pputzer
6 years ago

  • Focuses privacy added

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


4 years ago

@garrett-eclipse
4 years ago

2x Freedoms image used in CSS

@garrett-eclipse
4 years ago

Freedoms Image (Not used in CSS)

@garrett-eclipse
4 years ago

Initial Patch to introduce the freedoms image locally inside core.

@garrett-eclipse
4 years ago

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

@garrett-eclipse
4 years ago

Screen illustrating it's locally hosted.

#6 @garrett-eclipse
4 years 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
4 years ago

Image served locally

#7 @hellofromTonya
4 years 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
4 years 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
4 years 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
4 years ago

  • Type changed from enhancement to task (blessed)

#11 @garrett-eclipse
4 years 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
4 years 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
4 years 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
4 years 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
4 years ago

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

#12 @garrett-eclipse
4 years 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
4 years 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.


4 years ago

@garrett-eclipse
4 years 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
4 years 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
4 years ago

Showing responsive styling in Firefox with patch 46363.3.diff applied

@hellofromTonya
4 years ago

Showing responsive styling in Safari with patch 46363.3.diff applied

@hellofromTonya
4 years ago

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

#16 @hellofromTonya
4 years 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.


4 years ago

#18 @ryelle
4 years 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). (well, and here :) )

Last edited 4 years ago by ryelle (previous) (diff)

@garrett-eclipse
4 years 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
4 years ago

@ryelle
4 years ago

#19 @ryelle
4 years 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.


4 years ago

#21 @helen
4 years 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.