Strip HTML from gallery image alt text #26574
Conversation
WunderBart
added
[Status] In Progress
|
Size Change: +37 B (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
WunderBart
force-pushed
the
fix/img-caption-html
branch
from
37d0b33 to
898b5f8
Compare
|
The current error seems to indicate that a block migration is run, even though it shouldn't be run 🤔 BTW how did you create the fixtures? https://github.com/WordPress/gutenberg/blob/710373b254fbcd15d524afdeb31da0d93c4defd4/packages/e2e-tests/fixtures/blocks/README.md#creating-fixtures says you only need to manually create I had a look at |
Yeah, setting those (and regenerating the other fixtures) seems to do the trick 👍 |
I did use the |
WunderBart
added
[Type] Bug
WunderBart
requested review from
ajitbohra,
mkevins,
nerrad,
ntwb and
talldan
as code owners
|
Writing a deprecation here is currently blocked by the fact that apparently, 'implicit' attributes (i.e. the ones that aren't stored as part of the block comment, but inferred through a queue) -- in this case: the I think that's a feature that should be added to deprecations, although it might be a bit tricky to get right, since serializing the updated attribute back to HTML would involve calling the right cc/ @youknowriad @mcsf @gziolo |
How did you arrive to this conclusion. That's not true I think or unexpected. |
| anchor: true, | ||
| align: true, | ||
| }, | ||
| isEligible( { images } ) { |
There was a problem hiding this comment.
Why do you need isElligible here? I believe this particular deprecation doesn't really need it?
There was a problem hiding this comment.
Because of what you said in your other comment? I guess if we don't need a migrate, we won't need an isEligible. But if we need a migrate, we'd need a way to know that the alt includes HTML, no?
| }; | ||
| } ), | ||
| }; | ||
| }, |
There was a problem hiding this comment.
Also, I'm not sure the migrate function is needed here because if I understand the PR properly, the attribute is not really rewritten, the same attribute will produce another output, that's it.
There was a problem hiding this comment.
Well the alt attribute -- which is implicit -- is being changed; so yeah, that attribute is part of the output (and not an explicit attribute in the block comment). Are you saying that a change like that doesn't need a migration? 🤔 If we don't add one, it means the block will load with HTML in the alt attribute -- it will only be removed when saving it again, correct?
There was a problem hiding this comment.
yes, correct. but now that I think about it, there's something weird in the implementation of the gallery save function regardless.
If the image doesn't have any "alt", it will get assigned an "alt" text after saving because it will be parsed from the fallback. I wonder if we should remove the fallback personally and instead apply the fallback in edit function instead.
There was a problem hiding this comment.
yes, correct. but now that I think about it, there's something weird in the implementation of the gallery save function regardless.
If the image doesn't have any "alt", it will get assigned an "alt" text after saving because it will be parsed from the fallback. I wonder if we should remove the fallback personally and instead apply the fallback in
editfunction instead.
You mean weird in terms of UX? So right now, after saving and reloading, the alt, if otherwise empty, would be prefilled with the caption. Moving to edit would essentially do the same though, no?
Whatever we do, let's please make sure to strip the HTML, and provide a working migration path. This issue (when combined with some other HTML mangling/sanitization) is causing havoc.
There was a problem hiding this comment.
I mean the block saving is not idempotent. If you save the same post twice without changes, you get different attributes.
What if we do this:
- When adding an image, use the caption as a fallback for the alt property. (and escape it)
- Remove the fallback in save
Would that work?
There was a problem hiding this comment.
I'll add that I believe that we need some logic like this in the save anyway, for the purpose of the pseudo-migration for alt attributes that were touched with GB 9.2 active, and had HTML inserted. (I don't think we can solve that solely at edit level.) And since the change (at save) level is sufficient also for newly created galleries, there's nothing left to do at edit level.
There was a problem hiding this comment.
I'll add that I believe that we need some logic like this in the save anyway, for the purpose of the pseudo-migration for alt attributes that were touched with GB 9.2 active, and had HTML inserted.
For me personally, I don't see that as a big issue. I see the conceptual issue as more important: (the conceptual issue being the save function not idempotent creating different block attributes without touching the editor).
How many persons are affected by this issue and for Core, it's still a development plugin.
There was a problem hiding this comment.
I'd also note that in all cases, the user has to open and save the post anyway since migrations don't run automtically.
There was a problem hiding this comment.
For me personally, I don't see that as a big issue. I see the conceptual issue as more important: (the conceptual issue being the save function not idempotent creating different block attributes without touching the editor).
I'm all for purity of code and all that, but it's the impact of this issues that worries me: Post content actually gets broken, so people will have to revert to an earlier revision (and might lose the changes they were working on). OTOH, the feature that was added by the PR that caused this is an (arguably minor) a11y improvement. IMO, those aren't strong enough reasons to keep around a feature that has such negative side effects.
How many persons are affected by this issue and for Core, it's still a development plugin.
Please refer to Automattic/wp-calypso#46813. I'm counting ~15 so far (some of those comments refer to the WP.com forums, where each thread can potentially contain multiple reports. Here's a few:
https://wordpress.com/forums/topic/gallery-block-problem/
https://wordpress.com/forums/topic/galleries-not-showing-up-right/
https://wordpress.com/forums/topic/unreadable-appearance-of-gallery-page-after-editing/
https://wordpress.com/forums/topic/cant-make-changes-to-website-and-drop-down-menus-are-not-working/
https://wordpress.com/forums/topic/galleries-not-showing-up-right/
I'd also note that in all cases, the user has to open and save the post anyway since migrations don't run automtically.
That's correct, but that's how migrations work, isn't it? This could be addressed by adding additional tooling to e.g. 'fix' the database upon plugin upgrade, but let's not get the lack of a perfect solution in the way of a sufficient one.
Needing to open and save can at least help with the cases where users with GB 9.2 were editing their galleries and because of the issue lost content, revert the post to a previous revision, and would like to re-apply those changes.
Anyway, is the compromise to simply remove the image.caption fallback for alt? I'd be okay with that, for the sake of at least preventing any further breakage into 9.3, even if it means that we won't unbreak existing content.
There was a problem hiding this comment.
Anyway, is the compromise to simply remove the
image.captionfallback foralt? I'd be okay with that, for the sake of at least preventing any further breakage into 9.3, even if it means that we won't unbreak existing content.
Filed the alternative PR: #26676
I put a diff --git a/packages/block-library/src/gallery/deprecated.js b/packages/block-library/src/gallery/deprecated.js
index 8542eb5bfc..b0ed9d7ea1 100644
--- a/packages/block-library/src/gallery/deprecated.js
+++ b/packages/block-library/src/gallery/deprecated.js
@@ -99,7 +99,9 @@ const deprecated = [
anchor: true,
align: true,
},
- isEligible( { images } ) {
+ isEligible( props, blcks ) {
+ console.log( 'isElig', props, blcks );
+ const { images } = props;
return (
images &&
images.some(
@@ -107,7 +109,8 @@ const deprecated = [
)
);
},
- migrate( attributes ) {
+ migrate( attributes, blcks ) {
+ console.log( 'migrate', attributes, blcks );
return {
...attributes,
images: attributes.images.map( ( image ) => {
|
There was a problem hiding this comment.
This LGTM (but we should look into the failing tests -- maybe a rebase is enough).
I'd like to get final approval by @youknowriad since we ended up going with a save-based approach; see the conversation starting here for the rationale.
Thanks a lot for this fix, @WunderBart!
...and strip it from the HTML if present.
Co-authored-by: Bernie Reiter <ockham@raz.or.at>
ockham
force-pushed
the
fix/img-caption-html
branch
from
57986d8 to
138e640
Compare
|
See my comment on the alternative PR proposed by @ockham #26676 (comment). |
|
Obsoleted by #26676. |
|
Thanks again for working on this and exploring different options to solve it, @WunderBart! |
Fixes #26549
Description
When a gallery image's
altattribute value is empty, we use what's in thecaptionprop to fill it out. The issue is thatcaptioncan contain links, whereas thealtattribute should not contain any HTML. This PR makes sure thealtattribute when created fromcaption, receives a value stripped out of HTML.How has this been tested?
Manually:
altattribute doesn't contain any HTML.To run unit tests covering the change:
Types of changes
Bugfix
Checklist:
TODO
migratealtattribute.