Problem/Motivation

This ability to link to a vertical tab was introduced in #376293: Activate current vertical tab from URL fragment and has been merged. You currently can use the case-sensitive vertical tab id as the hash, but without knowing how this works users cannot link to specific tabs.

For example if I wanted to link a user to authoring information I would need to know that in Drupal 8 I would need to add #edit-author to the end of the URL. Ideally when authoring information is clicked the window hash should be updated to reflect the tab that is open.

Proposed resolution

When clicking the vertical tab, the URL in the browser should be updated with the relevant hash of the tab. If this URL is entered in to a new browser said tab should be open by default and brought in to the users view.

Before clicking on the vertical tab link switches the tab pane, but does not change the hash in the URL bar:

After clicking on the vertical tab link (including mouse and keydown) switches the tab pane AND changes the hash in the URL bar:

After pasting this URL with the preserved hash into a new window highlights the relevant tab and scrolls down to it:

Remaining tasks

Backport to 8.2.x (Should be the same patch)
Backport to 7.x potentially based on patch #2 which was created for 7.38

User interface changes

Minimal changes. A page with a hashed URL will be scrolled down, and focused on the relevant vertical tab. If the relevant tab is not the default open tab, the default will be closed and the tab from the hash will be opened.

API changes

N/A

Data model changes

N/A

CommentFileSizeAuthor
#122 2752511-nr-bot.txt151 bytesneeds-review-queue-bot
#121 2752511-121-9.5.x.patch4.48 KBsaidatom
#120 2752511-120-9.5.x.patch4.49 KBsaidatom
#119 2752511-119-9.5.x.patch4.88 KBsingularo
#118 2752511-110-9.5.x.patch5.39 KBclaudiu.cristea
#117 2752511-109-9.5.x.patch5.34 KBclaudiu.cristea
#116 interdiff_115-116.txt2.62 KBranjith_kumar_k_u
#116 2752511-116.patch10.66 KBranjith_kumar_k_u
#115 2752511-115.patch10.46 KBravi.shankar
#112 Twitter-settings-example.jpg62.66 KBAaronMcHale
#109 reroll_diff_92-109.txt6.53 KBmlncn
#109 vertical-tabs-2752511-109.patch10.55 KBmlncn
#95 vertical-tabs-2752511-92-test-only.patch973 bytespfrenssen
#94 vertical-tabs-2752511-94-D8.patch10.39 KBsaidatom
#93 vertical-tabs-2752511-93-D8.patch15.03 KBsaidatom
#92 vertical-tabs-2752511-92-D8.patch15.03 KBsaidatom
#92 vertical-tabs-2752511-92.patch10.49 KBsaidatom
#89 vertical-tabs-2752511-89.patch10.79 KBsaidatom
#88 vertical-tabs-2752511-88-D8.patch10.69 KBsaidatom
#88 vertical-tabs-2752511-88.patch10.81 KBsaidatom
#85 vertical-tabs-2752511-85.patch85.03 KBsaidatom
#85 vertical-tabs-2752511-85-D8.patch6.96 KBsaidatom
#83 vertical-tabs-2752511-83.patch1.99 KBsaidatom
#82 vertical-tabs-2752511-82-D8.patch1.96 KBsaidatom
#81 vertical-tabs-2752511-81-D8.patch2.4 KBsaidatom
#81 vertical-tabs-2752511-81.patch2.42 KBsaidatom
#80 vertical-tabs-2752511-80-D8.patch2.41 KBsaidatom
#80 vertical-tabs-2752511-80.patch2.43 KBsaidatom
#79 interdiff-77_79.txt921 bytesGauravvvv
#79 2752511-79.patch1.99 KBGauravvvv
#77 vertical-tabs-2752511-77.patch2.54 KBvalthebald
#76 vertical-tabs.patch2.5 KBsingularo
#63 interdiff-2752511-51-62.txt3.59 KBvalthebald
#63 2752511-62-testonly.patch959 bytesvalthebald
#62 2752511-62.patch3.01 KBvalthebald
#54 vertical_location_hash-2752511-54-test-only.patch959 bytesRumyanaRuseva
#53 vertical_location_hash-2752511-51.patch4.33 KBRumyanaRuseva
#50 2752511-50.drupal.vertical_location_hash.patch4.35 KBgrahamC
#44 2752511--before--hash-not-preserved.png192.78 KBElijah Lynn
#44 2752511--after-hash-preserved-in-URL-bar.png196.32 KBElijah Lynn
#44 2752511--after--copying-hashed-url-scrolls-and-opens-relevant-vertical-tab.png195.67 KBElijah Lynn
#42 drupal-vertical_tabs_update_hash-2752511-42-D7-do-not-test.patch1.9 KBrooby
#37 interdiff.txt1.06 KBnod_
#37 core-2752511-37.patch1.99 KBnod_
#36 2752511-36.patch1.94 KBstefan.r
#34 2752511-34.patch2.99 KBstefan.r
#33 2752511-33.patch2.98 KBstefan.r
#32 2752511-32.patch2.93 KBstefan.r
#30 2752511-30.patch2.91 KBstefan.r
#25 update_location_hash-2752511-25.patch1.09 KBLiamPower
#19 update_location_hash-2752511-18.patch809 bytesLiamPower
#15 update_location_hash-2752511-15.patch5.39 KBLiamPower
#12 update_location_hash-2752511-12.patch789 bytesLiamPower
#2 drupal-vertical-tabs-location-hash-update-2752511-2.patch801 bytesElijah Lynn
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Elijah Lynn created an issue. See original summary.

Elijah Lynn’s picture

Here is the patch off of 7.38. Please re-roll against head, sorry, really busy right now.

Elijah Lynn’s picture

Issue summary: View changes
Elijah Lynn’s picture

I am already thinking of the case where maybe there is an existing hash, this patch would overwrite that. Although I am thinking that would actually be okay.

Also there is the case where the hash changes and the user uses the backward/forward in their browser history. With this patch nothing will happen however the hash will change, yet the user will not navigate to the vertical tab that they would expect.

I am thinking we should use history.pushState() for this instead but forget what browser support we are on.

Elijah Lynn’s picture

Waaat, Drupal 7 says IE6+. https://www.drupal.org/node/61509

http://caniuse.com/#feat=history says the History API is not in IE8 but is in IE11. And http://stackoverflow.com/a/4968481/292408 says IE9 is not but IE10 is yes.

So there would need to be some modifications to check support for the History API and if so use it and if not use what I have in the patch with window.location.

Elijah Lynn’s picture

Ahh, caniuse wasn't showing me everything because I didn't click 'import country usage data'. Now it shows the IE9/10.

stefan.r’s picture

Version: 7.x-dev » 8.3.x-dev
Issue tags: +Dublin2016

This will probably need to be fixed in 8.x first

LiamPower’s picture

Looking at this as part of the Mentored sprints @ DC Dublin2016

AdamB’s picture

Looking at this as part of the Mentored Sprints at DrupalCon Dublin 2016 with LiamPower.

AdamB’s picture

To be able to replicate this issue: Go to Appearance, ensure that Seven is not being used as the Admin theme, so use Bartik. Once you have disabled & you try to add/edit a node you can see the vertical tabs at the bottom of the page.
Thanks to @jp.stacey for find this.

-- Update, this does not seem to be the correct area, have tried on Drupal 7 with the patch & Drupal 8 by recreating the patch.

@elijah-lynn, could you direct us to the area of the site that you were seeing this?

AdamB’s picture

We have now been able to re-create this issue & will be adding a patch shortly.

This can be recreated in the above mentioned section, when creating/editing a new node.

LiamPower’s picture

Here is the updated patch for Drupal 8 using the patch from #2 for Drupal 7 as a starting point.

Drupal 8 is using settings.details.attr('id') as the hash each tab is referenced by within misc/vertical-tabs.js

shires82’s picture

Reviewed in pair programming session with LiamPower at Drupalcon Dublin 2016 contribution sprint.

Status: Needs review » Needs work

The last submitted patch, 12: update_location_hash-2752511-12.patch, failed testing.

LiamPower’s picture

Patch was created with an incorrect path. I have re-rolled it to be from core/misc/vertical-tabs.js

LiamPower’s picture

Status: Needs work » Needs review
LiamPower’s picture

Status: Needs review » Needs work

The last submitted patch, 15: update_location_hash-2752511-15.patch, failed testing.

LiamPower’s picture

Status: Needs work » Needs review
FileSize
809 bytes

Uploaded the wrong patches so re-rolled again!

jp.stacey’s picture

The patch in #19 applies cleanly on 8.3.x and implements the following:

* As vertical tabs are clicked, the hash in the URL bar changes.
* If a hashed URL is copied and pasted into a new window, the relevant vertical tab is opened.

Reviewed and confirmed working well.

LiamPower’s picture

Issue summary: View changes

Updated the summary to reflect the issue better and make it less confusing

LiamPower’s picture

Issue summary: View changes

Updated the UI changes in the summary

stefan.r’s picture

Looking at this with @valthebald and @jp.stacey

valthebald’s picture

Looking at this with @stefan.r and @jp.stacey

LiamPower’s picture

I've updated the patch to store settings.details.attr('id') in a variable so it isn't re-called.

jp.stacey’s picture

Issue summary: View changes
valthebald’s picture

Patch from #25 applies cleanly to 8.3.x, has the same behavior as before

stefan.r’s picture

Status: Needs review » Reviewed & tested by the community

@jp.stacey just showed me how this patch behaves and this looks great.

We also have screenshots now, so let's see if @webchick can give this a final review.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

jp.stacey walked me through this patch. Great work!

However, my concern is that in usability testing, we see that when people initially arrive at the node add page, they click in all the vertical tabs, trying to figure out what they do. And because the URL changes several times in this process, if a user hits the "back" button it's going to appear to be broken. :( Because the only thing that's changing is subtly up in the URL, not the entire page display (like in Gmail, Twitter, etc. that use similar path hacks).

So I think we need to account for this in the patch somehow, so it remembers where "Back" should ultimately link to.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
2.91 KB

I think we'd want to use window.location.replace instead so the back button is not affected.

vertical-tabs.js only applies if you disable the admin theme for node add/edit pages (which gives you Bartik, as in the screenshots) so we'd want to do this for the <details> elements as well (from Seven).

stefan.r’s picture

stefan.r’s picture

stefan.r’s picture

stefan.r’s picture

nod_’s picture

Status: Needs review » Needs work

It is not needed for details elements since details don't reuse the same visual space to display their content.

On the code side I'd make a function with that bit of code and some more comments, it'll be easy to break if we copy paste it

+      var currentUrl = window.location.href.split('#')[0];
+      var yScroll = document.body.scrollTop;
+      // Update the URL bar with the hash.
+      window.location.replace(currentUrl + '#' + detailsId);
+      // Prevent the page from scrolling when the URL is replaced.
+      document.body.scrollTop = yScroll;

And removing of the node.js changes.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
1.94 KB
nod_’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.99 KB
1.06 KB

Couple of things. Now it's good for me.

The last submitted patch, 2: drupal-vertical-tabs-location-hash-update-2752511-2.patch, failed testing.

catch’s picture

+++ b/core/misc/vertical-tabs.js
@@ -220,6 +224,27 @@
+      // prevent the page from scrolling fter replacing the URL.

s/fter/after.

Otherwise looks fine, but do we have any vertical tabs js tests yet?

catch’s picture

Status: Reviewed & tested by the community » Needs review

#2729663: Fragment link pointing to <textarea> should be redirected to CKEditor instance when CKEditor replaced that textarea adds some test coverage that might be enough to go on to add similar coverage here. Marking CNR to see if we want to try that.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

rooby’s picture

Here is a new D7 patch based on #37.

jibran’s picture

@rooby you can create a new D7 issue for this according to the new policy.

Elijah Lynn’s picture

Attached missing images so they show in the summary. They somehow never made it past preview stage via discussion with @mixologic and @drumm in #drupalorg. Messaged jp.stacey and he still had them and sent them to me, thanks!

stefan.r’s picture

Needs tests per #40

dmsmidt’s picture

The following issue is related: #2531700: Fragment links to children elements in closed grouping elements don't work.
It makes it possible to target a child item inside a grouping element like vertical tabs or details, and then automatically show/open the tab/details.

dmsmidt’s picture

The needed tests would fit well in the new functional JS tests of the above mentioned issue.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

grahamC’s picture

Ported to ES6, and added a minimum viable test.

The part where it tries to prevent the page from scrolling was broken in current versions of Chrome, due to a change in how they handle document.body.scrollTop.

I've applied a workaround based loosely on the conversation at #2843240: IE11 & Chrome(PointerEvents enabled) & some Firefox scroll to the top of the page after dragging the bottom item with jquery 1.5 <-> 1.11.

Status: Needs review » Needs work

The last submitted patch, 50: 2752511-50.drupal.vertical_location_hash.patch, failed testing. View results

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

RumyanaRuseva’s picture

Status: Needs work » Needs review
FileSize
4.33 KB

The last submitted patch does not apply to the current 8.6 and 8.7. Rerolled.

RumyanaRuseva’s picture

Adding a test-only patch to see that the test fails without the js fixes.

Status: Needs review » Needs work

The last submitted patch, 54: vertical_location_hash-2752511-54-test-only.patch, failed testing. View results

RumyanaRuseva’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs tests

The test-only patch failed as expected, and the patch from @grahamC in #50 and #53 passed successfully.
The patch looks correct, tested manually and automatically, approved by previous comments as well. RTBC.

The last submitted patch, 50: 2752511-50.drupal.vertical_location_hash.patch, failed testing. View results

lauriii’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Usability

Would be great to get feedback from the UX team on this.

icf-vpathak’s picture

The patch from #53 does not work on Drupal 8.60

dmsmidt’s picture

Status: Needs review » Needs work

I like the idea of this issue.
However the scroll restore after the hash update seems overkill.

+++ b/core/misc/vertical-tabs.es6.js
@@ -274,6 +279,28 @@
+      // Get the URL before the hash.
+      const currentUrl = window.location.href.split('#')[0];
+      // Save the numbers of pixels that we are scrolled upwards so we can
+      // prevent the page from scrolling after replacing the URL.
+      const scrollNode = document.scrollingElement || document.documentElement || document.body;
+      const yScroll = scrollNode.scrollTop;
+      // Update the URL bar with the hash.
...
+      // Prevent the page from scrolling when the URL is replaced.
+      scrollNode.scrollTop = yScroll;

This can be simplified by just calling:
window.history.replaceState(null, null, `#${detailsId}`);
IMHO we can get rid of the helper function completely since is is just such a small piece of code.

For support see: https://caniuse.com/#feat=history

I think the tests also needs to make sure the clicked tab is still visible in the viewport (testing isVisible() is not enough).

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

valthebald’s picture

Status: Needs work » Needs review
FileSize
3.01 KB

I second with @dmsmidt that window.history.replaceState() is a simpler solution here.

valthebald’s picture

Attaching also test-only (this should fail) patch, and interdiff

Status: Needs review » Needs work

The last submitted patch, 63: 2752511-62-testonly.patch, failed testing. View results

valthebald’s picture

Status: Needs work » Needs review

Failure of patch in #63 is expected result

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

mlncn’s picture

We've applied the patch in 62 and are not seeing any change. Anything else we should be doing to make it work?

EDIT: Never mind, the Claro theme (at least the contrib version we're using) overrides this file.

Heads up that maybe this patch will have to be extended to the Claro theme too.

valthebald’s picture

Status: Needs review » Needs work

TODO: Make the patch Claro-compatible

mlncn’s picture

One other question— shouldn't we also be updating the history, so that the back button will take someone to the latest hash location they saw, and not clean to another page?

LiamPower’s picture

@mlncn it was decided at Drupalcon Dublin in 2017 that this would be a bad user experience. See #29 by @webchick

mlncn’s picture

Oy. Sorry i missed that.

And i respectfully disagree. People who are adding content (rather than exploring functionality, as it sounds like perhaps they were in the usability testing) are much less likely to want to leave an add or edit page. In any case, the back button leaving the edit form rather than going to the previous tab surprised our clients!

Even if it were roughly equal in people's expectations, it's far worse for someone to expect a back button to take them to the last tab, and for it to instead throw them out of editing entirely. Worst case scenario without updating history to have the hash is someone losing a ton of work because they wanted to change something on the second-to-last tab before saving. Worst case scenario with updating history to have the hash is someone has to press the back button a few more times to get what they expect.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

ilechcod’s picture

I just discovered this is still an issue in Drupal 8.9 core! I thought the patch was committed? I went over to vertical_tabs.js and the part that appends the hash to window.location was still missing. I had to add it manually (for now). Just wondering... Why is this still an issue?

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

singularo’s picture

Attaching patch i'm using for 9.1.x, theres some tiny changes that make the #62 one not apply.

valthebald’s picture

Status: Needs work » Needs review
FileSize
2.54 KB

Resubmitting the patch from #76 since it was missing '/core' in the file paths

Gauravvvv’s picture

Assigned: Unassigned » Gauravvvv
Gauravvvv’s picture

Re-rolled patch #77. Attached interdiff for same.
Please review.

saidatom’s picture

Missing yarn run build:js

The last submitted patch, 82: vertical-tabs-2752511-82-D8.patch, failed testing. View results

saidatom’s picture

When a user clicks a link having the URL fragment, the page will be loaded with the corresponding tab activated.

pfrenssen’s picture

Assigned: Gauravvvv » pfrenssen
Status: Needs review » Needs work
Issue tags: +Needs tests

It seems something went wrong in the last patch, it contains unrelated dependency updates. This also needs test coverage to prove that it works as expected and prevent future regressions.

pfrenssen’s picture

Issue tags: -Needs tests

I looked through the patch history and there was a test for this earlier on but it got lost. The last patch that was complete including test coverage is the one from comment #62.

Looking at the previous reviews, the remaining work to do to bring this to completion is:

  1. Start from patch #62.
  2. Integrate the changes from patches #76 - #85 to make this compatible with Drupal 9.3.
  3. Port the changes to core/themes/claro/js/vertical-tabs.es6.js because the Claro theme overrides this functionality.
saidatom’s picture

pfrenssen’s picture

Status: Needs review » Needs work

The patch is looking great! I just have 1 small remark:

  1. --- a/core/misc/vertical-tabs.es6.js
    +++ b/core/misc/vertical-tabs.es6.js
    -          const focusID = $this.find(':hidden.vertical-tabs__active-tab').val();
    +          let focusID = null;
    +          const windowLocationHash = window.location.hash;
    +          if (windowLocationHash) {
    +            focusID = windowLocationHash.replace('#', '');
    +          } else {
    +            focusID = $this.find(':hidden.vertical-tabs__active-tab').val();
    +          }
    

    In this change we are losing the const declaration for windowLocationHash. Since this declares the variable as immutable it prevents any following code from making accidental changes to this value. If possible we should try to keep it as a constant. Possibly something like this can be used instead?

    const focusID = windowLocationHash ? windowLocationHash.replace('#', '') : $this.find(':hidden.vertical-tabs__active-tab').val();

saidatom’s picture

saidatom’s picture

pfrenssen’s picture

Assigned: pfrenssen » Unassigned
Status: Needs work » Needs review
FileSize
973 bytes

Thanks! Looking good to me. I am just uploading only the test from #92 so we can check that the test correctly covers the functionality. If this fails this ticket is RTBC.

Status: Needs review » Needs work

The last submitted patch, 95: vertical-tabs-2752511-92-test-only.patch, failed testing. View results

idimopoulos’s picture

Status: Needs work » Reviewed & tested by the community
nod_’s picture

Happy with the patch, I would agree that replacing the state so that the back button navigates away from the form is a bad thing. Maybe the 5 years old decision in #29 might be revised based on different trade-offs we'd go with nowadays? Left a question in UX slack channel.

Patch is in #92 btw

nod_’s picture

One thing to note is that when using the back button we can change which vertical tab is shown (the current patch doesn't account for that) so it's not only a tiny thing in the URL that changes, it's also which vertical tab is shown.

AaronMcHale’s picture

Assigned: Unassigned » AaronMcHale

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 95: vertical-tabs-2752511-92-test-only.patch, failed testing. View results

mlncn’s picture

Status: Needs work » Reviewed & tested by the community

Still RTBC let's please get this in?

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 95: vertical-tabs-2752511-92-test-only.patch, failed testing. View results

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mlncn’s picture

This does need a re-roll now, #92 no longer applies.

mlncn’s picture

Assigned: AaronMcHale » Unassigned
mlncn’s picture

Issue tags: +Needs reroll
mlncn’s picture

Assigned: Unassigned » mlncn
mlncn’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
10.55 KB
6.53 KB

Patch re-rolled for 9.4.x, accounting for the removal of JQuery once from core ([#3158256]). This did not affect these changes themselves, just their ability to apply.

If this passes it's back to RTBC.

mlncn’s picture

Status: Reviewed & tested by the community » Needs review
AaronMcHale’s picture

We reviewed this issue at #3255717: Drupal Usability Meeting 2022-01-07.

The first thing I'd like to highlight is this article, which I referenced as I was presenting this issue during the UX meeting:

Tabbed Interfaces | inclusive-components.design

The article talks about - among other things - the distinction between "panels" and "views", this difference is explained more under the section titled "When panels are views". Coincidentally it also has an anchor: #whenpanelsareviews.

To be clear, going forward when I use the words "view" or "views", I'm not talking about Views in Drupal (the module), I'm referring to the terms described here.

In the context of these vertical tabs, these are definitely panels, they are a sub-set of the page/form that changes focus when you navigate between the tabs; Navigating between the vertical tabs also does not lose the form state, in other words if I enter text into a field on one tab, and then move to another tab, this does not impact my changes to the form.

Whereas, a tab that is a view, you would expect the form to be fully within that tab/view and for most of the page to be contained within it. Changing to a different view would behave in the same way as if I navigated to a whole new page. An example of this, which we showed during the UX call, the settings area of Twitter: each tab behaves like a new page, where each tab has a separate form with a separate save button, a unique URL, and it is clear that if I navigate to another tab I would lose any unsaved changes.

Twitter settings screen, showing the "Change display language" view.

The equivalent of this distinction in Drupal terms is the Local Tasks and Vertical Tabs, where the Local Tasks behave like changing the view in a single-page app, and the Vertical Tabs behave like moving between panels. A problem here is that Drupal's Admin UI is not a single-page application, and navigating to a different Local Task does reload the entire page causing any changes to be lost without any warning to the user (that is a problem in itself); While the vertical tabs do behave like tab panels in a single-page application, and navigating between them does not reload the page and does not cause any information to be lost.

So why does that all matter?

Because we have two sets of competing behaviour here, both Local Tasks and Vertical Tabs look similar, they both look like tabs, but they behave very differently. This is a problem in itself and so we have to be careful not to exacerbate that problem, we have to be very clear about the differences between the Local Tasks and the Vertical Tabs.

One of the concerns shared during the UX call was that it's not great that you can lose your changes by navigating to a new Local Task, and so we should not make that problem worse by blurring the distinction between the Local Tasks and Vertical Tabs; Which is potentially what we are doing by updating the URL and make it so that the browser's back and forward buttons allow navigating through Vertical Tabs. By updating the URL in the browser's address bar when the user navigates between Vertical Tabs, we're further blurring the distinction between the behaviour of the Vertical Tabs and Local Tasks, for the average user whether there's a hash symbol in that URL or not doesn't matter, all they see if the URL changing.

For example, if I make some changes on a form, navigate through some of the vertical tabs, then hit the back button, currently I would expect to go back to the previous page and to lose any changes, in part because Vertical Tabs look like panels within the page, they don't look like nor do they behave like views (or separate pages); However with this change if I press the back button and all it does is change the vertical tab that is active, then that is introducing inconsistent/conflicting behaviour, which is not good from a UX perspective. It could go the other way, where if I get used to expecting that after navigating through some vertical tabs, that pressing the back button will take me to the previous tab, but I press the back button one too many times, and it takes me to the previous page, all of a sudden, I have unexpectedly lost all of my changes.

Another concern that was raised is how this would behave if there is more than one group of Vertical Tabs on any given page, we felt it was unclear what was supposed to happen with the anchor URL, there is potential for conflict between the multiple groups. The example above then becomes even more confusing, where pressing the back button might take me back to a tab in a different group, this could be a jarring experience.

The best way I could think to summarise this is as follows: Vertical tabs are intended to only be used in the context of one part of a form, and it's possible for multiple vertical tab groups to exist on one page, that means they are self-contained and their context is limited. Updating the URL, on the other hand, has a global context, the URL impacts the entire page, therefor it does not make sense for something which has a self-contained limited context to update the URL. In other words, the URL impacts the whole page, vertical tabs only impact a sub-set of the page and are designed to be self-contained.

AaronMcHale’s picture

FileSize
62.66 KB

(Forgot to upload the image for the previous comment)

AaronMcHale’s picture

nod_’s picture

Version: 9.4.x-dev » 10.0.x-dev
Status: Needs review » Needs work

Thanks for the detailed explanation, very clear :)

Patch needs reroll.

Please make a 10.x patch first as it is now the default branch to get things in :)

ravi.shankar’s picture

Added reroll of patch #109 on Drupal 10.0.x.

ranjith_kumar_k_u’s picture

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
5.34 KB

A 9.5.x version of #109 to whom it may concern.

claudiu.cristea’s picture

singularo’s picture

Slightly tweaked patch attached to work with 9.5.0 as the most recent doesn't apply.

saidatom’s picture

saidatom’s picture

needs-review-queue-bot’s picture

Status: Needs review » Needs work
FileSize
151 bytes

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Version: 10.0.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.