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

Selects in Modals do not escape Dialog #2235

Closed
connorjsmith opened this issue Sep 10, 2019 · 19 comments
Closed

Selects in Modals do not escape Dialog #2235

connorjsmith opened this issue Sep 10, 2019 · 19 comments
Assignees

Comments

@connorjsmith
Copy link
Contributor

What are the steps to reproduce the issue?
Put a Select element inside a Dialog which cannot contain the full menu's height

Example:
image

What behavior did you expect?
Menu should exceed bounds of Dialog component

image

In what environment did you see the issue?

  • OS & Device: Windows 10
  • Browser: Edge Canary
  • Version 78.0.273.0

Is there any additional context?
Seems like if we set position: absolute, width: inherit on the listbox for the menu it behaves as expected.

@triage-new-issues triage-new-issues bot added the status:triage New Issue - needs triage label Sep 10, 2019
@chrisdholt chrisdholt added the bug A bug label Sep 10, 2019
@triage-new-issues triage-new-issues bot removed the status:triage New Issue - needs triage label Sep 10, 2019
@scomea
Copy link
Collaborator

scomea commented Sep 11, 2019

I was able to get this to work by setting overflow = visible on some of the select's parent containers in the modal:

image

The suggested fix of setting postion=absolute and width=inherit only works with fixed width selects and not when the select has width set to a percentage, for example. This may be a case where the correct approach is to override the styles as the containers also need to scroll so they can't apply my fix of setting the overflow.

@connorjsmith
Copy link
Contributor Author

A classic select can escape its container and seems relatively fundamental to the control type. Should we instead be using position absolute, and manually calculate width and position using Javascript?

@chrisdholt
Copy link
Member

@connorjsmith since we're referencing the native select and the ability to break out of the chrome / container by default, I'm going to ping @gregwhitworth for a bit of awareness as he's been gathering quite a bit of info on native selects. Native select does escape containers and by default also escapes the chrome as well. I'm not sure if that's always the correct UX though for all dropdowns. Just because breaking out of the chrome and a container is the current implementation, I'm not sure that it's actually fundamental to the control or pattern itself. I can imagine a few possible scenarios where you wouldn't want a select to escape a modal. I do think this is a worthwhile conversation though and if we can improve the control by supporting this behavior I think we should. It is a common request and it does seem like a valuable addition. Ultimately, I'm just hesitant to refer to it as "fundamental" because the native control does this.

@scomea I think we should do our due diligence to consider how we can enable this kind of support, even if it means doing so in a breaking change.

@connorjsmith I wonder if there is a more performant way to manage the width outside JS...the perf costs could get really high depending on the amount of items you have.

Can we keep this thread going to work towards a vNext API on select?

@chrisdholt chrisdholt added improvement A non-feature-adding improvement and removed bug A bug labels Sep 12, 2019
@chrisdholt
Copy link
Member

Changing to a feature request and tracking for vNext

@connorjsmith
Copy link
Contributor Author

Sounds good, thanks for the replies @chrisdholt @scomea ! I agree maybe the word "fundamental" is a bit too strong here. That being said, I do believe most people expect selects to act like context menus in that they are in their own "global" layer, unaffected by the layout of the page around them (apart from matching the trigger's width). Of course it'd be difficult (read: pretty well impossible) for an HTML element to escape the browser window, but otherwise I'd like to be able to use the fast Select as a drop-in replacement for a system select element without have to worry about caveats or other limitations.

In the mean time, I believe we can dictate a fixed widths on our Selects (will have to double check all of our affected instances can do this) and can then use my suggested solution.

Coming back to the perf impact, do you expect setting a style on menu open (the only time a menu width can change, as far as I'm aware) to be overly expensive for a common base control? It should be independent of the number of items, since the listbox's width is only determined by the size of the trigger button. Is there something I'm missing here which would require us to update the width of the modal while it is open?

Another limitation that I could see is in the semantics of position: absolute. We'd only be able to escape elements until the first statically positioned element. This is doesn't quite give us a true "global" layer, but position: fixed is a bit trickier and would see the listbox detach from the trigger unless we update position on every scroll event (expensive)

I'm happy to take on driving this change (my first commit to fast-dna 🎉), after discussing with @scomea to determine what is best/expected of consumers of fast

@connorjsmith
Copy link
Contributor Author

Seems like Fabric UI uses position: fixed, but dismisses the dropdown whenever you scroll the page

Ex:
https://developer.microsoft.com/en-us/fabric#/controls/web/dropdown

This could be a viable strategy, might be worth talking to them about their design considerations too

@connorjsmith
Copy link
Contributor Author

connorjsmith commented Sep 12, 2019

Synced up with @scomea and I think my project is able to specify widths to all of our selects used inside Dialogs. These allows us to use my initial mitigation via jssStyleSheet.

There is still an issue for selects which cannot have fixed widths while also being Dialogs, but this is a relatively niche scenario which doesn't meet the bar for a "bug" in my mind.

Bigger discussion needs to be had around the javascript positioning method mentioned above. It would be easy to write code which would lay it out to always match the width of the select trigger, but consumers may want a drop down which is larger or smaller than the trigger. We will likely have to use some sort of callback which a consumer can use to position/layout their dialog, but the proper design for that will take some thought.

All in all, thanks for the help everyone :)

@stale
Copy link

stale bot commented Dec 11, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the warning:stale No recent activity within a reasonable amount of time label Dec 11, 2019
@robpaveza
Copy link

robpaveza commented Jul 10, 2020

I ran into this not with the above description, but similar:

image

Here, flex is used frequently:

<div style={{display: 'flex', flexFlow: 'column nowrap'}}> <!-- vertical layout -->
  <div display: 'flex', flexDirection: 'row nowrap', justifyContent: 'center'> <!-- radio button list container -->
    <Radio>None</Radio>
    <Radio>form-data</Radio>
    <Radio>x-www-form-urlencoded</Radio>
    <Radio>Raw text</Radio>
    <HideUnless ...> <!-- renders as a <div> and is shown in this case -->
      <Select ... />
    </HideUnless>
  </div>
</div>

Here, the <Select /> renders into this...

<div class="select-0-1-19">
  <button id="selecttrigger-1" class="select_button-0-1-20" aria-haspopup="listbox" aria-labelledby="selecttrigger-1" aria-expanded="true"><span class="select_buttonContentRegion-0-1-21"><div class="select_buttonDisplayText-0-1-22">Plain text (text/plain)</div><svg width="16" height="16" viewBox="0 0 16 12" class="select_toggleGlyph-0-1-23" aria-hidden="true" xmlns="http://www.w3.org/2000/svg"><path d="M11.3613 2.73633L11.8887 3.26367L6 9.15234L0.111328 3.26367L0.638672 2.73633L6 8.09766L11.3613 2.73633Z"></path></svg></span></button>
  <select style="display: none;"></select>
  <div tabindex="-1" role="listbox" aria-activedescendant="" class="select_menu-0-1-24">
    <div title="Plain text (text/plain)" tabindex="-1" class="selectOption-0-1-97 selectOption__selected-0-1-101" role="option" id="text/plain" aria-selected="true" aria-disabled="false"><span class="selectOption_contentRegion-0-1-98">Plain text (text/plain)</span></div>
    <div title="JSON (application/json)" tabindex="-1" class="selectOption-0-1-97" role="option" id="application/json" aria-selected="false" aria-disabled="false"><span class="selectOption_contentRegion-0-1-98">JSON (application/json)</span></div>
    <div title="XML (text/xml)" tabindex="-1" class="selectOption-0-1-97" role="option" id="text/xml" aria-selected="false" aria-disabled="false"><span class="selectOption_contentRegion-0-1-98">XML (text/xml)</span></div><div title="XML (application/xml)" tabindex="-1" class="selectOption-0-1-97" role="option" id="application/xml" aria-selected="false" aria-disabled="false"><span class="selectOption_contentRegion-0-1-98">XML (application/xml)</span></div>
    <div title="HTML (text/html)" tabindex="-1" class="selectOption-0-1-97" role="option" id="text/html" aria-selected="false" aria-disabled="false"><span class="selectOption_contentRegion-0-1-98">HTML (text/html)</span></div>
    <div title="JavaScript (application/javascript)" tabindex="-1" class="selectOption-0-1-97" role="option" id="application/javascript" aria-selected="false" aria-disabled="false"><span class="selectOption_contentRegion-0-1-98">JavaScript (application/javascript)</span></div>
  </div>
</div>

If I modify the element matching .select_menu-0-1-24 here and apply the styles position: relative; z-index: 100; then it appears fine. But I'd prefer not to need to do that. Also, the only obvious way to target that item via CSS is to apply a CSS class name to my <Select>, and then target it indirectly, e.g., .my-select-css-name > div[role=listbox] { ... } which feels fragile.

@stale stale bot removed the warning:stale No recent activity within a reasonable amount of time label Jul 10, 2020
@gregwhitworth
Copy link

@robpaveza is this within an iframe or is one of the ancestors set to overflow: hidden?

@robpaveza
Copy link

@gregwhitworth Likely one or more ancestors are set to overflow:hidden.

@chrisdholt
Copy link
Member

@robpaveza, is the expectation that this break out of the dialog as the normal select would?

@robpaveza
Copy link

@chrisdholt It's my expectation that a popup for a <select> will escape other UI.

@chrisdholt
Copy link
Member

@gregwhitworth I think we talked about what @robpaveza talked about previously, but not formally. Part of this is the expectation set by the platform and a seemingly special sauce where the select listbox overlays UI and breaks out of containers. I can't recall - have we discussed this in Open UI at all? We don't really have a good hook to ensure that in every app select overlays all other DOM elements, such as the case here. The platform seemingly has this internally, it would be great to be able to leverage it. If there isn't an issue tracking this, I'd be happy to open one :).

@gregwhitworth
Copy link

@chrisdholt we're actively working on an explainer for this. This isn't an Open UI issue but a CSSWG one. The explainer's are being iterated on it will introduce a new stacking context. Still things to flush out - but yes this is being worked on. When the explainer goes public I'll post it here for feedback.

@chrisdholt
Copy link
Member

@gregwhitworth, thanks for the context! @robpaveza given the existing constraints we're a bit blocked by the above - but once that makes its way through we will take advantage of it 😄 .

@gregwhitworth
Copy link

@melanierichards

@scomea
Copy link
Collaborator

scomea commented Jul 15, 2020

I'm working on the Web Component "anchored-region" (ie. the theoretical container for menu flyouts) and I think we can use fixed positioning when we need it to open in the foreground. It wouldn't easily track the element it's anchored to after initial placement, but if we're ok with menus that close if the user does something to change the outside layout (ie. scrolls, resizes) I think it could work.

@EisenbergEffect EisenbergEffect added area:react and removed improvement A non-feature-adding improvement labels Jul 20, 2020
@chrisdholt
Copy link
Member

closed with #3517 - will link to this in a select issue for WC's

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants