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: Add fixed table-layout to tables within full post view #1238

Closed
wants to merge 1 commit into from

Conversation

alexjgustafson
Copy link

Fixes #986 , to keep tables from overflowing within Reader's full post view in firefox

Testing instructions:

  1. Publish a new post including a table with images in two 50% width colums
  2. View the post in the Reader
    Result: the table is not constrained to the content area when the post is viewed in the Reader using Firefox.

Before:
before
before-mobile

After:
after
after-mobile

cc: @blowery @designsimply

@alexjgustafson alexjgustafson added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 3, 2015
@@ -217,6 +217,10 @@
}
}

table {
table-layout: fixed;
Copy link
Contributor

Choose a reason for hiding this comment

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

That seems... dangerous: https://developer.mozilla.org/en-US/docs/Web/CSS/table-layout

Under the "fixed" layout method, the entire table can be rendered once the first table row has been downloaded and analyzed. This can speed up rendering time over the "automatic" layout method, but subsequent cell content may not fit in the column widths provided. Any cell that has content that overflows uses the overflow property to determine whether to clip the overflow content.

Why does setting fixed fix the issue? Is there another way we could do it without changing how most tables render?

@blowery blowery added [Type] Bug [Feature] Reader The reader site on Calypso. [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 3, 2015
@blowery blowery added this to the Reader: December Bug Scrub milestone Dec 3, 2015
@alexjgustafson
Copy link
Author

The setting defaults to auto, with that in place for our test case, the photos are the widest unbreakable content in their respective columns. The table element has a width:100%; style in place, and each td has a width of 50% in place, but because the photos are the widest unbreakable content the photos set their respective columns width regardless of the table's width. Because of that, it's stretching out the table's cells past the containing .reader__full-post-content div, even though the table has those widths set.

It should be noted this effect occurs in other environments, not just the Reader. Here's the sample HTML provided in the issue in a post in the Twenty Fourteen theme:

blog___923_is_numbered__not_lettered___soon_we_ll_be_spelling_everything_with_letters

The Twenty Fifteen theme handles this issue by using table-layout:fixed for all table elements. Here's the sample HTML provided in the issue in a post in the Twenty Fifteen theme:

overflow_display_in_firefox___923_is_numbered__not_lettered

Setting table-layout to fixed fixes the issue because the table renders based off the widths set in the tables first row rather than the contents within the table's cells. The content's width then gets handled within the cell as a container, following any overflow rules.

To produce the same effect without using table-layout: fixed I suppose we'd need to make adjustments to the content itself when found within a table. Images seem the most likely element to cause this width issue. Non-wrapping text would cause this effect too, but that happens even when the table-layout is fixed, and seems we handle it in the Reader view already. A table nested within a table may also cause this effect with the right content.

I'll attempt a solution that at least accounts for the image case, and test out how other elements react to see if fixing more than images is necessary.

@blowery
Copy link
Contributor

blowery commented Dec 4, 2015

The odd thing is that is only does it in firefox. I wonder if this is a vague bit of the spec, or if Firefox is getting it wrong..

@alexjgustafson
Copy link
Author

I was hoping I could figure out what it is that Chrome and Firefox handle differently by using the same table setup without all the other styles already in Calypso, including normalize/reset styles that are in play. So I started working with this HTML on its own: https://gist.github.com/alexjgustafson/cf3fe38df0d15e52a675

When viewing that HTML alone, the images overflow the table in both Chrome and Firefox, regardless of table-layout. So I think you're right, there must be some other styles we already have in place that Chrome is supporting and Firefox just isn't. Having a hard time tracking down exactly what those are though.

@blowery
Copy link
Contributor

blowery commented Jan 26, 2016

@alexjgustafson Have you had a chance to look at this again?

@alexjgustafson
Copy link
Author

@blowery a couple times I've attempted to find other solutions but I'm afraid I'm stumped beyond my original PR.

@lancewillett
Copy link
Contributor

Closing this one as no movement in several months — feel free to re-open if it's actionable and can be pushed in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Reader The reader site on Calypso. [Type] Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants