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
Comment | File | Size | Author |
---|---|---|---|
#122 | 2752511-nr-bot.txt | 151 bytes | needs-review-queue-bot |
#121 | 2752511-121-9.5.x.patch | 4.48 KB | saidatom |
#120 | 2752511-120-9.5.x.patch | 4.49 KB | saidatom |
#119 | 2752511-119-9.5.x.patch | 4.88 KB | singularo |
#118 | 2752511-110-9.5.x.patch | 5.39 KB | claudiu.cristea |
Comments
Comment #2
Elijah LynnHere is the patch off of 7.38. Please re-roll against head, sorry, really busy right now.
Comment #3
Elijah LynnComment #4
Elijah LynnI 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.
Comment #5
Elijah LynnWaaat, 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.
Comment #6
Elijah LynnAhh, caniuse wasn't showing me everything because I didn't click 'import country usage data'. Now it shows the IE9/10.
Comment #7
stefan.r CreditAttribution: stefan.r commentedThis will probably need to be fixed in 8.x first
Comment #8
LiamPower CreditAttribution: LiamPower at Reading Room commentedLooking at this as part of the Mentored sprints @ DC Dublin2016
Comment #9
AdamB CreditAttribution: AdamB at Reading Room commentedLooking at this as part of the Mentored Sprints at DrupalCon Dublin 2016 with LiamPower.
Comment #10
AdamB CreditAttribution: AdamB at Reading Room commentedTo 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?
Comment #11
AdamB CreditAttribution: AdamB at Reading Room commentedWe 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.
Comment #12
LiamPower CreditAttribution: LiamPower at Reading Room commentedHere 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
Comment #13
shires82 CreditAttribution: shires82 commentedReviewed in pair programming session with LiamPower at Drupalcon Dublin 2016 contribution sprint.
Comment #15
LiamPower CreditAttribution: LiamPower at Reading Room commentedPatch was created with an incorrect path. I have re-rolled it to be from core/misc/vertical-tabs.js
Comment #16
LiamPower CreditAttribution: LiamPower at Reading Room commentedComment #17
LiamPower CreditAttribution: LiamPower at Reading Room commentedComment #19
LiamPower CreditAttribution: LiamPower at Reading Room commentedUploaded the wrong patches so re-rolled again!
Comment #20
jp.stacey CreditAttribution: jp.stacey at Magnetic Phield commentedThe 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.
Comment #21
LiamPower CreditAttribution: LiamPower at Reading Room commentedUpdated the summary to reflect the issue better and make it less confusing
Comment #22
LiamPower CreditAttribution: LiamPower at Reading Room commentedUpdated the UI changes in the summary
Comment #23
stefan.r CreditAttribution: stefan.r commentedLooking at this with @valthebald and @jp.stacey
Comment #24
valthebaldLooking at this with @stefan.r and @jp.stacey
Comment #25
LiamPower CreditAttribution: LiamPower at Reading Room commentedI've updated the patch to store settings.details.attr('id') in a variable so it isn't re-called.
Comment #26
jp.stacey CreditAttribution: jp.stacey at Magnetic Phield commentedComment #27
valthebaldPatch from #25 applies cleanly to 8.3.x, has the same behavior as before
Comment #28
stefan.r CreditAttribution: stefan.r commented@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.
Comment #29
webchickjp.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.
Comment #30
stefan.r CreditAttribution: stefan.r commentedI 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).Comment #31
stefan.r CreditAttribution: stefan.r commentedComment #32
stefan.r CreditAttribution: stefan.r commentedComment #33
stefan.r CreditAttribution: stefan.r commentedComment #34
stefan.r CreditAttribution: stefan.r commentedComment #35
nod_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
And removing of the node.js changes.
Comment #36
stefan.r CreditAttribution: stefan.r commentedComment #37
nod_Couple of things. Now it's good for me.
Comment #39
catchs/fter/after.
Otherwise looks fine, but do we have any vertical tabs js tests yet?
Comment #40
catch#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.
Comment #42
rooby CreditAttribution: rooby commentedHere is a new D7 patch based on #37.
Comment #43
jibran@rooby you can create a new D7 issue for this according to the new policy.
Comment #44
Elijah LynnAttached 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!
Comment #45
stefan.r CreditAttribution: stefan.r commentedNeeds tests per #40
Comment #46
dmsmidtThe 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.
Comment #47
dmsmidtThe needed tests would fit well in the new functional JS tests of the above mentioned issue.
Comment #50
grahamCPorted 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.
Comment #53
RumyanaRuseva CreditAttribution: RumyanaRuseva at FFW commentedThe last submitted patch does not apply to the current 8.6 and 8.7. Rerolled.
Comment #54
RumyanaRuseva CreditAttribution: RumyanaRuseva at FFW commentedAdding a test-only patch to see that the test fails without the js fixes.
Comment #56
RumyanaRuseva CreditAttribution: RumyanaRuseva at FFW commentedThe 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.
Comment #58
lauriiiWould be great to get feedback from the UX team on this.
Comment #59
icf-vpathak CreditAttribution: icf-vpathak commentedThe patch from #53 does not work on Drupal 8.60
Comment #60
dmsmidtI like the idea of this issue.
However the scroll restore after the hash update seems overkill.
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).
Comment #62
valthebaldI second with @dmsmidt that window.history.replaceState() is a simpler solution here.
Comment #63
valthebaldAttaching also test-only (this should fail) patch, and interdiff
Comment #65
valthebaldFailure of patch in #63 is expected result
Comment #67
mlncn CreditAttribution: mlncn as a volunteer and at Agaric for Drutopia, Find It Cambridge, Cambridge, Massachusetts Family Policy Council commentedWe'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.
Comment #68
valthebaldTODO: Make the patch Claro-compatible
Comment #69
mlncn CreditAttribution: mlncn as a volunteer and at Agaric for Drutopia, Find It Cambridge, Cambridge, Massachusetts Family Policy Council commentedOne 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?
Comment #70
LiamPower CreditAttribution: LiamPower commented@mlncn it was decided at Drupalcon Dublin in 2017 that this would be a bad user experience. See #29 by @webchick
Comment #71
mlncn CreditAttribution: mlncn as a volunteer and at Agaric for Drutopia, Find It Cambridge, Cambridge, Massachusetts Family Policy Council commentedOy. 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.
Comment #74
ilechcod CreditAttribution: ilechcod commentedI 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?
Comment #76
singularoAttaching patch i'm using for 9.1.x, theres some tiny changes that make the #62 one not apply.
Comment #77
valthebaldResubmitting the patch from #76 since it was missing '/core' in the file paths
Comment #78
Gauravvvv CreditAttribution: Gauravvvv at OpenSense Labs commentedComment #79
Gauravvvv CreditAttribution: Gauravvvv at OpenSense Labs commentedRe-rolled patch #77. Attached interdiff for same.
Please review.
Comment #80
saidatomComment #81
saidatomComment #82
saidatomComment #83
saidatomMissing yarn run build:js
Comment #85
saidatomWhen a user clicks a link having the URL fragment, the page will be loaded with the corresponding tab activated.
Comment #86
pfrenssenIt 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.
Comment #87
pfrenssenI 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:
Comment #88
saidatomAdd Claro overrides and re-add testing from patches #62
Comment #89
saidatomComment #90
saidatomComment #91
pfrenssenThe patch is looking great! I just have 1 small remark:
In this change we are losing the
const
declaration forwindowLocationHash
. 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();
Comment #92
saidatomFix
const
remarkComment #93
saidatomComment #94
saidatomFix patch Drupal 8
Comment #95
pfrenssenThanks! 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.
Comment #97
idimopoulos CreditAttribution: idimopoulos for European Commission and European Union Institutions, Agencies and Bodies commentedAs @pfrenssen said in https://www.drupal.org/project/drupal/issues/2752511#comment-14182096, this is now in RTBC.
Comment #98
nod_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
Comment #99
nod_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.
Comment #100
AaronMcHaleComment #102
mlncn CreditAttribution: mlncn as a volunteer and at Agaric for Drutopia, Find It Cambridge, Cambridge, Massachusetts Family Policy Council commentedStill RTBC let's please get this in?
Comment #105
mlncn CreditAttribution: mlncn as a volunteer and at Agaric for Drutopia, Find It Cambridge, Cambridge, Massachusetts Family Policy Council commentedThis does need a re-roll now, #92 no longer applies.
Comment #106
mlncn CreditAttribution: mlncn as a volunteer and at Agaric for Drutopia, Find It Cambridge, Cambridge, Massachusetts Family Policy Council commentedComment #107
mlncn CreditAttribution: mlncn as a volunteer and at Agaric for Drutopia, Find It Cambridge, Cambridge, Massachusetts Family Policy Council commentedComment #108
mlncn CreditAttribution: mlncn as a volunteer and at Agaric for Drutopia, Find It Cambridge, Cambridge, Massachusetts Family Policy Council commentedComment #109
mlncn CreditAttribution: mlncn as a volunteer and at Agaric for Drutopia, Find It Cambridge, Cambridge, Massachusetts Family Policy Council commentedPatch 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.
Comment #110
mlncn CreditAttribution: mlncn as a volunteer and at Agaric for Drutopia, Find It Cambridge, Cambridge, Massachusetts Family Policy Council commentedComment #111
AaronMcHaleWe 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.
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.
Comment #112
AaronMcHale(Forgot to upload the image for the previous comment)
Comment #113
AaronMcHaleComment #114
nod_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 :)
Comment #115
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedAdded reroll of patch #109 on Drupal 10.0.x.
Comment #116
ranjith_kumar_k_u CreditAttribution: ranjith_kumar_k_u at Zyxware Technologies commentedComment #117
claudiu.cristeaA 9.5.x version of #109 to whom it may concern.
Comment #118
claudiu.cristeaD9.5 reroll
Comment #119
singularoSlightly tweaked patch attached to work with 9.5.0 as the most recent doesn't apply.
Comment #120
saidatomD9.5 reroll
Comment #121
saidatomComment #122
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe 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.