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

Reader: Keep inline styles in post content #46720

Closed
wants to merge 2 commits into from

Conversation

mmtr
Copy link
Member

@mmtr mmtr commented Oct 23, 2020

Fixes #44992.
Helps with #43595.

Changes proposed in this Pull Request

The reader API endpoints return posts with content that might include inline styles provided by the Gutenberg blocks fallbacks plugin (D50508-code). However, Calypso was stripping out those styles as part of a normalization process. This PR removes the piece of logic that was removing the inline styles so they are kept in the post content rendered in the reader.

Before After
Screen Shot 2020-10-23 at 15 03 13 Screen Shot 2020-10-23 at 15 03 18 Requires D51569-code
Screen Shot 2020-10-23 at 15 00 25 Screen Shot 2020-10-23 at 15 20 21 They look the same, but we use less code now given that most styles are coming from Gutenberg.
Screen Shot 2020-10-23 at 15 35 33 Screen Shot 2020-10-23 at 15 35 42

Testing instructions

  • Apply D51569-code and sandbox the API.
  • Make sure the block fallbacks cache is turned off by adding define( 'BLOCK_FALLBACKS_DEBUG', true ); to your /wp-content/mu-plugins/0-sandbox.php file.
  • Go to Reader and check different post.
  • Make sure the block styles are applied to what's rendered.

@mmtr mmtr added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Feature] Reader The reader site on Calypso. Block Rendering labels Oct 23, 2020
@mmtr mmtr requested a review from a team October 23, 2020 13:45
@mmtr mmtr requested a review from a team as a code owner October 23, 2020 13:45
@mmtr mmtr self-assigned this Oct 23, 2020
@matticbot
Copy link
Contributor

@matticbot
Copy link
Contributor

matticbot commented Oct 23, 2020

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~163 bytes removed 📉 [gzipped])

name    parsed_size           gzip_size
reader       -784 B  (-0.1%)     -163 B  (-0.1%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~533 bytes removed 📉 [gzipped])

name                                                     parsed_size           gzip_size
async-load-design-blocks                                      -784 B  (-0.0%)     -207 B  (-0.0%)
async-load-calypso-components-web-preview-component           -784 B  (-0.1%)     -163 B  (-0.1%)
async-load-calypso-blocks-support-article-dialog-dialog       -784 B  (-0.8%)     -163 B  (-0.6%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

@krymson24
Copy link
Contributor

This is what I get back for content:

content: "<div class="wp-block-buttons">↵<div class="wp-block-button"><a class="wp-block-button__link">button1</a></div>↵↵↵↵<div class="wp-block-button"><a class="wp-block-button__link">button2</a></div>↵↵↵↵<div class="wp-block-button"><a class="wp-block-button__link">button3</a></div>↵</div>↵↵↵↵<h2 class="has-text-align-center has-secondary-color has-background-dark-background-color has-text-color has-background">heading</h2>"

This is what I see:

Screen Shot 2020-10-23 at 5 25 09 PM

@mmtr mmtr force-pushed the remove/reader-normalize-styles branch from 0d906ec to 96a65d8 Compare October 26, 2020 11:30
@bluefuton
Copy link
Contributor

@mmtr just to clarify - won't this leave all inline styles in place, not just Gutenberg ones? Can we be more selective?

I think we stripped them originally because the user-defined inline styles often don't render well in Reader.

@gibrown
Copy link
Member

gibrown commented Oct 26, 2020

@mmtr all of this feels like it should be handled server side in a more consistent manner rather than just in Calypso. This won't deal with any issues in the mobile apps or elsewhere if we only do it here.

@kwight
Copy link
Contributor

kwight commented Oct 26, 2020

@gibrown I gave some additional background in paYKcK-KO-p2.

won't this leave all inline styles in place, not just Gutenberg ones? Can we be more selective?

It would, yes. Maybe we could add a class marker to an element (has-gutenberg-styles) which could be identified by removeStyles and allowed to pass?

I think we stripped them originally because the user-defined inline styles often don't render well in Reader.

Does this refer to things like strikethrough and underline?..

@krymson24
Copy link
Contributor

Still seeing this 😢 Screen Shot 2020-10-26 at 9 10 23 PM

@krymson24
Copy link
Contributor

krymson24 commented Oct 27, 2020

@kwight, @gibrown I believe the [inline_block_styles](D50508-code) fallback that @mmtr wrote is being applied to the Reader content and passing through inline styles, with some exceptions mentioned here: D51569-code#1047745, so this isn't just being applied on the frontend. Unfortunately, for whatever reason, my Reader instance in particular isn't show anything of these styles, but I believe there's something not quite right with my local environment.

@bluefuton I think there's new context here. My understanding is that the Reader posts are supposed to look exactly like the original post, and our work here is to enable more rich content and styles here to flow through. What was previously styled content that doesn't render well, we want to gradually make better.

@gibrown
Copy link
Member

gibrown commented Oct 27, 2020

How about expanding on how the normalizer was already allowing certain styles through?

export default function removeContentStyles( post, dom ) {

After poking at the code more I rescind my attempt to have this happen server side. I think I remember making that argument years ago too. Sigh...

My understanding is that the Reader posts are supposed to look exactly like the original post

@krymson24 this has never fully been possible. There are lots of styles someone could add to their content that would look terrible in the Reader. That is why we build our own excerpts for instance where the streams look terrible if you just take whatever the user gives you. We definitely should continue to make things better, but it is almost certainly going to break a lot of things if we completely remove any of the normalization steps. Let's improve what is there and add more tests along the way to make it easier.

@@ -248,28 +248,17 @@
margin: auto;
}

.wp-block-buttons {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why were these classes removed? (They aren't the correct styles, admittedly, but I feel like we still need them, without knowing a better place to put them)

@bluefuton
Copy link
Contributor

bluefuton commented Oct 27, 2020

Does this refer to things like strikethrough and underline?

No, more that any style the user adds inline (e.g. using a Custom HTML block) ends up making it to Reader with this change.

Screen Shot 2020-10-27 at 16 44 50

https://hash-96a65d8c72cff8e86eb15e46d6c559e887609d03.calypso.live/read/feeds/40474296/posts/2988003678

The display of content in Reader has always been opinionated - we respect certain structural basics of the layout (like tables, headings, and so on) but we make the decisions about text size, font, colour, which images or video to highlight, spacing, etc.

If we're changing that, it's a big change, and we should talk through the ramifications of it with Design.

@kwight
Copy link
Contributor

kwight commented Oct 27, 2020

The display of content in Reader has always been opinionated - we respect certain structural basics of the layout (like tables, headings, and so on) but we make the decisions about text size, font, colour, which images or video to highlight, spacing, etc.

Ah, I see – I didn't realize myself how opinionated it was. The goal of our block rendering project is to have blocks render in the Reader in a sensible, predictable way, but we certainly don't want to just regress any intentional design done up to now. It sounds like being more detailed in the post normalization handling will be a better way forward, so I'll close this for now and we can pick up elsewhere with a new approach.

@kwight kwight closed this Oct 27, 2020
@kwight kwight deleted the remove/reader-normalize-styles branch October 27, 2020 14:55
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Block Rendering [Feature] Reader The reader site on Calypso.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reader: centered paragraphs are displayed with left text alignment
6 participants