Automattic / wp-calypso

The JavaScript and API powered WordPress.com
https://developer.wordpress.com
GNU General Public License v2.0
12.41k stars 1.98k forks source link

Images with descriptions output HTML as well as image #44897

Closed jordesign closed 4 years ago

jordesign commented 4 years ago

Steps to reproduce

  1. Create a new blog post
  2. Add a featured image - making sure the image has a 'Description' in the metadata
  3. Publish post.
  4. Create a new page and insert a "Latest Post' block that includes the post you just created and shows featured images
  5. Set an alignment on the 'Latest Posts' block (left/right/center/wide width/full width).
  6. Publish the page

What I expected

I expect the most recent post to show the feature image and then post title

What happened instead

Underneath the feature image some HTML code of the image src is output This appears to not occur on self-hosted sites but on both SImple and AT sites on WPCOM

Browser / OS version

Mac OSX/Chrome

Screenshot / Video

Screen Shot 2020-08-13 at 1 24 52 pm Screen Shot 2020-08-13 at 1 24 59 pm

Context / Source

This was uncovered originally in #3220085-zen and then further tested by @JoshuaGoode and myself.

lsl commented 4 years ago

Somewhat debugged in D48055-code looks like it might be a wider issue, you can likely replicate this in more contexts using html e.g. <strong>Test</strong> in the image description.

jordesign commented 4 years ago

Have tested this in a .org/JurassicNinja site and could not recreate - even with HTML in the image description.

lsl commented 4 years ago

It seems limited to the wpcom context, I was not able to repro the issue on .org with a default jetpack setup activated. (I was potentially missing an activating a JP module or something?)

I suspect this is related to the issues seen in jetpack here:

lsl commented 4 years ago

@jeherve would it make sense to move this issue into the Jetpack repo or is it ok to track this here too?

jeherve commented 4 years ago

I was not able to repro the issue on .org with a default jetpack setup activated. (I was potentially missing an activating a JP module or something?)

If you still have your Jetpack site setup, could you try to activate the Carousel module under Jetpack > Settings?

image

If the issue appears with the module on, I think it's safe to close this issue in favor of https://github.com/Automattic/jetpack/issues/13428

jordesign commented 4 years ago

If you still have your Jetpack site setup, could you try to activate the Carousel module under Jetpack > Settings?

I've just tried this on my .org test - and can't reproduce even with the Carousel module active

jeherve commented 4 years ago

Hm, that's an odd one. On WordPress.com Simple and Atomic sites we also run the Gutenberg plugin; maybe the Latest Posts Block was recently modified in Gutenberg and the bug was introduced then? What happens when you activate the Gutenberg plugin on your Jetpack site?

jordesign commented 4 years ago

I've been able to recreate slightly different symptoms on https://specific-guanaco.jurassic.ninja/testing-latest-posts-block/

This is with Jetpack Modules enabled and Gutenberg plugin activated. I've confirmed this for not take place when Gutenberg plugin is disabled - so I guess it is something there?

Screen Shot 2020-08-17 at 9 50 06 am

No code output - but they still seem broken - no links on titles when image has a description.

Screen Shot 2020-08-17 at 9 52 03 am
jordesign commented 4 years ago

Further testing - the issue is not seen when Gutenberg plugin is active and Carousel mode is turned off. So a conflict between the two?

jeherve commented 4 years ago

Yes, it would seem like it, and that's most likely something that would be fixed by https://github.com/Automattic/jetpack/issues/13428

lsl commented 4 years ago

So turns out that note about requiring an alignment (even if its a center alignment) is quite important.

:wave: @ockham, @jeherve

The jetpack carousel module augments the latest-posts block featured images with some extra attributes. I'm assuming this is to apply some jetpack styling/features.

This augmentation includes adding an image description which will by way of wpautop always include some html.

The html is being escaped here and injected here

When an alignment is added to this latest posts block, the alignment block supports code in Gutenberg is triggered leading to an mb_convert_encoding to make use of DomDocument.

This mb_convert_encoding call and subsequent reversal seems to be what causes the issue we see here.

The issue seems to be that the fix for WordPress/gutenberg/issues/24445 is decoding html entities it probably shouldn't be in its attempt to rollback the encoding it had to do to use DomDocument cleanly.

I can't really think of a good fix other than doing a dodgy temp replacement of anything that's already encoded but I'm not really sure how safe that is. Then again, it does work so there's that. https://github.com/lsl/gutenberg/commit/126b1a34c6fcc524bfaaa5926da086c1111484ef

Should I move this to a Wordpress/Gutenberg issue (if one doesn't exist?) - Or do you think we should fix this on ourside? (remove image descriptions from Jetpack's carousel data attributes?)

jeherve commented 4 years ago

The jetpack carousel module augments the latest-posts block featured images with some extra attributes. I'm assuming this is to apply some jetpack styling/features.

Ideally, the Carousel module should not add extra attributes here, I think. This may be the best approach here.

ockham commented 4 years ago

Flagging for @sirreal @fullofcaffeine @WunderBart since this is related to our Gutenberg encoding fix :confused:

ockham commented 4 years ago

Thanks for flagging @lsl!

I can't really think of a good fix other than doing a dodgy temp replacement of anything that's already encoded but I'm not really sure how safe that is. Then again, it does work so there's that. lsl/gutenberg@126b1a3

Dodgy doesn't sound great TBH :grimacing: As a rule of thumb, the further upstream we go, the cleaner our fixes should be. So: Gutenberg should handle things in a clean fashion; okay to have a workaround in Jetpack, if need be.

On a more practical level -- did you check whether Gutenberg unit tests still pass with your fix applied?

Should I move this to a Wordpress/Gutenberg issue (if one doesn't exist?) - Or do you think we should fix this on ourside? (remove image descriptions from Jetpack's carousel data attributes?)

Leaning towards Jetpack.

There's a bit of a chance that https://github.com/WordPress/gutenberg/pull/24447 also affects other downstream projects that inject attributes into the rendered HTML, but I'm not sure we can construct a char replacement function that still fixes https://github.com/WordPress/gutenberg/pull/24445 but doesn't break attributes injected by 3rd parties (such as Jetpack). If we can, all the better, but my intuition says no, so this might just be a pill that downstream projects have to swallow.

lsl commented 4 years ago

On a more practical level -- did you check whether Gutenberg unit tests still pass with your fix applied?

The expected tests do, a separate css test was failing (so yes, it was a dodgy fix after all :grimacing:!)

Leaning towards Jetpack.

I found a better way to fix this (based on), all tests pass and I've confirmed the desired behavior for WordPress/gutenberg#24445 is maintained.

I would appreciate any ideas around what to cover when writing a couple of tests for this new fix.

Also is dropping LIBXML_HTML_NOIMPLIED a problem? I think we can keep this in if its required.

ockham commented 4 years ago

@lsl Great, thanks! I left a comment on the commit. Let's discuss there (or better yet, on a PR in the Gutenberg repo :slightly_smiling_face: )

lsl commented 4 years ago

This will be fixed in the next Gutenberg rollout of v8.8

sirreal commented 4 years ago

I was expecting to fix this for Simple and Atomic sites running 8.7.1, but I've been unable to reproduce the issue.

Is this issue still present on Simple and Atomic (not running 8.8)?

lsl commented 4 years ago

Seems to be https://testing705633960.wordpress.com/home/ 8.7.1 simple site business plan.