Problem/Motivation

One of the most awesome benefits of adding vertical tabs in Drupal 7 was the javascript that allowed people to see what the current values were without having to pop-open the fieldsets. This benefit has been lost by moving the tabs to the right hand column.

I did a little research into the development of this new display of vertical tabs, and it looks like the current values are supposed to be present - at least according to the screenshots shown in this issue - the one that got committed.

Missing summaries

Perhaps they were forgotten, or removed later for some other reason?

Proposed resolution

Put back the awesomeness that was vertical tabs - javascript revealing current values from within fieldsets. In Drupal 7.38 it's handled in /misc/collapse.js, line 56-101, propably doesn't work one-by-one to D8.

Remaining tasks

see above

User interface changes

see above

API changes

none

#1936700: Book module usability improvements

Files: 
CommentFileSizeAuthor
#54 core-js-details-summary-1936708-54.patch4.85 KBespurnes
#52 core-js-details-summary-1936708-52.patch4.43 KBespurnes
#48 Tested in Microsoft edge.jpg40.01 KBjaspreetsingh31
#48 Tested in FireFox browser.jpg49.22 KBjaspreetsingh31
#48 Tested in Chrome browser.jpg51.78 KBjaspreetsingh31
#47 firefox.png68.04 KBbendev
#47 safari.png62.45 KBbendev
#47 chrome.png52.62 KBbendev
#46 interdiff.txt3.26 KBnod_
#46 core-js-details-summary-1936708-46.patch4.6 KBnod_
#44 current_element_values-1936708-44.patch2.18 KBbendev
#41 Firefox browser_screenshot.jpg52.48 KBjaspreetsingh31
#41 Doublearrow_bug_ChromeBrowser.jpg57.79 KBjaspreetsingh31
#40 Capture d’écran 2015-10-07 à 21.16.12.png49.1 KBbendev
#40 current_element_values-1936708-40.patch1.75 KBbendev
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 117,313 pass(es). View
#33 d8-vertical-tab-summaries-1936708-33.patch982 bytesespurnes
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,498 pass(es). View
#31 html-patch-14-and-23.jpg59.76 KBespurnes
#31 ui-patch-14-and-23.jpg45.4 KBespurnes
#27 chrome.png49.9 KBespurnes
#27 firefox.png67.45 KBespurnes
#23 screenshot-2015-09-26-14.42.08.png36.65 KBespurnes
#23 d8-vertical-tab-summaries-1936708-23.patch471 bytesespurnes
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,215 pass(es). View
#19 d8-vertical-tab-summaries-1936708-19.patch500 bytesespurnes
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,993 pass(es). View
#19 screenshot-2015-09-26-10.36.54.png34 KBespurnes
#14 d8-vertical-tab-summaries-1936708-5.patch2.64 KBKnee-X
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,978 pass(es). View
#10 Screen-Shot2015-09-25.png57.71 KBsokru
#4 d8-vertical-tab-summaries-1936708-4.patch2.55 KBjustinchev
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch d8-vertical-tab-summaries-1936708-4.patch. Unable to apply patch. See the log in the details link for more information. View
#4 vertical-tab-summaries.png32.61 KBjustinchev

Comments

jenlampton’s picture

Issue summary:View changes

typo

yoroy’s picture

Issue summary:View changes

fixed img tag: srtc=" -> src="

yoroy’s picture

Issue summary:View changes

asd

yoroy’s picture

Not sure why that doesn't work. Here's the link to the image:

http://drupal.org/files/Screen%20Shot%202012-10-13%20at%2012.45.57%20AM.png

yoroy’s picture

Issue summary:View changes

still trying to fix img tag

rootwork’s picture

Issue summary:View changes

This appears to still be the case in 8.0.x dev, and seems like a real shame. What was apparently intended, as Jen points out and as is shown in the image, would make a lot of sense, and not having that information definitely seems like a usability regression.

The span meant for this information, with the class "summary," is empty. For instance, here's the code for the Menu Settings tab:

<details id="edit-menu" class="menu-link-form form-wrapper collapse-processed">
  <summary aria-expanded="" aria-controls="edit-menu" role="button">
    <a class="details-title" href="#edit-menu">
      <span class="details-summary-prefix visually-hidden">Show</span>
      Menu settings
    </a>
  <span class="summary"></span>
  </summary>
<div class="details-wrapper">
  [...]
</div>
</details>

The span with class "summary" is also being hidden with CSS:

.entity-meta details .summary {
  display: none;
}

The CSS comes from the last definition in entity-meta.css (in core/themes/seven/css/components) and has the comment "Hide JS summaries. @todo Rethink summaries." That was originally written in the patch from #1838114-23: Change node form’s vertical tabs into details elements (subsequently worked on and committed in #1838114-81: Change node form’s vertical tabs into details elements). It's not clear to me reading that issue why the summaries weren't working, but presumably the fact that they're empty is why they're being hidden.

If someone else is more familiar with that history, maybe they could suggest what/how we should rethink those summaries, because it'd be great to get them working again.

yoroy’s picture

Category:Task» Bug report

Thanks for the investigations. Seems like a bug at this point as it would be a usability regression to not bring summaries back.

justinchev’s picture

StatusFileSize
new32.61 KB
new2.55 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch d8-vertical-tab-summaries-1936708-4.patch. Unable to apply patch. See the log in the details link for more information. View

I've been taking a look at this and trying to see if I can find a working example. I've gone as far back as D8 Alpha 2 and it's not in there.

Seems that these threads are relevant:
https://www.drupal.org/node/1838114#comment-6952220 - See patch on comment #6 as it seems to have some reference to it.
https://www.drupal.org/node/1751312 - Talks about re-writing the JS behaviours relevant to this

Anyway having delved into the current beta1 code I see that there is what looks like references to creating summary info within this file, but I couldn't decipher exactly how it was supposed to work (bit over my head),:
/core/modules/node/node.js

Anyway I've added a bit of jQuery (see patch) which provides this basic functionality and could potentially be used as a basis. It's rather messy at the moment with inline CSS, and no translation options etc, but I didn't want to spend too much time at this stage without getting some input on whether this is a viable/good way to approach this (I suspect this could be done in a better way).

PS I see that this is logged under Component 'Seven Theme' although all I found Seven Theme was doing is creating the Summary bit that appears at the top of the Tabs where the Published, last saved, author and revision info is showing)

LewisNyman’s picture

Issue tags:+frontend, +JavaScript
LewisNyman’s picture

Component:Seven theme» javascript

This is not a Seven issue

nod_’s picture

Issue tags:+Novice

Not really novice, but let's see if someone is up for something a little challenging.

nod_’s picture

Status:Active» Needs work

Also patch in #4 needs quite a bit of work.

justinchev’s picture

Assigned:Unassigned» justinchev

Yeah that patch needs a lot of work, but if you think achieving this with JS is fine I'll have a tinker.

sokru’s picture

Issue summary:View changes
StatusFileSize
new57.71 KB
sokru’s picture

Issue summary:View changes
sokru’s picture

Issue summary:View changes
Knee-X’s picture

We try to apply the patch (#4) with git apply but it display an error.

error: while searching for:

  Drupal.behaviors.nodeDetailsSummaries = {
    attach: function (context) {
      var $context = $(context);
      $context.find('.node-form-revision-information').drupalSetSummary(function (context) {
        var $context = $(context);
Knee-X’s picture

StatusFileSize
new2.64 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,978 pass(es). View

Patch corrected with good indentations.

Also adds the Drupal T function. Ready to be improve.

Tks anansi
Mentored by Chipway

made @ DrupalConEur Barcelona

rootwork’s picture

Status:Needs work» Needs review

The last submitted patch, 4: d8-vertical-tab-summaries-1936708-4.patch, failed testing.

The last submitted patch, 4: d8-vertical-tab-summaries-1936708-4.patch, failed testing.

bendev’s picture

Status:Needs review» Needs work

tested patch in #14 . I can see the content between brackets but it is not updated.
Although I changed the url alias, it stays "No alias"

espurnes’s picture

Assigned:justinchev» espurnes
Status:Needs work» Needs review
StatusFileSize
new34 KB
new500 bytes
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,993 pass(es). View

After some time looking at the functionality I realised that it is working as expected. The problem is that there is a css property hidding the summary tag. No need to apply any patch to /core/modules/node/node.js

The css property is in line 57 of /core/themes/seven/css/components/entity-meta.css

.entity-meta details .summary {
display: none; /* Hide JS summaries. @todo Rethink summaries. */
}

Proposed solution
Remove lines 57 to 59 of entity-meta.css file and style the summary tag using style guide documentation.

Remaining tasks
Define style for summary tag. There is no style information about this kind of element (https://groups.drupal.org/node/283223#Details_and_Accordion). Currently it's shown in uppercase and black.

espurnes’s picture

There two a discussions started 3 years ago related to this issue:
- https://www.drupal.org/node/1919956
- https://www.drupal.org/node/1838114#comment-6774686

No idea about the motivations to add display:none to summaries.

espurnes’s picture

Issue tags:+Barcelona2015

Added Barcelona2015 tag to the issue.

bendev’s picture

Status:Needs review» Needs work

I did the same check this morning in the plane (back from drupalcon barcelona)

I started from D7 and I I came to the same conclusion.
For those interested, the code is in the js of each module (corresponding to the tabs):

E.g : core/module/path/path.js
Drupal.behaviors.pathDetailsSummaries =
the same for pathauto etc...

By the way I tested your patch and it is ok.
I agree the style should likely be like on the first screenshot in the issue description...
todo @css

espurnes’s picture

Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new471 bytes
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,215 pass(es). View
new36.65 KB

I checked the style of the summary element in D7:

  • It is not uppercase
  • It is in #666 color
  • It is in font-size: .846em

So maybe we can apply the same style in D8 in the entity-meta.css

.entity-meta details .summary {
  color: #666;
  text-transform: none;
  font-size: 0.85em;
}

I think it's better to not force to show the summary value in uppercase.

bendev’s picture

Status:Needs review» Reviewed & tested by the community

#23 tested.
this looks ok.

Thanks

bendev’s picture

Assigned:espurnes» Unassigned
bendev’s picture

Status:Reviewed & tested by the community» Needs work

It looks like it is working in firefox but neither in chrome nor in safari.
Could anyone else confirm?

espurnes’s picture

StatusFileSize
new67.45 KB
new49.9 KB

I confirm the patch is working in firefox but not in chrome nor safari.

The next code is applied in firefox but not in the other browsers:

<a class="details-title" href="#edit-menu">
<span class="details-summary-prefix visually-hidden">Hide</span>
Menu settings
</a>
<span class="summary"> (Not in menu)</span>

Anyone can check why the html is different?

bendev’s picture

Issue summary:View changes
Issue tags:+browser compatibility

the code is in collapse.js locate in core/misc/ directory

sokru’s picture

Only webkit supports HTML5 <details> element and I dont know why following code (in collapse.js) is there in first place, but that's causing summaries not to be visible in Chrome.

125       if (Modernizr.details) {
126         return;
127       }
rootwork’s picture

#23 doesn't include the changes from #14. We need a patch that integrates that.

In terms of the font size, per #2045473: Improve visibility of Seven's smallest font elements the smallest we should make text is 1em. I agree on unsetting the all-caps text transform, but perhaps the parent item's text transform could be more specifically set so that we don't have one rule applying it and another rule removing it.

If you're curious about the source of this regression, see #2.

espurnes’s picture

StatusFileSize
new45.4 KB
new59.76 KB

applying #23 and #14 not solves the problem.

#14 adds additional markup and #23 just works in firefox... so it needs more work.

espurnes’s picture

I commented code from /core/misc/collapse.js and the summary is visible in firefox (40.0.3), Chrome (45.0.2454). and Safari (6.2.8).

125       if (Modernizr.details) {
126         return;
127       }

So we have to look deeper in this file and change the condition that prints the output.
Or maybe change the output to use different element than details or summary. Maybe a span or div?

Test with patch #23 applied.

Thanks sokru.

espurnes’s picture

StatusFileSize
new982 bytes
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,498 pass(es). View

I ckecked again the functionality without the modernizr.details code and it works perfect in firefox, chrome and safari.

caniuse.com

The element generates a simple no-JavaScript widget to show/hide element contents, optionally by clicking on its child element. (http://caniuse.com/#search=details)

Maybe the functionality show/hide is not working in some browsers but if the information is displayed this is not a problem from my point of view.

Other option is to use different tags like I said in #32, span or div tags can work.

Remaining tasks
- Test the functionality in other browsers
- Check if this patch affects other functionalities

espurnes’s picture

Status:Needs work» Needs review
bendev’s picture

I tested #33 and it works as you described

we could replace the return in the check by something inspired from this in order to allow some specific handling

// @js: Use Modernizr to check the navigator capacities
if( !Modernizr.details ) jQuery(“html”).addClass(“no-details”);

source http://html5doctor.com/the-details-and-summary-elements/

Modernizr.details returns
fierefox => false
chrome => true
safari => true

espurnes’s picture

Assigned:Unassigned» espurnes

working on bendev solution.

details is used to show/hide extra information in a way like an accordion does. This is css3 native (so, no need to use javascript). But right now just the following browsers support the details element:

Chrome 12+
Opera 15+
Safari 6+
iOS Safari 6.1+
Android Browser 4+
Blackberry Browser 10+
Opera Mobile 30+
Chrome dor android 45+
UC Browser for Android 9.9+

So IE, Edge, Firefox, Opera Mini, Firefox for Android and IE Mobile don't support it.

But at least in Firefox the open/close functionality doesn't work but the content is displayed. So the bendev approach extracted from here is a good option. We set a new class to tell the browser doesn't support details.

While I was writting this message I realised the classes .details or .no-details have been already setted to the html element. I can't find the code that adds these classes but they are there.

So no need to add the bendev code. I think patch #33 should work.

Remaining tasks
- Test the functionality in other browsers
- Check if this patch affects other functionalities

espurnes’s picture

Assigned:espurnes» Unassigned
espurnes’s picture

Ok the class is added by Modernizr.details.
But after applying #33 (that deletes the use of Modernizr.details in /core/misc/collapse.js) the class is added anyway. I can't find another place where Modernizr.details is called.

Aggregation of css and js is disabled and I cleared the cache.

Remaining tasks
- Test the functionality in other browsers
- Check if this patch affects other functionalities

source:https://modernizr.com/docs#using-modernizr-with-css

Using Modernizr with CSS
Modernizr's classes

By default, Modernizr sets classes for all of your tests on the root element ( for websites). This means adding the class for each feature when it is supported, and adding it with a no- prefix when it is not (e.g. .feature or .no-feature). This makes it super simple to add features in via progressive enhancement!

Say you include Modernizr's detection for CSS gradients. Depending on the browser, it will result in either or . Now that you know those two states, you can write CSS to cover both cases

bendev’s picture

In my option this can go RTBC

bendev’s picture

StatusFileSize
new1.75 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 117,313 pass(es). View
new49.1 KB

I noticed that the summary was missing on the comment tab
I adapted the corresponding JS to fix this

I propose the new patch in attachment for review

jaspreetsingh31’s picture

Status:Needs review» Needs work
StatusFileSize
new57.79 KB
new52.48 KB

I have tested the patch in multiple browsers like Chrome ( version: 45.0.2454.101 ) , Firefox ( version:41.0.1) , Microsoft Edge and in terms of functionality it works fine. But In chrome browser, it shows twice arrows but seems fine on FireFox and Microsoft Edge browser.

For sake reference, Please find the attachments.
doublearrow_bug_chromebrowser.jpg & Firefox browser_ screenshot.jpg

Thanks
Jaspreet singh

bendev’s picture

OK I think we finally understand the point.

we need to use the no-details class added automatically to firefox-like browsers (see #36 and #37) in order to display the sumarry arrows so that they don't appear twice in the other browsers...

I will work on this soon

bendev’s picture

Version:8.0.x-dev» 8.1.x-dev
StatusFileSize
new1.75 KB

Here is the reroll of patch #40 for 8.1.X-dev

what remain to be done is #41
+ check the summaries from menu which are gone (perhaps another issue for this)

bendev’s picture

Status:Needs work» Needs review
StatusFileSize
new2.18 KB

Sorry wron patch in #43

here it is with the fix for the double arrows . this one needs review

Can anyone confirm me if we are supposed to work against 8.1.x-dev as I assumed ?
+to do create another issue for the summary of menu tab

nod_’s picture

Version:8.1.x-dev» 8.0.x-dev
Status:Needs review» Needs work

8.1.x is outdated, nothing is happening there, should do the work on 8.0.x since Patch doesn't apply and we can't test it :)

nod_’s picture

Status:Needs work» Needs review
StatusFileSize
new4.6 KB
new3.26 KB

Polished the code, it works both with chrome and firefox. We could even split up the code more but let's leave that for another time.

Fixed a few inacurate comments and remaining eslint issues.

bendev’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new52.62 KB
new62.45 KB
new68.04 KB

testd in chrome firefox and safari

jaspreetsingh31’s picture

I have tested this double arrow issue on multiple browsers like chrome, Firefox, Microsoft edge and it works fine.
Status: Done
For sake reference, Please find the attachments.

Thanks

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 46: core-js-details-summary-1936708-46.patch, failed testing.

nod_’s picture

emma.maria’s picture

Removing the Novice tag as this issue needs some new direction for what the new patch needs to contain.

espurnes’s picture

StatusFileSize
new4.43 KB

Rerroled patch #46

droplet’s picture

Compare to #2493957: Add back errors & summary support to native HTML5 details tag:

- Both patched little more than main problem of issue thread.
- Both are good patches but slightly different on few code style. #2493957 is more close to other core scripts. But I like this patch move following code into `CollapsibleDetails`

+++ b/core/misc/collapse.js
@@ -12,11 +12,18 @@
+      this.$node.addClass('collapse-processed');

From my quick review of this patch, only missing following code from another issue thread:

+++ b/core/themes/classy/css/components/collapse-processed.css
@@ -30,3 +30,6 @@
+.collapse-processed .details-title {
+  color: #333;
+}

To standardize the color.
espurnes’s picture

StatusFileSize
new4.85 KB

I added suggestion from droplet #53 to collapse-processed.css