Scroll to the right position after switching Behavior/Content tabs

Promoting for better editing UX.

Yeah 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.

I 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?

Did 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:

• The first Paragraph who starts below the sticky tabs (and starts above the viewport end)
• Or if missing, the first Paragraph in that field

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.

Ah, 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.

Yes, 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.

+++ b/js/paragraphs.admin.js
@@ -89,11 +115,45 @@
+            if ($(this).parent().attr('id') === 'behavior') {

Why 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.

Did 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

You can stop the loop from within the (each) callback function by returning false.

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.

We also need tests. :-)

More clean-ups and some todo's

We are still missing the tabs stickyness part in this issue.

Improved the code again to make it work with nested Paragraphs.
This even reduced code complexity again - wohoo!

Now tests for real.

Starting to work on tests. Had some problems with setting the height of the browser. Will continue investigating tomorrow.

Tested 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.

Removing some css that is not needed, fixing the problem described in the screenshot of the previous comment.

Now the sticky position of the tabs work also on collapsed secondary toolbar level.

But when it is collapsed, the scrolling position is not preserved.

+++ b/js/paragraphs.admin.js
@@ -92,8 +140,45 @@
+          // If the tabs are sticky (offset positions are the same),
+          // find the first paragraph and attach a class to it.

I extended the calculation without that condition and added another early exit point to maintain top level elements better.

Oops, those are the right ones.

Looks like the tests need work, that's pretty empty still.

Can 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?

Oh 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.. :-)

Couldn'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).

Well 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.

Lets hope this doesn't cause any random fails or so :) Committed.