Broken story markup for users without unfiltered_html capabilities

[swissspidy]

Bug Description

I noticed this while debugging a story. Not sure how it happened, but in this case there was no closing </strong> tag on a text element.

Why P0?

This broken markup led to a completely broken layout on the frontend. It was impossible to read the story.

Possibly related: #413, #1007, #1093.

Expected Behaviour

There should never be broken markup like this.

Steps to Reproduce

Drafts:

1. Log in as user with role Author
2. Create a new story
3. Add a new text element
4. Make the text bold
5. Add a new image
6. Rotate image
7. Preview story
8. Notice how story is broken

Published stories:

1. Log in as user with role Author
2. Create a new story
3. Publish story
4. Add a new text element
5. Make the text bold
6. Add a new image
7. Rotate image
8. Preview story
9. Notice how story is broken

Screenshots

---

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance Criteria

QA Instructions

Drafts:

1. Log in as user with role Author
2. Create a new story
3. Add a new text element
4. Make the text bold
5. Rotate element
6. Add background color with opacity
7. Preview story
8. Verify story is displayed as expected

Published stories:

1. Log in as user with role Author
2. Create a new story
3. Publish story
4. Add a new text element
5. Make the text bold
6. Add a new image
7. Rotate image
8. Preview story
9. Story should be displayed as expected

[merapi]

Can you post the story_data that led to this output?

[swissspidy]

I'll try to see if there is a revision where I can grab it from

[dvoytenko]

As a quick fix we could try to always push the content through fakeElement.innerHTML = content; return fakeElement.content. Even better if the fakeElement is a template element.

[miina]

@dvoytenko Quick-fix based on your suggestion: #1169

Note: I was not able to replicate the issue locally, tried toggling the bold on/off together with various cmd+b options, thus, couldn't debug it for detecting the root cause. This, however, will likely get fixed when we stop using the bold toggle as element attribute and manipulate the content directly.

[swissspidy]

Interestingly this popped up again on the Stories Labs site, even after we revamped the formatting.

Maybe issues related to copy/paste text with formatting?

Reopening for investigating.

[barklund]

@swissspidy, specifically unmatched <strong> tags? Because text formatting isn't even using <strong> anymore, so that would be very weird.

Do you have some links or more info?

[miina]

@barklund Apparently the copy-paste logic still allows strong tags.

See https://github.com/google/web-stories-wp/blob/d82934bfb8095687d5b7a9be3bd6653465a3adef/assets/src/edit-story/utils/copyPaste.js#L31 and related code using it.

[barklund]

@barklund Apparently the copy-paste logic still allows strong tags.

DraftJs should actually strip all formatting on paste - because of line 89 here:

https://github.com/google/web-stories-wp/blob/d82934bfb8095687d5b7a9be3bd6653465a3adef/assets/src/edit-story/components/richText/editor.js#L83-L90

Is it not doing that?

And proper handling of converting pasted content formatting to our inline formatting is to be handled in #1605.

[miina]

Is it not doing that?

I didn't actually test it before, was thinking through options where could have the strong tag come from.

Did test it out now and the strong tag does indeed end up in the DB when pasting from somewhere.
To reproduce:

1. Copy some text which has bold in it
2. Select a Page and Paste (Cntl + V)
3. Save the Story
4. See the preview source (or the markup saved to DB)

[swissspidy]

I was able to reproduce this issue! 🎉

It's not really about <strong> tags, but about markup in general.

Prerequisites for testing

1. Create a new user with the role Author (or editor on Multisite)
2. Install and activate the User Switching plugin

Why Author role? Well, that role does not have the unfiltered_html capability, which means our story data and generated markup will be passed through wp_filter_post_kses in WordPress...and breaks.

To reproduce:

1. Log in as user with role Author
2. Create a new story
3. Add a new text element
4. Make the text bold
5. In the background, this will result in some markup like <span style="font-weight: 700">Fill in some text</span>
6. Preview story
7. Notice how story is broken

Meanwhile, the database will look like this:

post_content:

body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}
              amp-story-grid-layer {
                overflow: visible;
              }

              .page-fullbleed-area {
                position: absolute;
                overflow: hidden;
                width: 100%;
                left: 0;
                height: calc(1.1851851851851851 * 100%);
                top: calc((1 - 1.1851851851851851) * 100% / 2);
              }

              .page-safe-area {
                overflow: visible;
                position: absolute;
                top: 0;
                bottom: 0;
                left: 0;
                right: 0;
                width: 100%;
                height: calc(0.84375 * 100%);
                margin: auto 0;
              }

              .wrapper {
                position: absolute;
                overflow: hidden;
              }

              .fill {
                position: absolute;
                top: 0;
                left: 0;
                right: 0;
                bottom: 0;
                margin: 0;
              }
              <amp-story standalone="standalone" publisher="Web Stories" publisher-logo-src="https://web-stories.local/wp-content/plugins/web-stories-wp/assets/images/fallback-wordpress-publisher-logo.png" title="Author Test" poster-portrait-src="https://web-stories.local/wp-content/plugins/web-stories-wp/assets/images/fallback-poster.jpg"><amp-story-page id="4bc5a275-f372-4770-baaa-534820bfb5c2" auto-advance-after="7s"><amp-story-grid-layer template="vertical"><div class="page-fullbleed-area" style="background-color:#fff"><div class="page-safe-area"><div style="width:100%;height:118.51852%" id="el-c9cf75ea-f702-46a2-9bb1-df9ccdf5efeb" class="wrapper"><div class="fill"></div></div></div></div></amp-story-grid-layer><amp-story-grid-layer template="vertical"><div class="page-fullbleed-area"><div class="page-safe-area"><div style="width:36.36364%;height:2.72727%" id="el-4942e49e-c28d-4437-a7fd-4572e48dac54" class="wrapper"><p class="fill" style="margin:0;,sans-serif;font-size:0.212121em;line-height:1.3;text-align:initial;padding:0% 0%;color:#000000"><span style="font-weight: 700">Fill in some text</span></p></div></div></div></amp-story-grid-layer></amp-story-page></amp-story>

post_content_filtered:

{"version":21,"pages":[{"elements":[{"opacity":100,"flip":{"vertical":false,"horizontal":false},"rotationAngle":0,"lockAspectRatio":true,"backgroundColor":{"color":{"r":255,"g":255,"b":255}},"x":1,"y":1,"width":1,"height":1,"mask":{"type":"rectangle"},"isBackground":true,"isDefaultBackground":true,"type":"shape","id":"c9cf75ea-f702-46a2-9bb1-df9ccdf5efeb"},{"opacity":100,"flip":{"vertical":false,"horizontal":false},"rotationAngle":0,"lockAspectRatio":true,"backgroundTextMode":"NONE","font":{"family":"Roboto","weights":[100,300,400,500,700,900],"styles":["italic","regular"],"variants":[[0,100],[1,100],[0,300],[1,300],[0,400],[1,400],[0,500],[1,500],[0,700],[1,700],[0,900],[1,900]],"fallbacks":["Helvetica Neue","Helvetica","sans-serif"],"service":"fonts.google.com"},"fontSize":14,"backgroundColor":{"color":{"r":196,"g":196,"b":196}},"lineHeight":1.3,"textAlign":"initial","padding":{"vertical":0,"horizontal":0,"locked":true},"type":"text","id":"4942e49e-c28d-4437-a7fd-4572e48dac54","content":"<span style="font-weight: 700">Fill in some text","x":93,"y":130,"width":160,"height":18,"scale":100,"focalX":50,"focalY":50}],"backgroundColor":{"color":{"r":255,"g":255,"b":255}},"type":"page","id":"4bc5a275-f372-4770-baaa-534820bfb5c2"}],"autoAdvance":true,"defaultPageDuration":7}

@miina This might ring a bell for you, since we had to deal with this in the old editor as well: ampproject/amp-wp#2691

[dreamofabear]

I reproduced this on a new test account on stories-lab.dev. Adding a text element was not necessary for me -- a completely blank story will be broken. So this might be a separate bug unless I'm missing something.

My guess for the missing </strong> tag issue is that it was due to a missing migration path for stories created on an older release.

[swissspidy]

Thanks for testing!

Still the same root cause though. It‘s not about specific tags, just that there‘s markup at all (of which we have plenty).

[swissspidy]

Another manifestation of this same issue: author users cannot rotate elements.

[csossi]

Drafts (1st scenario)
Editor:

Preview:

[csossi]

Update to a published story:

Preview:

[swissspidy]

Note for UAT:

This has been on Stories Labs for a bit now, and users already reported that they were now able to successfully create and publish stories, thanks to this change.