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

[Tooltip] Tooltip arrows are incompatible with text-wrap: pretty #44563

Open
rileyjshaw opened this issue Nov 26, 2024 · 14 comments
Open

[Tooltip] Tooltip arrows are incompatible with text-wrap: pretty #44563

rileyjshaw opened this issue Nov 26, 2024 · 14 comments
Assignees
Labels
bug 🐛 Something doesn't work component: tooltip This is the name of the generic UI component, not the React module! external dependency Blocked by external dependency, we can’t do anything about it package: material-ui Specific to @mui/material

Comments

@rileyjshaw
Copy link

rileyjshaw commented Nov 26, 2024

Steps to reproduce

Steps:

  1. Open this link to live example
  2. Notice that the word “orphan” is on its own line, despite text-wrap: pretty being enabled.
  3. Uncheck the arrow switch; text-wrap: pretty will now work as expected.

Current behavior

The arrow span is interpreted as a word in the text-wrap: pretty layout algorithm.

Expected behavior

The arrow span should not affect text wrapping.

Context

I think the easiest way to fix this is to simply insert the arrow span before { children }.

Your environment

npx @mui/envinfo
On latest Chrome, latest Codesandbox.

Search keywords: tooltip, text-wrap, arrow

@rileyjshaw rileyjshaw added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Nov 26, 2024
@aarongarciah aarongarciah self-assigned this Nov 27, 2024
@aarongarciah aarongarciah added component: tooltip This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material labels Nov 27, 2024
@aarongarciah
Copy link
Member

@rileyjshaw I can reproduce the issue. I'm very surprised the arrow span, which has position: absolute, is affecting the text-wrap: pretty algorithm. Your suggestion works and I think it shouldn't affect any other aspect of the component. Would you like to open a PR with the proposed change?

@aarongarciah aarongarciah added status: waiting for author Issue with insufficient information enhancement This is not a bug, nor a new feature and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Nov 27, 2024
rileyjshaw added a commit to rileyjshaw/material-ui that referenced this issue Nov 27, 2024
@rileyjshaw
Copy link
Author

@aarongarciah I agree—surprising right? I just opened a PR with the fix: #44579.

@github-actions github-actions bot added status: waiting for maintainer These issues haven't been looked at yet by a maintainer and removed status: waiting for author Issue with insufficient information labels Nov 27, 2024
@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 27, 2024

Could we report this to Chromium https://issues.chromium.org/issues?q=text-wrap%20pretty&p=1&s=status:asc&s=type:asc with a simplified test case and see what they say before we take any steps here?

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work and removed enhancement This is not a bug, nor a new feature labels Nov 27, 2024
@rileyjshaw
Copy link
Author

@oliviertassinari I can report this to Chromium, but is there any downside to merging this in the meantime? As far as I can tell, the spec is vague on how this should behave.

@siriwatknp
Copy link
Member

Could we report this to Chromium https://issues.chromium.org/issues?q=text-wrap%20pretty&p=1&s=status:asc&s=type:asc with a simplified test case and see what they say before we take any steps here?

Yes, I agree to wait for the response from Chromium first.

@siriwatknp siriwatknp added external dependency Blocked by external dependency, we can’t do anything about it and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Dec 2, 2024
@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 2, 2024

I can report this to Chromium

@rileyjshaw Thanks. They seem to have confirmed it's a bug https://issues.chromium.org/issues/381270274?

Is there any downside to merging this in the meantime?

I imagine there isn't a downside, we could try to merge this and see. It's more that it feels backward to fix something in the wrong abstraction layer.

@rileyjshaw
Copy link
Author

@oliviertassinari no problem! I’m glad Chromium is looking into it as well.

I understand wanting to avoid extra code weight or complexity for an upstream issue. But since this change just swaps two elements with an arbitrary order, this feels like a safe and straightforward improvement to merge in the meantime.

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 2, 2024

Maybe we can wait a few weeks to see the pace at which Chrome handle this. If they are too slow, we move forward here?

We could also argue that we should fix this in https://master--base-ui.netlify.app/components/react-tooltip#arrow too.

@aarongarciah
Copy link
Member

aarongarciah commented Dec 2, 2024

@oliviertassinari Base UI is unstyled (except very specific functional styles); I think Base UI should not care about this.

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 2, 2024

@aarongarciah I see a few ways to look at this:

@aarongarciah
Copy link
Member

@oliviertassinari you're right, I misunderstood your comment.

@siriwatknp
Copy link
Member

I propose to add a new prop to render the title at the end instead. This way, we have a workaround for v6 and add a comment that it might become the default behavior in v7 (based on how Chrome handles it).

@oliviertassinari
Copy link
Member

@siriwatknp what would be the advantage of a new prop?

@rileyjshaw
Copy link
Author

Maybe we can wait a few weeks to see the pace at which Chrome handle this. If they are too slow, we move forward here?

We could also argue that we should fix this in https://master--base-ui.netlify.app/components/react-tooltip#arrow too.

@oliviertassinari Chrome haven’t moved on this yet. And let me know if a PR to Base UI would be helpful!

I propose to add a new prop to render the title at the end instead. This way, we have a workaround for v6 and add a comment that it might become the default behavior in v7 (based on how Chrome handles it).

@siriwatknp I can’t see much advantage to adding a prop. Aside from the specific text-wrap bug we’re discussing here, I can’t think of any functional difference between having the empty, position: absolute span come before or after the title text. And people might confuse that prop with meaningful props like placement="top-start".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: tooltip This is the name of the generic UI component, not the React module! external dependency Blocked by external dependency, we can’t do anything about it package: material-ui Specific to @mui/material
Projects
None yet
Development

No branches or pull requests

4 participants