[EuiText] Updated euiTextContrainedMaxWidth Value

[breehall]

Summary

Resolved a bug with EuiText that caused EuiPopover to have a length that extended beyond the width of the screen when the popover contains a lot of text. In #5977, euiTextConstrainedMaxWidth was converted with a max-width of max(64ch, 75%) as opposed to 36rem.

I've also opted for using the native CSS max-width property instead of the logicalCSS function with max-width as a parameter because it is converted to max-inline-size which was not respected by the popover.

Before

After

Checklist

- [ ] Checked in both light and dark modes

• Checked in mobile
  - [ ] Checked in Chrome, Safari, Edge, and Firefox
  - [ ] Props have proper autodocs and playground toggles
• Added documentation
  - [ ] Checked Code Sandbox works for any docs examples
  - [ ] Added or updated jest and cypress tests
• Checked for breaking changes and labeled appropriately
  - [ ] Checked for accessibility including keyboard-only and screenreader modes
  - [ ] Updated the Figma library counterpart
• A changelog entry exists and is marked appropriately

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_6146/

[thompsongl]

Just curious, if we keep this as max(64ch, 75%) but use max-width what are the results?

I'd be ok with a slight update to width as long as the constraint is reasonable

Original request for the change:
#5895 (comment)

[thompsongl]

Ok so it looks like max-inline-size is not the problem. It's the 75% param, which is much too wide.

Seems like we could keep logicalCSS('max-width') and update euiTextConstrainedMaxWidth to 64ch.

@constancecchen Any insight here from making this change initially?

[chandlerprall]

Thanks for digging further @thompsongl ! Looking at the .euiText--constrainedWidth css,

  &.euiText--constrainedWidth {
    max-width: $euiTextConstrainedMaxWidth;
    // If the max-width is way too short of the width of the container,
    // at least make it 2/3 of its parent
    min-width: 75%;
  }

it looks like the styles were incorrectly combined to max(64ch, 75%).

[cee-chen]

To be honest Caroline made the change/request within my PR, I didn't think too much about it 😅 The comment you linked (+ 32690fa) is the only context I had.

++ to max-inline-size not being the issue

++ to Chandler's discovery, it looks like what we actually want is min(64ch, 75%); instead of max()?

[thompsongl]

I like the min idea but it still doesn't seem to work. The popover is still huge in the Maps example.

[breehall]

My only guess as to why it was working correctly previously was that max-width takes precedence over min-width, which means the CSS was actually not working as the dev/code comment expected it to.

Very interesting. It "worked" in Kibana because it was "broken" in EUI. Then as a standard, consumers who are have large bits of content would be responsible for the sizing of the popover via CSS?

[breehall]

I'll reach out to them in the Slack thread and use this convo as a reference

[chandlerprall]

+1 to the suggestion for Kibana to update. We cannot predict what text / flow is going to be present in a popover, and solving for the maps set up will likely cascade a failure change to another location, etc. This amount of text in a popover already makes assumptions on screen size, etc and has usability concerns that, IMO, only the app can properly address.

[chandlerprall]

Not to contradict myself (but I am anyway), one thing we should probably consider is modifying popover to have a max constraint on both dimensions to ensure it fits in the screen.

[cee-chen]

@chandlerprall see below thread, our popover_positioning service is having issues correctly calculating the content when max()/min() CSS functions percentages are being used - normally our popover should be correctly modifying its dimensions to ensure it fits on the screen 🤔

[cee-chen]

[starting a new thread to not make the old one go a million comments long]

Aghh I hate to 180 on this so much but it looks like there actually is a very weird / buggy interaction with <EuiText grow={false}> used within an EuiPopover specifically.

To contrast, here's the before/after using a static container:

• https://eui.elastic.co/v58.1.1/#/display/text (the previous max-width/min-width 75% implementation)
• https://eui.elastic.co/v59.0.0/#/display/text (using the new max(64ch, 75%) change)

The widths of the first example in both links are exactly the same (using 75% correctly).

So I went into src-docs/src/views/popover/popover.tsx and modified that example with grow={false} and expanding the context so that it would overflow:

    <EuiPopover
      button={button}
      isOpen={isPopoverOpen}
      closePopover={closePopover}
    >
      <EuiText grow={false}>
        <p>
          Popover content that&rsquo;s wider than the default widthPopover
          content that&rsquo;s wider than the default widthPopover content
          that&rsquo;s wider than the default widthPopover content that&rsquo;s
          wider than the default widthPopover content that&rsquo;s wider than
          the default width
        </p>
      </EuiText>
    </EuiPopover>

And here are the results with max():

And here are the results with max-width: 64ch; min-width: 75%:

So... basically, it is a bug, but I think it's a bug specifically with EuiPopover and CSS min/max functions.

I'm relatively certain it's EuiPopover's popover_positioning service, which sets the height and width of the popover based on the content, not correctly detecting the content's true width. My best guess is that min-width: 75% is essentially 0 because the popover panel is 0 width before it finishes rendering in, therefore 75% is being ignored totally and only 64ch is being used. Using max(), 75% is not ignored, but EuiPopover is bizarrely calculating it at 100% screen width.

[cee-chen]

I think there are a couple approaches/fixes to this issue:

• Fix/enhance the popover_positioning service to accommodate min()/max() functions percentage widths (see below comment)
• Add a .euiPopover__panel .euiText override for this CSS to set it back to its old min-width/max-width CSS
• Advise against using EuiText grow={false} within an EuiPopover, and tell consumers to prefer using their own static widths/max-widths (which would my suggestion for Maps in the interim)

[cee-chen]

After a little more testing, it's actually not min/max that's causing the bug, it's percentages:

    <EuiPopover
      button={button}
      isOpen={isPopoverOpen}
      closePopover={closePopover}
    >
      <div style={{ width: '75%' }}>
        Popover content that&rsquo;s wider than the default widthPopover content
        that&rsquo;s wider than the default widthPopover content that&rsquo;s
        wider than the default widthPopover content that&rsquo;s wider than the
        default widthPopover content that&rsquo;s wider than the default width
      </div>
    </EuiPopover>

max(64ch, 75vw) for example works just fine. So basically popover_positioning needs to be updated to correctly account for % widths, and calculate them as if they're a % of window.innerWidth.

[cee-chen]

FWIW, this doesn't fix Maps' use case, as they likely still do not want a 75% wide popover, so again they should be advised to stop using EuiText grow={false} and instead something like EuiText style={{ maxWidth: '64ch' }}.

[chandlerprall]

To recap:

1. need an investigation + bugfix for popover with percentage-width children
2. recommend Maps team modifies their use of EuiText

Sound right?

[cee-chen]

Exactly!

[thompsongl]

So basically popover_positioning needs to be updated to correctly account for % widths, and calculate them as if they're a % of window.innerWidth

Need to remember to account for the container config also: https://elastic.github.io/eui/#/layout/popover#constraining-a-popover-inside-a-container

[cee-chen]

After poking a little more into the guts of popover_positioning and EuiPopover, this is semi-interesting shenanigans as the positioning service doesn't actually ever set width on anything - it's deriving its calculations purely from the existing width of the panel content, so basically CSS itself is doing a confused psyduck once you set a % width on content popovers.

So, as Chandler mentioned in the above thread, we may need to start modifying EuiPopover to manually set width/height styles where we're currently not... but that may also cause other unexpected shenanigans with popover down the road. Fun times!