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

Fix some x overflows #1617

Merged
merged 5 commits into from
Jun 27, 2022
Merged

Fix some x overflows #1617

merged 5 commits into from
Jun 27, 2022

Conversation

lf-
Copy link
Contributor

@lf- lf- commented Jul 30, 2021

This PR fixes long inline code doing an x overflow.

There's another x-overflow on this page, where the table with the heading "punycode + encoding" also overflows slightly. I have fixed this one as well.

Spotted on
https://rust-lang.github.io/rfcs/2603-rust-symbol-name-mangling-v0.html

@ehuss
Copy link
Contributor

ehuss commented Sep 8, 2021

Thanks, I have a few questions, though:

What does the overflow-x do specifically? What circumstances are there when the tables overflow? Can you show screenshots of the differences?

Can you say more about why this uses a div wrapper? Is it a concern about <table> being used by other things?

Are there any concerns about using word-break: break-word;? According to MDN it is deprecated. I'm not sure what deprecated even means in this case. Would it maybe be better to use overflow-wrap: anywhere?

@lf-
Copy link
Contributor Author

lf- commented Sep 8, 2021

Thanks for the review! Looking deeper into these questions has been useful for me as a CSS non-expert.

What does the overflow-x do specifically? What circumstances are there when the tables overflow? Can you show screenshots of the differences?

Tables overflow like this:
image

The conditions in which this happen are when there are long cells (I stuffed some extra long text into the cell to get it to overflow a little harder) or generally more width than fits on the screen.

This is what the overflow-x: auto on the wrapper div does: it makes the table scrollable horizontally rather than making the entire page scrollable horizontally:

image

The reason it needs to be on a wrapper div with the overflow-x: auto on it — or at least my best guess of it based on reading MDN — is that table elements are display: table rather than display: block (indeed overriding them to display: block has it scroll the table itself. It appears, however, that the element then doesn't end up centered with the margin: 0 auto on it, so I think we probably need the wrapper?). This SO answer claims that display: table in some parent is required for table cell rendering to work properly.


Are there any concerns about using word-break: break-word;? According to MDN it is deprecated. I'm not sure what deprecated even means in this case. Would it maybe be better to use overflow-wrap: anywhere?

I somehow missed the deprecation notice, my bad (from further googling, it was deprecated as of CSS 3). I can confirm that overflow-wrap: anywhere gets the same effect.

From putting some long stuff into a paragraph, I realized we probably should enable this everywhere on the app since it's possible to cause x overflows with body text too. I am going to do that:

image

overflow-wrap: always on main:

image

Some code, overflowing without word breaking configured:

image

With wrap turned on:
image

@YJDoc2
Copy link
Contributor

YJDoc2 commented Oct 5, 2021

@lf- @ehuss please take a look :
Following tests are done on the test_book, which I have requested to be added in PR #1662, it can be found in my fork at https://github.com/YJDoc2/mdBook/tree/test-book

I think rather than overflow-wrap: anywhere , overflow-wrap: break-word will be a better choice :
I checked this on Linux + Firefox + (Desktop mode / Mobile mode) and Linux + Brave (chromium based) + (desktop/mobile) mode, following results are consistent on both.

Original

This is without the changes in this PR
Original table desktop
original-table-desktop

Original Table mobile
original-table-mobile

As you can see this makes the whole page scroll in mobile, rather than the table (evident by the page title being off screen), which was the issue.

After this PR

The issue is due to overflow-wrap: anywhere, this also breaks the words which should not be broken, and the tables don't scroll
For example, on desktop mode :
PR-desktop
this breaks words col2 and val2, which definitely should not have been broken on desktop.
On mobile, it is worse :
PR-mobile

Here, to prevent overflow, it breaks every word into characters and wraps them around.

With suggested changes

Now with using

main {
    overflow-wrap: break-word;
}

(table wrapper class introduced in this PR is kept as in this PR)
we get :
on desktop :
suggested-desktop

and on mobile :
suggested-mobile

where it scrolls the table, rather than scrolling the whole page, as evident by the page title staying in view, and the columns being scrolled.

@ehuss
Copy link
Contributor

ehuss commented Oct 24, 2021

Oh, yea, it looks like break-word looks better to me.

@ehuss
Copy link
Contributor

ehuss commented Jan 4, 2022

@lf- Would you be willing to update this using break-word?

@ehuss ehuss added the S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. label Jan 4, 2022
@lf-
Copy link
Contributor Author

lf- commented Jan 28, 2022

Done and rebased.

@lf-
Copy link
Contributor Author

lf- commented Jun 17, 2022

@ehuss if you missed the notif

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

Thanks! I'm going to go ahead and merge, though I still have some concerns about how this might impact or break things. I can't think of any specific problems, and testing on a variety of browsers seems to be OK.

@ehuss ehuss merged commit 5f00625 into rust-lang:master Jun 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants