Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
When switching between the Behavior and Content tabs, the last active paragraphs is not persisted and not visible on screen.
Proposed resolution
As a followup of #2901995, I suggest we fix the window position while switching between Behavior and Content tabs for Paragraphs.
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#36 | paragraphs_3025693_scroll_36-interdiff.txt | 3.53 KB | Berdir |
#36 | paragraphs_3025693_scroll_36.patch | 12.37 KB | Berdir |
#35 | paragraphs_3025693_scroll_35-interdiff.txt | 7.37 KB | Berdir |
#35 | paragraphs_3025693_scroll_35.patch | 12.58 KB | Berdir |
#35 | paragraphs_3025693_scroll_35-test-only.patch | 5.94 KB | Berdir |
Comments
Comment #2
miro_dietikerPromoting for better editing UX.
Comment #3
sasanikolic CreditAttribution: sasanikolic commentedMy idea was to add a class to the first paragraph in the viewport and scroll to that when switching tabs. Does this make sense? It basically works, but not sure about the user experience. This attached patch does that.
Comment #4
sasanikolic CreditAttribution: sasanikolic commentedComment #5
miro_dietikerYeah we need such a reference. Two additional inputs though:
Sometimes i‘m interacting with the Paragraph that starts inside the Viewport. The first would then be the previous one and scroll too much up. I like to see the whole Paragraph in the viewport if possible.
Maybe we could check if there is an active paragraph, also by considering the form element in focus. If such an element is present, we should use its Paragraph as a reference. That would solve the first problem as well for many cases.
We still need a fallback if no element is in focus. Don‘t know which case happens how often - this needs testing.
Comment #6
sasanikolic CreditAttribution: sasanikolic commentedDid some refactoring and improvements for the scrolling position. The issues were caused by the sticky positioned elements, such as admin bar and sticky tabs.
Regarding the form element in focus, we don't have such thing yet, that's why I opened this issue the other day #3036530, where we would add the focused paragraph indicator. I think that would just add complexity to this issue.
Please test the new patch and see if that works better / as expected.
Comment #7
miro_dietikerI understood that the other issue was about visually indicating the active element. Otherwise please extend its description to provide an API.
It's clear we need an API and know what is active, but i'm unsure about the visual indication of such an active Paragraph.
I wouldn't expect that this adds so much complexity here, except the one-off-problem is very well solved.?
See document.activeElement
https://developer.mozilla.org/en-US/docs/Web/API/DocumentOrShadowRoot/ac...
We can easily check if one of the form elements has focus and then determine its parent Paragraph widget as active.
Do you think it adds that much complexity?
Comment #8
sasanikolic CreditAttribution: sasanikolic commentedFixed some scrolling issue in this patch.
Tried getting the active element on mouseup event, so whenever the user would click on a paragraph, we would get the current element and persist the position based on it, but the issue is with ckeditor iframe and the Edit paragraph button, as the event does not happen there. Adding also a patch with a function to test that.
Comment #9
miro_dietikerDid some research about what has a minimal complexity and still is of biggest value for our users.
So to be clear, we don't want to maintain workarounds like mouse-up events and other crazy event handlers.
Our approach will have some limitations, but it is the job of Core to indicate the active (form, widget) element and we will build on top of that once it is available.
Core Layout builder tries to identify the active element when opening actions in dialogs, see #2994909: Highlight active element while working with dialogs in Layout Builder
We only want to scroll if a Paragraphs field is inside the Viewport and the behavior tabs are present and sticky.
Thus we should start with fixing these tabs while scrolling into a Paragraph field.
Then while the sticky tabs are present, we can easily determine a highlighted reference Paragraph when clicking one of the tabs:
That Paragraph will be marked as highlighted with a class.
The highlight class will apply the usual hover blue #f7fcff - maybe a bit stronger?
And then the class is immediately removed again, resulting in a CSS phase-out transition of the color highlight.
The user has then a visible reference in the Viewport.
A first version is fine if it only acts on most top level.
However we might prefer to apply this logic to the most nested Paragraph as a reference point.
Comment #10
miro_dietikerAh, almost forgot... The scrollto action after tab switching scrolls the highlighted reference paragraph to the same vertical screen offset as it was before! Don't put it to the viewport top.
Comment #11
sasanikolic CreditAttribution: sasanikolic commentedThis patch includes the fixes mentioned above.
Note that this change for the background animation affects also the paragraphs hover state.
What do you mean by the same vertical screen offset as it was before? Currently it scrolls to the top of the first-level (parent) paragraph.
Comment #12
BerdirYes, and instead of scrolling to the top of that, it should consider the height difference that the paragraph had to the top so that it remains at basically the same position.
I'm also not quite sure how this makes without sticky content/behaviors, so we either need to include that or do that first in a separate issue.
Comment #13
sasanikolic CreditAttribution: sasanikolic commentedI added a lot of comments and fixes for the scroll position. I'm saving the current position on the content tab, so this should work now.
Comment #14
sasanikolic CreditAttribution: sasanikolic commentedSorry for the spam, here i fixed the empty lines at the end of files.
Comment #15
miro_dietikerWhy would we only want to determine the position in one case?
You can scroll up / down on each of the two tabs. On each switch, we should determine the reference of the current situation before switching.
Comment #16
sasanikolic CreditAttribution: sasanikolic commentedThis maintains the scroll positions for both tabs whenever the first paragraph is the same. Once we scroll to another paragraph, when switching tabs the page will scroll to the top of the new paragraph. Does this make more sense?
Also improved some comments.
Comment #17
miro_dietikerDid some more testing here and there is some more work to do.
Much confused about the use of $allParagraphs.filter(function () {
And then not returning anything at all... I guess you intended .each?
See also
Let's avoid global variables like setScrollPosition. Instead markFirstVisibleParagraph can return true or even the element in case of success.
...
Here is a reimplementation of the calculation that also will need many less special cases...
Still needs more testing and maybe polishing.
We should not animate scrollto as the element visually appears persistent (stable vertical offset) with its scroll position. An animation adds confusion.
Comment #18
miro_dietikerWe also need tests. :-)
Comment #19
miro_dietikerMore clean-ups and some todo's
Comment #20
miro_dietikerWe are still missing the tabs stickyness part in this issue.
Comment #21
miro_dietikerImproved the code again to make it work with nested Paragraphs.
This even reduced code complexity again - wohoo!
Now tests for real.
Comment #22
johnchqueStarting to work on tests. Had some problems with setting the height of the browser. Will continue investigating tomorrow.
Comment #23
miro_dietikerTested and the tabs stickyness works for me.
However the tests above contain multiple special cases of the sticky tabs positioning, varying toolbar cases and also a special case in the library edit dialog. This all would need to be tested with the various additional toolbar modules (Admin Toolbra, ..?) as well...
And i even found a bug on desktop when the secondary toolbar is collapsed:
So it seems we can only let this in as an opt-in (widget setting to enable sticky tabs) to avoid breaking UX for existing users. Then we can limit the supported cases explicitly and caring about more with proper test coverage (and thus document them) in follow-ups.
Comment #24
johnchqueRemoving some css that is not needed, fixing the problem described in the screenshot of the previous comment.
Comment #25
miro_dietikerNow the sticky position of the tabs work also on collapsed secondary toolbar level.
But when it is collapsed, the scrolling position is not preserved.
I extended the calculation without that condition and added another early exit point to maintain top level elements better.
Comment #26
miro_dietikerOops, those are the right ones.
Comment #27
BerdirLooks like the tests need work, that's pretty empty still.
Comment #28
sasanikolic CreditAttribution: sasanikolic commentedAdded the tests for switching the tabs and a default top position fix. I noticed in the test that the .toolbar-fixed was missing, therefore the paragraph tabs were not sticky.
Comment #30
miro_dietikerCan you please explain / comment in tests about what you're testing?
I can not see that the test is scrolling to any specific position and yet it is expecting that the first item is not visible?
Comment #31
sasanikolic CreditAttribution: sasanikolic commentedI added some comments to the test. What I tried to test here is the scroll persistency, as I couldn't find any command to scroll to a specific element. I'd like to add that though. Do you know if some command for scrolling exists?
And yes, it is expecting that the first item is not visible, because when we add all those paragraphs, the viewpoint on that window size looks like in this screenshot of the content/behavior tab viewports attached.
Not sure why it fails, as the test passes locally.
Comment #33
miro_dietikerOh i thought we committed this long ago.
We're using this in production and i never had any issue with the position adjustment here, so it seems quite solid.
Re-reading test coverage in #31 and it looks fine now.
It only needs to pass.. :-)
Comment #34
sasanikolic CreditAttribution: sasanikolic commentedI cleaned up the patch, I think there was an unrelated change in there for the ParagraphsExperimentalAddModesTest.
Comment #35
BerdirCouldn't reproduce the test fail either but it also pretty pretty fickle with so many empty paragraphs.
I've changed it to have a string_long text field in there which I think is already a much better representation of how a real form looks. Limited the amount of paragraphs we add and tried to get an even baseline by moving to a defined position (the second of 4 paragraphs).
Also rewrote the initial part of the test to use our test helper methods to create node type and paragraph field, saves a ton of code and is considerably faster.
Lets see if testbot likes this more.
(Also removed the yarn lock file and fixed the unrelated field_group changes).
Comment #36
BerdirWell that didn't help.
Did a lot of debugging, first thought it might be related to the test software overlay headless or not, but not sure. After updating to latest chromedriver version I did get a (different value), so lets try this approach to see if that helps.
Also removed an unused variable in JS.
Comment #38
BerdirLets hope this doesn't cause any random fails or so :) Committed.