Current element values missing from vertical tabs when shown in 2-column layout

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

asd

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

still trying to fix img tag

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.

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

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)

This is not a Seven issue

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

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

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

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);

Patch corrected with good indentations.

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

Tks anansi
Mentored by Chipway

made @ DrupalConEur Barcelona

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"

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.

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.

Added Barcelona2015 tag to the issue.

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

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.

#23 tested.
this looks ok.

Thanks

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

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?

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

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       }

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

applying #23 and #14 not solves the problem.

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

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.

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

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

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

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

In my option this can go RTBC

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

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

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

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)

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

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

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.

testd in chrome firefox and safari

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

Most of that patch is in #2493957: Add back errors support to native HTML5 details tag.

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

Rerroled patch #46

Compare to #2493957: Add back errors 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.

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

Adding Novice for manual testing, needs screenshots.

This will need a re-roll and manual testing. Maybe manual test first to make sure this is still an issue to be solved before rerolling.

I'm going to attempt to re-roll patch 54 now for 8.4.

On Drupal 8.4.0-dev there is no such information. I have no patch running.

See that shot to make sure I was on the right page or view:

So which patch should I test?

Tested manually and re-rolled the patch. Works local.

I'll test the patch in #66. Glad you've managed to re-roll.

Hi everyone! I'm at Drupalcon Vienna2017 and I'm a bit confused by the status of this issue.

I just applied the 1936708-66.patch against 8.5.x-dev and it seems to be working as intended. (see screenshot)

What else needs to be done? (First time sprinter, instructions unclear... :)

EDIT:

Validated correct function in MacOSX 10.12.6 (16G29) using
Chrome 60.0.3112.113;
Safari 11.0 (12604.1.38.1.7);
Firefox 52.0.2 (64-bit)

Confirming that patch in #66 is working for me too.

Confirming that it works for me too. 👍

We have JavaScript testing in core now, we need a test here so we don't lose it again.

Thanks everyone

Rebasing

Patch applies cleanly to 8.9.x-dev.

After applying the patch from #79 and checking out a node edit form:
1) the span tags with class summary are not empty (good),
2) but the span contents are not displayed in the UI (not good).

(Seven theme, on Firefox and on Chrome)

Commenting out display: none; produces a result similar to the result in #70, but this seems to conflict with the issue mentioned in the preceding @todo;

/**
 * Hide JS summary from the details polyfill to make it consistent with native
 * details elements.
 *
 * @todo Consider removing this after https://www.drupal.org/node/2493957 has
 *   been solved.
 */
.entity-meta .seven-details .summary {
  display: none;
}

Not sure what the status should be, Removing the 'Novice' tag.

doesn't apply cleanly on 9.1.x

Like tvb said, Need to remove the CSS that hides the summay, otherwise this patch doesn't change anything visually.

Rerolled patch for 9.1.x and removed the CSS that hides the summary as mentioned in #85, please review.

Test Fixed, Please review.

Verified and tested by applying the patch #90.It was applied successfully. Can be moved to RTBC.
Had one observation for the Claro theme the text didn't display.

Steps to test -
1. Go to the admin site.
2. Go to admin/appearance.
3. Select theme Bartik/Claro/Seven/Default
4. Edit any of the created content type.
5. Verify.

Seven_theme_Chrome

Seven_theme_Safari

Bartik_theme_Safari

Bartik_theme_Chrome

Claro_theme_Safari

Claro_theme_Chrome

When you create a JavaScript patch please make sure to always run yarn run lint:core-js-passing before sending the patch, for the whole workflow have a look here: https://www.drupal.org/node/2986680

Lately the JS patches have had issues with not following core standards so please make sure to use the tools we have in core to reduce the workload of reviewers, Thank you.

Claro overrides some things about collapse.js, needs to be fixed.

I noticed most of the logic was modifying collapse.js. That is a polyfill for browsers that don't support <details>. Browsers that didn't need the polyfill were getting some of it anyway, and I think it may prove difficult to maintain. I opted to move the functionality to a dedicated JS file.

The CSS changes are pretty straightforward. Claro needed some additional JavaScript changes to remove its own workarounds for this issue, otherwise summaries would be duplicated in some instances.

Added test. The test addition is the only change, but there is a dedicated interdiff as a bit of formatting was changed.

1936708-97-TEST-ONLY failing was expected, so switching back to NR.

Re-adding with the Test-only patch first so testbot doesn't kick back to Needs review.

you can name it --FAIL.patch instead of TEST-ONLY.patch. It'll do what you want :)

Looking at it, it definitely should be in the collapse file. Because now there are a lot of duplications. There is a theme function in the details.js file, but the same thing is hardcoded in the collapse.js file. What is done in details.js is a light version of collapse.js. some of what's in claro details.js could be in core, I don't see why we can't have summary tags trigger when hitting space everywhere.

Because now there are a lot of duplications

Ah, that's right, collapse,js is a polyfill but it doesn't replace details.js, it runs in addition to it.

Looking at it, it definitely should be in the collapse file.

My concern is that this file has been long understood as strictly being a polyfill for browsers that don't support <details>. There has been interest in replacing this with a different polyfill #1839158: Replace collapse.js with a proper polyfill for <details>, and this would be made far more difficult by splicing in functionality that applies to all browsers.

If there is still a preference to include the fix in collapse.js, there would be a need to edit several comments in the file that reference itself as a polyfill, which it would no longer be, starting with @file,

/**
 * @file
 * Polyfill for HTML5 details elements.
 */

My preference would be to address the duplication by removing the summary logic in collapse.js, and make sure that the new summary logic is compatible with the collapse.js polyfill

some of what's in claro details.js could be in core

I'm I correct in interpreting that this is a reference to the shim that's already there, not the changes i the patch? That does seem like something better applied to core. I believe it would be out of scope here, but tagging with needs followup as a reminder to create that issue.

Right, like you said, moving the code for setupSummary from collapse.js to details.js is good.

And yes about the claro code, it's about what already exists you're correct.

Created followup #3160367: Move Claro's polyfills in details.es6.js to core for the claro functionality that should be in core.

Removed the now-redundant code from the collapse polyfill.

The patches that should have been with #105

1. +++ b/core/misc/collapse.es6.js
   @@ -24,8 +24,6 @@
   -    // Initialize and setup the summary,
   -    this.setupSummary();
   

   Checked that there aren't any instances where it would be expected that this.setupSummary gets called http://grep.xnddx.ru/search?text=setupSummary&filename=

2. +++ b/core/misc/details.es6.js
   @@ -0,0 +1,110 @@
   +  function DetailsElement(node) {
   ...
   +  Drupal.DetailsElements = DetailsElement;
   

   I'm pondering if we should make this more specific to the functionality we're providing. The current names sound very generic 🤔

3. +++ b/core/misc/details.es6.js
   @@ -0,0 +1,110 @@
   +      setupSummary() {
   ...
   +      setupLegend() {
   

   What are the benefits of splitting this into two functions?

4. +++ b/core/misc/details.es6.js
   @@ -0,0 +1,110 @@
   +  Drupal.theme.detailsSummarySummary = () => `<span class="summary"></span>`;
   ...
   +  Drupal.theme.detailsSummarySummaryText = text => (text ? ` (${text})` : '');
   

   detailsSummarySummary sounds a bit strange. Would be great if we come up with a name that doesn't require using summary twice.

This addresses #107

An explanation for how I addressed item 3:

What are the benefits of splitting [setupSummary(), setupLegend()] into two functions?

It was two separate functions in collapse.js, but looks like the separation was no longer needed after #1685140: Refactor collapse.js. They were separate to address cover Drupal's former use of fieldset+legend to achieve details+summary like behavior. I did some refactoring of both details and the polyfill to make this clearer, and checked http://grep.xnddx.ru/ to ensure the refactoring wouldn't cause problems for contrib.

Based on the discussion on Slack with @bnjmnm we should still consider if we could improve the naming to be more descriptive on what is actually being done. DetailsEnhanced sounds like the details element is enhanced which is not 100% accurate because what we're actually doing, is just generating summary for the details based on form input.

Did some renaming (and refactoring due to it)

Forgot the patch in 110 😛

Synopsis does not work for me: It's a complicated word, the meaning is not obvious, it's hard to say (for me at least). For exemple I honestly do not know how to pronounce: synopsized.

I would suggest brief, then we have names like: DetailsBriefSummary, Drupal.theme.detailsBriefSummary, Drupal.theme.detailsBriefSummaryText, etc. It makes sense when used both as a noun or an adjective. It's not an usual word used in webdev though.

A better one might be outline, benefit is that it's a well known term in HTML land, accessibility, in CSS (with a different meaning but still the word is already known). And it doesn't rely on a pun to be useful. DetailsSummaryOutline it's more descriptive, it's an outline inside a details inside a summary.

In any case, not synopsis.

Those are both much better options that synopsis, thanks!
I opted for outline. Both brief and outline have multiple meanings that could potentially be confusing, but when seen in context I think the confusion for "outline" is mitigated more quickly.

We can probably get away with DetailsOutline, the fact that it's in the summary element is kind of an implementation detail. In any case, much better already, thanks!

Since this summary is at least at the moment only being used in forms, how about we call it DetailsFormSummary? Or if we think we should be open to outside of form implementations, we could use DetailsGeneratedSummary. The reason I like summary is that the API is still using the word summary in other places, for example drupalGetSummary and summaryCallback.

At the Claro weekly check-in we discussed the naming and agreed on DetailsSummmarizedContent. Summary is the most accurate term, and using "Summarized" eliminated the confusion with the <summary> element. The patch that renames it should also see if any comments need to be changed to reflect the renaming.

Renamed per #116

That was fast! I was pretending to assign the issue to me when I finished to read comment #116 but I was to slow hahaha
So if patch #117 passes on tests, I will test it. Thanks @bnjmnm!

Cheers, Paulo.

Patch #117 looks good to me!

I tested with Seven and Claro Theme. (I attached images to prove it)
Another point is that the patch should use DetailsSummmarizedContent term and I can confirm that the patch do it right.
All comments mention the right term too.

Set to RTBC.

Cheers, Paulo.

1. +++ b/core/misc/details-summarized-content.es6.js
   @@ -0,0 +1,105 @@
   +  $.extend(
   +    DetailsSummarizedContent,
   ...
   +      instances: [],
   +    },
   +  );
   

   We are not adding the instances to the list at the moment so this is always empty.

2. +++ b/core/themes/claro/js/details.es6.js
   @@ -55,4 +55,23 @@
   +  Drupal.theme.detailsSummarizedContentText = text => text || '';
   

   Should we prepend a whitespace to the beginning of the text like the default instance of this theme function does?

#120.1 Removed that extend.

#120.2 Whitespace shouldn't be appended in Claro as its design has the extended summary is enclosed in a block so it appears on the next line.

+++ b/core/misc/details-summarized-content.es6.js
@@ -0,0 +1,93 @@
+  Drupal.behaviors.detailsSummary = {
+    attach(context) {
+      const $detailsElements = $(context)
+        .find('details')
+        .once('details');
+      $detailsElements.map(
+        (index, details) =>
+          // eslint-disable-next-line no-new
+          new DetailsSummarizedContent(details),
+      );
+    },
+  };

The instances property was used here for 2 reasons: 1. allow scripts to access the created objects in case they need an override of some kind. 2. not have the eslint warning about using new for sideeffects.

We should keep the created instances so they're accessible from the outside so I'd do:

  Drupal.behaviors.detailsSummary = {
    attach(context) {
      const $detailsElements = $(context)
        .find('details')
        .once('details');

      DetailsSummarizedContent.instances = DetailsSummarizedContent.instances.concat(
        $detailsElements
          .map((index, details) => new DetailsSummarizedContent(details))
          .get(),
      );
    },
  };

  Drupal.DetailsSummarizedContent = DetailsSummarizedContent;

Thanks @nod_, that explanation helps make sense of why it's done and better way to do it.

That's good with me :) thanks

This looks pretty good for me too. Here's few minor changes we still have to do. Feel free to move this back to RTBC once they have been addressed.

1. +++ b/core/themes/seven/css/components/entity-meta.css
   @@ -59,14 +59,13 @@
   +
   

   themes/seven/css/components/entity-meta.css
    72:1  ✖  Expected no more than 1 empty line   max-empty-lines
   

2. +++ b/core/themes/claro/css/components/accordion.pcss.css
   @@ -34,13 +34,8 @@
   +  color: var(--color-davysgrey);
   
   +++ b/core/themes/claro/css/components/details.css
   @@ -597,18 +597,11 @@ rgba(0, 0, 0, 0.1);
   +  color: var(--color-davysgrey);
   

   These are referring to a non-existing variable. These should be renamed to --color-davysgray.

Made the changes suggested in #125. Please review

Thanks @ayushmishra206

In #126
1. Updates the source accordion.pcss.css, but the complied version isn't present and the unchanged CSS would be loaded
2. Changes the compiled details.css, when the changes could be made to details.pcss.css then complied to details.css.

More on how Claro uses PostCSS to build CSS can be found here https://www.drupal.org/node/3084859

Updated patches attached and returning to RTBC as #125 said that would be fine.

Committed 6e6ad99 and pushed to 9.1.x. Thanks!