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

Add overflow-wraps to several elements currently overflowing #1558

Merged
merged 12 commits into from
Mar 31, 2025

Conversation

LunarWatcher
Copy link
Contributor

@LunarWatcher LunarWatcher commented Mar 9, 2025

Various instances of fixed overflow:

  • Tables, particularly the user activity table when the edit summary overflows. The fix here is overflow-wrap: anywhere;, because overflow-wrap: break-word did nothing - not sure if this has any unintended consequences, but it doesn't appear to have.
  • Titles (post listing, question page)
  • Body (post listing, post preview)
  • Tag wikis (summary, body)
  • Revisions, specifically the diff
  • ... and possibly more places affected by the modified/added CSS selectors

Comments, which were the initial problem with #1478, appear to have been fixed. Since no source was linked on the meta post, I couldn't see the original, but a quick test setup with inline code showed that it did indeed wrap within width under 306284d. Comment content for the test was just a long series of letters without spaces wrapped in an inline codeblock, because that was the text that overflowed.

No title was used for this comment, however, so the auto-generated title did overflow. That has also been fixed, so comment thread titles now also wrap properly.

Various unhandled edge-cases:

  • The "about me" currently handles it by adding horizontal scrollbars. Didn't touch this for now, what to do about it (do what everything else does, or accept it on account of the "about me" being special) needs to be handled separately
  • Though not described in any current issues, zalgo does overflow into other elements. It's pretty easy to fix in pure CSS (overflow: clip), but it requires adding it to quite a few more CSS classes, and just adding it everywhere feels unnecessary

On that note, I'm not sure if it would be better to remove some of the added overflow-wraps and throw then in a separate CSS class (has-overflow-break-word or something), but this would require the classes1 added to a lot of elements for it to work properly.


Closes #802
Closes #1249
Closes #1478

Footnotes

  1. break-word is the most common use-case, but anywhere has a few applications as well - though nowhere near as many. Some also need an explicit width for break-word to be properly respected, which may or may not be defined already for any given element.

@cellio cellio requested a review from a team March 9, 2025 23:26
@Oaphi
Copy link
Member

Oaphi commented Mar 10, 2025

To check (if somebody gets to any of these items before I do, feel free to mark as complete):

  • Tables, particularly the user activity table;
  • Titles (post listing, question page);
  • Body (post listing, post preview);
  • Tag wikis (summary, body);
  • Revisions, specifically the diff;

@Oaphi
Copy link
Member

Oaphi commented Mar 10, 2025

@LunarWatcher fixed another instance where we have an overflow:
Screenshot from 2025-03-10 11-44-42

I've added a wrap-anywhere utility CSS class, but don't take it as wanting to use it for the rest of the elements: as you know, I hate "utility classes" with a passion. We'll make a proper CSS component for such titles at a later point (hopefully)

@Oaphi
Copy link
Member

Oaphi commented Mar 10, 2025

Not sure what to do about tag names, though - break-word is certainly better than nothing, but it still leaves us with this corner case:

Screenshot from 2025-03-10 11-53-25

I am hesitant to just slap an overflow-wrap: anywhere there (although it's an option). Ideally, we should add max-width and clip with an ellipsis, with the tooltip showing the full name on hover, but that's out of scope for the PR, I believe, don't take it as a request to address.

@Oaphi
Copy link
Member

Oaphi commented Mar 10, 2025

Note to self (I think we already have it logged, but just in case) - tags with very long names can be created (seems like our validation checks are lacking) but cannot be used:
Screenshot from 2025-03-10 11-59-12

@trichoplax
Copy link
Contributor

Note to self (I think we already have it logged, but just in case) - tags with very long names can be created (seems like our validation checks are lacking) but cannot be used

I can't see an issue for this by searching for either "tag" or "tags" so we might not have it logged yet. I've only looked through the issue titles, so it's possible it's mentioned in the description or discussion of a related issue, but in that case it's probably worth it having its own issue anyway, so I've added #1559

@trichoplax
Copy link
Contributor

To check (if somebody gets to any of these items before I do, feel free to mark as complete)

Should we include #1249 in this issue as part of this list of things to check?

@cellio
Copy link
Member

cellio commented Mar 10, 2025

Should we include #1249 in this issue as part of this list of things to check?

If it's easy to do here, let's. If we encounter any complications, I think it's ok to defer because we already have an issue tracking that (so it won't be lost).

On the long-tag-name case, a clip with ellipsis seems better than wrapping (doesn't have to be here), but: if we properly validate the length, does the case arise? How long can a tag name be without causing problems (remember smaller tablets)? Tag length can be configured (35 is the default, not baked in), but if any reasonable length would work, then we could fix the bug that allows long names to be created and not worry about how to wrap too-long tags. At least until somebody sees a use case for 100-character tags or something...

@LunarWatcher
Copy link
Contributor Author

@Oaphi:

To check (if somebody gets to any of these items before I do, feel free to mark as complete):

For the sake of bookkeeping, there's also comment thread titles, but it wasn't a separate bullet because reasons

@trichoplax:

Should we include #1249 in this issue as part of this list of things to check?

I'll check when I get around to it - did look for other issues, but didn't find that one. There's also #802, but I've been unable to reproduce it, so it might be fixed already

@cellio
Copy link
Member

cellio commented Mar 10, 2025

There's also #802, but I've been unable to reproduce it, so it might be fixed already

I see the bug in the Math post linked in the issue. From discussion there, it seems to show up at some viewport sizes/zoom levels and not others.

@LunarWatcher
Copy link
Contributor Author

Oh, one of those bugs. I tried to reproduce it at several zoom levels and screen sizes, but couldn't. I'll try again later with more pixel-perfect resizing then

@cellio
Copy link
Member

cellio commented Mar 10, 2025

#802

FWIW, my browser reports that it is currently 1160px wide and at 100% zoom I see the bug on that Math answer (Brave, Chrome). I haven't done more exhaustive testing, but that's a datum, anyway.

Not sure why this works, but MathJAX includes an assistive MathML
container, and _that_ container appears to be the source of the
overflow:

> mathjax/MathJax#2936 (comment)

The bug can also be fixed by overriding the positioning to not be
`relative`, but I don't know if that has unintended consequences, and
testing if it breaks positionally dependent mathjax is far more
difficult than testing if it anything renders weirdly as a result of the
max width statement.

Firefox renders identically with or without it.

There is also a small chance (based on comments) that Safari is
affected by the same bug, but I use Linux and can't spin up a macOS VM
to check (thanks, Apple).

Closes codidact#802
@LunarWatcher
Copy link
Contributor Author

d8322f4 probably needs a bit more manual testing than bbacd33, since the latter is limited to mathjax elements in any context. I don't think this should have unintended consequences, but I don't have that many test cases in my dev instance yet. Checked with the obvious troublemaker (tables), and it rendered fine:

Screenshot showing a table with 5 columns and one non-header row, where each cell just consists of a long string of random characters, with no table overflow

@LunarWatcher
Copy link
Contributor Author

LunarWatcher commented Mar 10, 2025

... bbacd33 might've broken long block equations. Checking now

Update: It did not, I checked out develop, and it's still borked. The test post is https://math.codidact.com/posts/286292, and the live version renders correctly, but not on develop. Weirdly, the master branch is also borked on my machine, so maybe there's custom CSS on prod?

Update 2: it wasn't, it was an HTML rendering difference. Changes to HTML generation means that mjx-container is no longer guaranteed to be in a <p>, unless there's also text that gets included along with the container, so the new overflowing mathjax containers are actually the fault of HTML generation. Any edits to math:286292 right now should cause overflow. I'd test, but math blocks don't render in the editor for some reason, and I don't have the privileges to just quietly edit it, and I don't want to disrupt anything. This is fixed in 1dd65f5 by adding x overflow to mjx-container, which also keeps associated text from scrolling along with the math (arguably a nice upgrade)

At Some Point:tm:, the markdown generator stopped adding `<p>` wrappers
around `mjx-container`, so any new posts with mathjax blocks (not inline
mathjax), or existing posts that are edited, will start to overflow. (I
think, I haven't checked if HTML is actually cached)

For posterity, the HTML prior to whatever change happened can be seen on
archive.org:

https://web.archive.org/web/20250310184547/https://math.codidact.com/posts/286292
@cellio
Copy link
Member

cellio commented Mar 10, 2025

Update: It did not, I checked out develop, and it's still borked. The test post is https://math.codidact.com/posts/286292, and the live version renders correctly, but not on develop. Weirdly, the master branch is also borked on my machine, so maybe there's custom CSS on prod?

I can repro -- live Math post uses horizontal scroll and does not overflow; a copy of that same post in my dev environment sometimers overflows and sometimes uses h-scroll. In this screenshot I'm seeing different behavior for consecutive math blocks (though they're separated by single-line text); don't know yet if that's related:

Screenshot

@LunarWatcher
Copy link
Contributor Author

@cellio See update 2 in that comment, I figured out why it's being weird

@cellio
Copy link
Member

cellio commented Mar 10, 2025

Oh good; I was working on assembling the list of mathjax for each of four equations of interest, but looks like I don't need to.

@cellio
Copy link
Member

cellio commented Mar 21, 2025

Where does this PR stand? Are we waiting for review, or are reviewers waiting for changes?

@LunarWatcher
Copy link
Contributor Author

@cellio Awaiting either review, or confirmation of a performed review. No changes have been requested yet, unless I missed something on Discord or something

Copy link
Member

@Oaphi Oaphi left a comment

Choose a reason for hiding this comment

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

@LunarWatcher I've added one more fix while we are at it (post history, initial revision) - with a scroll on overflow (preserves your fixes + ensures diffs display reasonably well when we can't wrap). CircleCI went into maintenance, so let's wait until it's up again before merging (just in case), and once it is - LGTM to me too.

Before:
Screenshot from 2025-03-28 02-40-27

After:
Screenshot from 2025-03-28 02-38-42

P.S. Screenshots do not start from the same position, the width is the same

@Oaphi
Copy link
Member

Oaphi commented Mar 28, 2025

And one other small fix for tags to ensure they don't overflow the container even in the worst cases.

Before:
Screenshot from 2025-03-28 02-58-43

After:
Screenshot from 2025-03-28 03-00-08

@Oaphi
Copy link
Member

Oaphi commented Mar 28, 2025

Last one, I swear: tags container in the posts list (once again - not a flaw with the PR, another extra thing I noticed).

Before:
Screenshot from 2025-03-28 03-06-52

After:
Screenshot from 2025-03-28 03-06-55

@Oaphi
Copy link
Member

Oaphi commented Mar 28, 2025

There's one other place we might want to address, but I am out of time to spare, unfortunately (and what's in the PR is certainly much more than enough as it is) - tag merge titles:

image

@Oaphi Oaphi added the area: frontend Changes to front-end code label Mar 28, 2025
@cellio
Copy link
Member

cellio commented Mar 28, 2025

There's one other place we might want to address, but I am out of time to spare, unfortunately (and what's in the PR is certainly much more than enough as it is) - tag merge titles:

That can wait. But what happened to the tests? Is that the maintenance you mentioned? Will we need to manually kick them off later?

@Oaphi
Copy link
Member

Oaphi commented Mar 28, 2025

There's one other place we might want to address, but I am out of time to spare, unfortunately (and what's in the PR is certainly much more than enough as it is) - tag merge titles:

That can wait. But what happened to the tests? Is that the maintenance you mentioned? Will we need to manually kick them off later?

Yup, all workflows are failing right now due to CircleCI's maintenance - if there will be no new commits after they wrap up, we will have to rerun the jobs manually (or they'll run automatically upon merge)

@Oaphi Oaphi merged commit 219c1bd into codidact:develop Mar 31, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: frontend Changes to front-end code
Projects
None yet
5 participants