storage: Scroll dialog fields into view as needed #20060
Conversation
There was a problem hiding this comment.
I don't think any of this is needed, and could probably be replaced with 1 line of JavaScript in a render event or perhaps a line and a setTimeout() on a change event.
Rely on the CSS selector to conditionally select the element, use optional chaining (in case it doesn't match anything — which it won't in some cases), and then the scrollIntoView() to scroll (which is a no-operation if not necessary).
Details inline in the part that it would replace... but I think we can probably drop everything else in the PR too using this method, as we wouldn't need to check its state or position or anything like that.
WDYT?
pkg/storaged/dialog.jsx
Outdated
| function scroll() { | ||
| const dialog_element = document.querySelector('#dialog .pf-v5-c-modal-box__body'); | ||
| const field_element = document.querySelector('#dialog [data-tag="' + tag + '"]'); | ||
|
|
||
| if (field_element) { | ||
| if (dialog_element) { | ||
| // check if we need to scroll | ||
| if (field_element.getBoundingClientRect().bottom <= | ||
| dialog_element.getBoundingClientRect().bottom) | ||
| return; | ||
| } | ||
|
|
||
| field_element.scrollIntoView({ behavior: "smooth", block: "end" }); | ||
| } | ||
| } | ||
| // By the time show_field is called from the "update" | ||
| // callback, newly visible fields don't exist yet in the | ||
| // DOM, so delay the scrolling a bit. | ||
| window.setTimeout(scroll, 10); |
There was a problem hiding this comment.
Why do we need to check if we need to scroll? Just ask for it to be in the view. If it's already in the view, then it's a NOOP (no operation).
It should also be block as start (which is default when passing options), not end, as we want people to be able to read the error and everything after. If scoll-behavior is set in CSS, we wouldn't need to specify an animation. (I'm not sure if that's set by default though. It's something to consider, probably. Then we could have it conditionally set across all of Cockpit provided reduce animations isn't turned on in as an OS pref. But this is outside the scope of this PR, likely.)
window.setTimeout(() => {
document.querySelector(`#dialog .pf-v5-c-form__group:has([data-tag="${tag}"])`)?.scrollIntoView({behavior: "smooth"});
}, 10);But that data-tag is probably not even needed? You can probably select it on the change event of the element and as long as it has a value, then scroll to top of all the encryption group? You can even do all that logic in the CSS selector like this:
// Scroll to encryption area into view when encryption details are required.
document.querySelector('[data-field="crypto"]:not([data-value="none"])')?.scrollIntoView({behavior: "smooth"});The ?. makes it not scrollIntoView when there are no matches (which would happen when none is selected in the selector). If there's a render call after changing the element, that's the place to put this line... and you probably wouldn't even need the setTimeout(), checking a scroll state, or any separate function either.
There was a problem hiding this comment.
Why do we need to check if we need to scroll?
As far as I can tell, scrollIntoView will always scroll the element to the requested position, even if it is already fully visible. The scrollIntoViewIfNeeded method would do what we want, but it is not widely enough available.
It should also be block as start (which is default when passing options), not end,
Ok.
There was a problem hiding this comment.
From the scrollIntoViewIfNeeded page:
This method is a proprietary variation of the standard Element.scrollIntoView() method.
We should not use a proprietary version of something that became standard.
If the scroll only happens when it can and when someone changes the dropdown to a value that isn't nothing, then it's very limited to scrolling to content. If we use block: nearest, then it won't scroll at all unless needed (which duplicates the proprietary behavior, but in a standards way).
We'd use this: {block: 'nearest', behavior: 'smooth'}
nearest details: It scrolls to show the content only when needed. When all the content is on screen, it doesn't scroll at all. When the content is larger than the screen, it also doesn't scroll. When the top or bottom is not shown and there's room, it scrolls to view it (unless it doesn't need to).
(I just double-checked nearest in a codepen example from https://matthiasott.com/notes/scrolling-elements-into-view and tried modifying it under various conditions.)
There was a problem hiding this comment.
What about always centering the field that the user is interacting with? Like this: https://www.youtube.com/watch?v=S4YdDXljuX8
When fields fail validation, the first of them is scrolled into view at the top of the dialog. Additionally, when the encryption fields of the Format dialog are revealed, they are scrolled into view. https://bugzilla.redhat.com/show_bug.cgi?id=2264540
mvollmer
force-pushed
the
storage-scroll-dialog-fields
branch
from
6c8e3a2
to
5d97d4e
Compare
| return; | ||
| } | ||
|
|
||
| field_element.scrollIntoView({ behavior: "smooth", block: align || "start" }); |
There was a problem hiding this comment.
This added line is not executed by any test. Details
When fields fail validation, the first of them is scrolled into view. Additionally, when the encryption fields of the Format dialog are revealed, they are scrolled into view.
https://bugzilla.redhat.com/show_bug.cgi?id=2264540
Demo: https://www.youtube.com/watch?v=POn3lDoPJOc