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

Linear filter not being applied to images on Stories #13013

Closed
adamdunnage opened this issue Feb 9, 2023 · 13 comments
Closed

Linear filter not being applied to images on Stories #13013

adamdunnage opened this issue Feb 9, 2023 · 13 comments
Labels
AMP Output Issues related to AMP output and validation Group: Style Panel PHP Pull requests that update PHP code Status: Needs More Info Follow-up required in order to be actionable. Type: Bug Something isn't working Type: Support Questions & Feedback from support escalation.
Milestone

Comments

@adamdunnage
Copy link

Bug Description

Stories that have had a linear filter added to an image do not display that filter when previewing from the editor or on the published Story. This has been introduced in the 1.29.0 release as this can't be replicated on previous versions. All other filters seem to work and this only effects the linear filter.

This was first reported by a user in the support forum.

Linear image in the editor:

linear-editor

Linear image in preview:

linear-preview

Expected Behaviour

Steps to Reproduce

  1. Create a new Story on version 1.29.0 of the plugin.
  2. Add an image.
  3. Apply the linear filter to the image.
  4. Compare the image in the editor to the preview and published Story.

Screenshots

See above.

Additional Context

  • Plugin Version: 1.29.0
@adamdunnage adamdunnage added Type: Bug Something isn't working Group: Style Panel Type: Support Questions & Feedback from support escalation. labels Feb 9, 2023
@swissspidy
Copy link
Collaborator

Hmm I am not able to reproduce this right now.

Left image has a filter, the right one doesn't. Looks correct on the frontend.

Also works fine for background images.

Screenshot 2023-02-09 at 17 36 48

Screenshot 2023-02-09 at 17 36 50

@swissspidy
Copy link
Collaborator

OK I can see the issue on the QA site but not locally 🤔

@swissspidy
Copy link
Collaborator

swissspidy commented Feb 9, 2023

The generated CSS should look like this:

background-image: linear-gradient(.5turn,rgba(0,0,0,0) 0%,rgba(0,0,0,.7) 100%

But it ends up being:

linear-gradient(.5 turn,rgba(0,0,0,0) 0%,rgba(0,0,0,.7) 100%
-linear-gradient(.5turn,rgba(0,0,0,0) 0%,rgba(0,0,0,.7) 100%
+linear-gradient(.5 turn,rgba(0,0,0,0) 0%,rgba(0,0,0,.7) 100%

Notice the space after .5

Don't know where that is coming from 🤔 Maybe the PHP-CSS-Parser or the AMP sanitization.

It's definitely saved correctly to the database, so definitely something that happens when serving.

@swissspidy swissspidy added PHP Pull requests that update PHP code AMP Output Issues related to AMP output and validation Status: Needs More Info Follow-up required in order to be actionable. labels Feb 10, 2023
@swissspidy
Copy link
Collaborator

So far unable to reproduce reliably unfortunately. It didn't work on our QA site yesterday, but today all seems fine. On the staging site it still doesn't seem to work though, but not sure why.

@swissspidy
Copy link
Collaborator

I've been able to track this down to AMP_Style_Sanitizer on our staging site, where this issue occurs. Does not happen with 'disable_style_processing' => true,

The issue does not happen for me locally though, makes debugging a bit cumbersome.

@westonruter Is this perhaps an issue you've seen in the past?

@westonruter
Copy link
Collaborator

westonruter commented Feb 10, 2023

Processing of the turn unit was broken in older versions of php-css-parser but it was fixed in MyIntervals/PHP-CSS-Parser#350. This is on master as of Dec 28, 2021 but is not in the most recent release which was 8.4.0 on Dec 21, 2021 (note: this is the latest tag, not the latest release). But you're using the version of php-css-parser in the most recent stable version of the AMP plugin, right?

@westonruter
Copy link
Collaborator

westonruter commented Feb 10, 2023

Is that CSS correct though? I tried looking at it on a non-AMP page and Chrome DevTools gave me a warning:

Warning in Chrome DevTools

And I just checked in the W3C CSS Validator with this:

div {
    background-image: linear-gradient(.5turn,rgba(0,0,0,0) 0%,rgba(0,0,0,.7) 100%
}

And I got:

Value Error : background-image Parse Error

@westonruter
Copy link
Collaborator

Is there a missing closing parenthesis for the linear-gradient() function? If I add that missing ) then Chrome DevTools no longer gives me a warning:

image

And I see the gradient show up on an AMP page (and non-AMP page) as expected.

@swissspidy
Copy link
Collaborator

I don't think it's an issue with the PHP-CSS-Parser version, because the exact same version of the plugin seems to run fine on one site but not the other. No AMP plugin installed.

Regarding the validity, I simply forgot to paste the closing ).

The problem is the .5 turn vs .5turn.

@swissspidy
Copy link
Collaborator

But you're using the version of php-css-parser in the most recent stable version of the AMP plugin, right?

Yes, we use whatever the AMP plugin uses. Looks like it contains that fix because of composer patches.

@westonruter
Copy link
Collaborator

Could it be that you have another plugin that also bundles php-css-parser, but an older version that lacks the fix?

@swissspidy swissspidy added this to the 1.30.0 milestone Feb 10, 2023
@swissspidy
Copy link
Collaborator

It happens with no other active plugins. Plus we use PHP-Scoper anyway, so other versions of that library shouldn't matter.

But now that you mentioned the specific PR and me mentioning the patches, I recall removing our own composer patches in #12904. Turns out that disabled the AMP plugin's patches too 🤦

For some reason my local install did still have the patches, which is why I didn't notice this at first.

I just reverted that change in 5b66321 and now it seems to work on our staging site as well.

Time for the weekend 🙃


@adamdunnage in short, this should be fixed in our next release

@swissspidy
Copy link
Collaborator

The patch is now definitely in our latest release, see https://plugins.svn.wordpress.org/web-stories/tags/1.30.0/third-party/vendor/sabberworm/php-css-parser/src/Value/Size.php

Yet we're getting reports that this issue is still happening, see https://marleysmenu.com/web-stories/fluffy-and-crispy-brown-sugar-pancakes/ for an example with broken turn unit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AMP Output Issues related to AMP output and validation Group: Style Panel PHP Pull requests that update PHP code Status: Needs More Info Follow-up required in order to be actionable. Type: Bug Something isn't working Type: Support Questions & Feedback from support escalation.
Projects
None yet
Development

No branches or pull requests

3 participants