Firestore Emulator UI doesn't have enough padding to edit items on small screens

thesandlord commented 3 years ago

If you are trying to edit an item in the UI on a smaller screen and the item is at the bottom, there is not enough space on the popup to click save.

https://user-images.githubusercontent.com/8902396/103427237-7966c580-4b74-11eb-9c83-8527f118d062.mp4

yuchenshi commented 3 years ago

Thanks for the bug and video! I've filed this internally as b/176910745. Help welcomed!

thesandlord commented 3 years ago

Ah not sure if that comment was for me, I'm actually no longer at Google. Need to update my profile :)

Hacky fix for anyone running into this issue, you can open up the inspector and change the style attribute from top: 0 to top: -300px or another negative value as appropriate.

I'll see if I can get a PR sent when I get some spare time, not sure what the best approach here would be. I haven't looked at the code, but might have to do a loop in javascript to make sure the element is visible, and if not bump up the top style attribute

cristianberroeta commented 3 years ago

Hi,

I've noticed that in InlineEditor.tsx, we are using a FocusOn component around the InlineEditor card, which is this card where you can edit a field:

The FocusOn component comes from this library: https://github.com/theKashey/react-focus-on

As they say in the docs, these are some of the things it does:

locks focus inside
disables page scroll and user interactions
hides rest of a page from screen-readers

I did some testing and was able to replicate the issue, but I also realized that, for some reason, the issue doesn't happen when the field list item that you click on is only partially visible, as shown in this video:

https://user-images.githubusercontent.com/17519897/131076283-89c8a51f-d92b-479d-91fc-e7c232faba72.mov

One possible reason for this would be a bug in the react-focus-on library, but I haven't checked if that is the case.

It would be great if the root cause can be found, but we can still fix our issue by other means. For instance, by allowing the scrolling. The FocusOn component has a property called "scrollLock". If set to false, it doesn't lock the scrolling, as displayed in this video:

https://user-images.githubusercontent.com/17519897/131076704-6a893df8-8898-4d87-a4e0-12d5b9a5531d.mov

Do you think this would be a good solution?

cristianberroeta commented 3 years ago

I was trying to figure out the root cause of the issue and I guess that it has to do with the way in which HTMLElement.focus() work (at least in Chrome and Safari, where I've tested).

I created this codepen https://codepen.io/cristian_berroeta/pen/powjzWZ to experiment, where you can see that when you call HTMLElement.focus() on an element, "the browser scrolls the document to bring the newly-focused element into view.", as they explain here: https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/focus.

However, as you can see in the following video, the position where the element ends up varies, depending on how visible it is when you call focus() on it: https://user-images.githubusercontent.com/17519897/131181908-f0a855ff-4fc0-43da-abee-0c4748334ee8.mov

The test in that video was run in Chrome, but I also ran it in Safari and it behaves exactly the same as far as I could tell.

So, apparently, the rule of the focus() function is:

if the element on which you call focus() is totally visible, then no automatic scrolling is run
if it is partially visible, then some automatic scrolling happens, but just enough to leave the element totally visible
if it is totally invisible, then the "full" automatic scrolling happens, which tries to leave the element in a centered position (if you note, in the experiment of the video, it always ends up at the same position, where the list item with number 29 is at the bottom of the screen).

What I suspect is that the FocusOn component probably runs the focus() function, maybe on the first focusable element it finds inside the wrapped component?? In the case of the first video in my first comment (https://github.com/firebase/firebase-tools-ui/issues/467#issuecomment-906940092), that would be the type selector element. When you click the edit button and it is only partially visible, that type selector is not visible at all (because it is some pixels below the edit button), and that causes the "full automatic scrolling" behavior of the focus() function to run. On the other hand, when you click the edit button and it is (approximately) totally visible, either no automatic scrolling or just a bit of automatic scrolling happens, which in some situations causes the "Save" and "Cancel" button to not be visible.

That's just a conjecture. We would have to review the react-focus-on library to check if that's true. I wonder if the FocusOn component has some props to tell it that the wrapped element should be completely visible. That would be useful.

yuchenshi commented 3 years ago

Thanks for looking into this! I think your idea to disable scrollLock is a great workaround while we look into the root cause. Let me see if I can get #636 merged.

yuchenshi commented 3 years ago

Fix released as of Firebase CLI v9.17.0.

firebase / firebase-tools-ui

Firestore Emulator UI doesn't have enough padding to edit items on small screens #467