[Popover] absolute positioning

[chrismcv]

Hi,
One thing that came up in our testing, was that when Popover contains a TextField or input, and when using Mobile Safari, (e.g. ipad) and the keyboard comes up, the fixed positioning causes the popover to lose its position, and become unusable. Making it absolute should sort this, the downside is that render-to-layer needs to track its position manually via a throttled WindowListener.

Comments welcome.

Thanks,
Chris

Fixes #3482.

[newoga]

@chrismcv Any chance we could refactor Popover to not use scroll position? It might take a lot of refactoring but it will super simplify a lot of this.

[chrismcv]

I can't think of a way to do this. Happy to invest time into it if you can!

[newoga]

@chrismcv Awesome, honestly I haven't really investigated it much.

I'm wondering if we can learn anything about how to do it using approaches used by these:
https://github.com/HubSpot/tether
https://github.com/souporserious/react-tether

Would you be able to do a preliminary check and see how feasible it would be?

Edit: Not saying to integrate/use those, just see if there's another approach we could implement.

[chrismcv]

@newoga - I'm pretty sure tether is doing the same thing
https://github.com/HubSpot/tether/blob/master/src/js/tether.js#L86
https://github.com/HubSpot/tether/blob/master/src/js/tether.js#L352

[chrismcv]

@newoga any more thoughts on this? (We're currently using a branch on my own repo with this in production, because iPad compatibility is important for us... I'd like to get back on the main tree.)

[newoga]

@newoga any more thoughts on this?

@chrismcv Sorry for the late response! I did have some thoughts but didn't have the time to formulate a response at the time. You're right the library are using listeners, but they're not (always) using fixed positioning based on scroll position. I think we should look into switching the popovers to be absolutely positioned relative to what it's anchored to. That should reduce repaints and fix the "thrashing" that's happen when the user scrolls now.

What do you think?

[chrismcv]

@newoga no problem.

I think we should look into switching the popovers to be absolutely positioned relative to what it's anchored to.

I'm not sure how to do this. Popover is using the portal pattern, so is rendered into a different subtree. This is useful/important/necessary as it allows it to break outside of overflow:hidden in the anchor's tree. Any thoughts/guidance would be great, as perhaps I'm missing something obvious.

[chrismcv]

@newoga just a friendly ping on this for your best thoughts!

[newoga]

@chrismcv Thanks for the reminder! I see your point about the portal pattern. I haven't given this too much thought, but I still wonder if we should just make Popover always the root/parent component when its behavior is used so that it doesn't have to worry about the overflow: hidden problem.

@callemall/material-ui Are there any other approaches that we're not considering? I think we should make a goal to not continuously update the Popover's position using inline styles based on scroll position (we can still use scroll/window to determine whether it should pop above/below/left/right on its anchor if necessary).

[chrismcv]

@newoga - I'm not sure how practical that is - Dialog is also a portal, which can (optionally) sit inside LeftNav (#1669) , and a SelectField (which uses Popover) can sit inside a Dialog, so I'm not sure we can reduce to root/parent that cleanly?

As for other ideas, https://github.com/souporserious/react-measure could be an alternative position detector, but probably won't reduce the layout thrashing much?

What about a container that matches the page dimensions? (With the popover fixed within that page?) At the moment this._layer is 100% by 100%, but if it was "measured width" by "measured height" then we'd only need to listen to onresize...?

[newoga]

@chrismcv Sorry, I'm not disagreeing there aren't valid use cases for the portal pattern. I agree that Dialog is a good use case. But I find that using it for Popover introduces a lot of unnecessary complexity. For example, right now on the material docs website, the version dropdown SelectField's Popover doesn't scroll because the window isn't scrolling, an inner div is.

[newoga]

@chrismcv Have you seen this project that react-bootstrap separated and made? It seems like they are able to use the portal pattern without relying on combination of fixed position and scroll state:

https://github.com/react-bootstrap/react-overlays

[nathanmarks]

@newoga Maybe we should use some modules from that instead of re-inventing the wheel. We can contribute back upstream with improvements if needed.

[newoga]

@nathanmarks I agree. I reviewed it in some more detail and it seems well implemented.

@chrismcv @callemall/material-ui Any thoughts on maybe porting our components to react-overlays? It would likely replace our internal/Overlay.js, internal/AutoLockScrolling.js, and internal/RenderToLayer.js.

[chrismcv]

I don't object to using it... however, I'm pretty sure we want to portal popover in the majority of circumstances, including SelectField e.g. #1621. I think we should agree on an API as well....

  <SelectField
     PopoverWrapper={PortalisingOverlayTrigger}
     {...props} />

[nathanmarks]

@chrismcv What @newoga is saying is that the portal'd content doesn't have to mean fixed positioning. You can see that in react-bootstrap their popovers aren't spastic when you scroll because they use absolute positioning for them in a portal.

[chrismcv]

yeah - I understand, but I don't see how this would work....

So if I have a SelectField in a Dialog, I want the DropDown part of the Select to break outside the Dialog, but my dialog body can scroll, so I need to portal out of it. Their solution requires a ref to be passed into to the portal, so to use a SelectField, I'd then need to pass it a dropdown container ref.

This seems like a substantial complexity to put this on users? Or am I missing something obvious?

[nathanmarks]

@chrismcv the idea is that we use the modules internally, not expose the API to users

[chrismcv]

@nathanmarks - I understand, but if we do that, I think we'll end up with a "lowest common denominator" implementation, which will result in being back on position fixed based solutions...

[nathanmarks]

@chrismcv Why is there "lowest common denominator" implementation? The Popover is only used for 1 thing in the entire MD spec, menus.

[chrismcv]

If we want to use <SelectField /> (which uses popover) in left nav like in the docs, then it needs to break outside <LeftNav /> as it does by default when components menu is collapsed. As LeftNav has overflow:scroll set on it, SelectField needs the dropdown part to portal to a different part of the DOM without the overflow, as a menu shouldn't be affected by it.

The problem here is the interaction between SelectField and LeftNav. If we were to absolutely position Popover for SelectField relative to the SelectField, LeftNav will still interfere with it, because of the overflow. (The same applies for dialog.)

[nathanmarks]

@chrismcv

LeftNav would not block it overflowing if it isn't in left nav.

BTW, not suggesting dialog should use absolute positioning.

[nathanmarks]

@chrismcv Sec, I see what you're saying about the nav bar edge case.

[nathanmarks]

@chrismcv Also, with the position: fixed solution we currently have, we're polling for the position under all conditions! With a 100ms throttle for perf reasons... therefore artificially limiting our popover scroll motion to 10 FPS. It looks absolutely terrible when you scroll the page with a popover open.

[chrismcv]

The same applies in dialog, it used to be that if we had select fields in
dialogs, we got scrollbars when opening them, which was non intuitive.

On Mon 4 Apr 2016 19:28 Nathan [email protected] wrote:

@chrismcv https://github.com/chrismcv Sec, I see what you're saying
about the nav bar edge case.

—
You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#3144 (comment)

[nathanmarks]

@chrismcv Some libraries opt to lock scrolling with menus open (including angular-material and polymer). This is also what the native <select> field does.

[chrismcv]

Native select triggers a close popup on a scroll for me? (on ubuntu) That would be straightforward to implement...

[nathanmarks]

@chrismcv aha, locks scroll on OS X.

I need to stew on this -- either way I don't think we can stick with a solution that means allowing scroll but capping movement at 10fps.

[newoga]

@chrismcv I'm going to close this for now. It has gotten really old since we removed all uses of isMounted (#3437) and the mixins such as WindowListenable (#3305). We can track the discussion regarding using scroll position for the position in #3917.

If you want to give this another stab in, feel free!

[chrismcv]

Fine to close, but I'm really keen for there to be a plan for a solution to this. At this stage, I don't see much point in investing time without a clearly agreed plan.

[nathanmarks]

@chrismcv yeah, there's a bunch of issues with dialogs/popovers/etc that need addressing together.

I'd really like to get the scrolling situation for popovers etc fixed. Right now the 100ms delay between updates is absolutely killing the feel.