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

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.