Problem/Motivation

In order to implement the new create content design (screenshot of design included), the vertical tabs used for settings fields on node forms need to be re-done as a set of collapsibles. These will ultimately be rendered within a two-column layout, but this issue focuses on implementing the change from vertical tabs to collapsibles.

Current status of this patch looks like this:

Only local images are allowed.

Proposed resolution

It would be nice to implement this using #1168246: Freedom For Fieldsets! Long Live The DETAILS., however at this time before feature freeze we should not reply on that, but use Core's existing collapsible form elements. Styling does not have to exactly match the proposed design at this point. The focus of this issue should be getting the feature working, looking good in Stark and acceptable in Seven. The two-column layout will be done in #1838156: Implement the two-column layout for the new create content page. The polish to match the design will be applied in #1751754: Implement new form style for Seven, based on blueprint mockups..

Remaining tasks

Files: 
CommentFileSizeAuthor
#90 90.patch5.08 KBsun
PASSED: [[SimpleTest]]: [MySQL] 49,115 pass(es).
[ View ]
#90 interdiff.txt566 bytessun
#89 89.patch5.06 KBsun
PASSED: [[SimpleTest]]: [MySQL] 49,122 pass(es).
[ View ]
#89 interdiff.txt3.61 KBsun
#88 1838114-89.patch5.25 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 49,175 pass(es).
[ View ]
#79 1838114-79.patch14.67 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 50,714 pass(es).
[ View ]
#77 1838114-77.patch15.67 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 44,892 pass(es), 2,953 fail(s), and 1,172 exception(s).
[ View ]
#74 1838114-74.patch15.69 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 44,938 pass(es), 2,978 fail(s), and 1,337 exception(s).
[ View ]
#74 interdiff.txt8.24 KBswentel
#74 Screen Shot 2013-01-15 at 17.41.36.png29.56 KBswentel
#67 drupal8.entity-meta.67.patch14.89 KBry5n
PASSED: [[SimpleTest]]: [MySQL] 49,622 pass(es).
[ View ]
#67 drupal8.entity-meta.59-67.interdiff.txt3.68 KBry5n
#60 drupal8.entity-meta.59.patch14.51 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 50,764 pass(es).
[ View ]
#60 interdiff.txt7.03 KBswentel
#52 drupal8.entity-meta.52.patch8.48 KBsun
FAILED: [[SimpleTest]]: [MySQL] 50,675 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#45 details-container.png32.18 KBswentel
#44 core--node-form-collapsibles--1838114--44.patch8.83 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 50,553 pass(es).
[ View ]
#44 1838114-39-to-44.txt959 bytesswentel
#44 details.png18.36 KBswentel
#41 modernizr-details-check.png19.29 KBjessebeach
#39 core--node-form-collapsibles--1838114--38.patch9.23 KBry5n
PASSED: [[SimpleTest]]: [MySQL] 49,266 pass(es).
[ View ]
#39 1838114--25-to-38--interdiff.txt6.52 KBry5n
#38 red-to-yellow-in-opera-2.png79.19 KBjessebeach
#38 red-to-yellow-ie10.png53.33 KBjessebeach
#38 red-to-yellow-in-firefox-windows.png51.98 KBjessebeach
#38 red-yellow-gradient.png53.08 KBjessebeach
#29 i1838114-29-node-add-regular-user-ff.png19.23 KBattiks
#29 i1838114-29-node-edit-regular-user-ff.png14.6 KBattiks
#29 i1838114-29-permissions-regular-user.png92.23 KBattiks
#25 core--node-form-collapsibles--1838114--25.patch8.97 KBry5n
PASSED: [[SimpleTest]]: [MySQL] 49,302 pass(es).
[ View ]
#23 core--node-form-collapsibles--1838114--23.patch8.59 KBry5n
PASSED: [[SimpleTest]]: [MySQL] 48,739 pass(es).
[ View ]
#23 core--node-form-collapsibles--1838114--23.interdiff.txt446 bytesry5n
#21 node-vertical-tabs-to-fieldsets.png46.43 KBeigentor
#20 core--node-form-collapsibles--1838114--20.patch8.92 KBry5n
PASSED: [[SimpleTest]]: [MySQL] 48,754 pass(es).
[ View ]
#20 core--node-form-collapsibles--1838114--20.interdiff.txt1.63 KBry5n
#17 core--node-form-collapsibles--1838114--17.patch10.34 KBry5n
PASSED: [[SimpleTest]]: [MySQL] 48,733 pass(es).
[ View ]
#17 interdiff-17-with-11.txt574 bytesry5n
#11 core--node-form-collapsibles--1838114--11.patch9.92 KBswentel
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#11 interdiff.txt914 bytesswentel
#8 core--node-form-collapsibles--1838114--6.screen.jpg95.76 KBry5n
#6 core--node-form-collapsibles--1838114--6.patch10.63 KBry5n
FAILED: [[SimpleTest]]: [MySQL] 48,062 pass(es), 409 fail(s), and 373 exception(s).
[ View ]
#3 node-form-collapsibles--seven--3.png52.47 KBry5n
#3 node-form-collapsibles--stark--3.png53.19 KBry5n
#3 core--node-form-collapsibles--1838114--3.patch5.32 KBry5n
FAILED: [[SimpleTest]]: [MySQL] 48,078 pass(es), 404 fail(s), and 364 exception(s).
[ View ]

Comments

Thanks ry5n, sounds like a good plan in here.

Tagging

StatusFileSize
new5.32 KB
FAILED: [[SimpleTest]]: [MySQL] 48,078 pass(es), 404 fail(s), and 364 exception(s).
[ View ]
new53.19 KB
new52.47 KB

After reviewing the state of #1168246: Freedom For Fieldsets! Long Live The DETAILS., it's looking good but still needs work. It would be inadvisable to rely on it, so I updated the issue summary to reflect that. Also there is some possible interdependency with #1751606: Move published status checkbox next to "Save" because both are altering Node tests and NodeFormController, sometimes the same functions.

That said, here is a first patch based on the old work in #1510532: [META] Implement the new create content page design. It works, looks OK in Stark, but Seven needs work.

Category:feature» task

The main issue #1510532: [META] Implement the new create content page design is tracked as a task, so this should match.

Category:task» feature

I guess this is a feature after all.

Status:Needs work» Needs review
StatusFileSize
new10.63 KB
FAILED: [[SimpleTest]]: [MySQL] 48,062 pass(es), 409 fail(s), and 373 exception(s).
[ View ]

OK, a patch for review. This gets us most of the way. One bug I know of is that the 'create new revision' checkbox is checked by default for creating new nodes, which is wrong. I need help fixing that.

This patch also fixes the form api bug that fieldsets with #title_display set to 'invisible' were not given the proper 'element-invisible' class.

Priority:Normal» Major

Since this is part of #1510532: [META] Implement the new create content page design, which is major, making this match.

Finally, a screen of what this looks like in Chrome.

patch 6 screenshot

Issue summary:View changes

Decided not to rely on #1168246; noted possible interdependencies with #1751606.

Issue summary:View changes

Add links to the layout issue.

Issue summary:View changes

Shortlink to layout issue didn't take.

Status:Needs review» Needs work

The last submitted patch, core--node-form-collapsibles--1838114--6.patch, failed testing.

I need help fixing these failing tests. Some of the changes in NodeFormController may be the culprit, or the addition to form.inc to allow #title_display for #type fieldset. I've been digging around but haven't made much progress. Again, I need help to get this in before feature freeze!

Status:Needs work» Needs review
StatusFileSize
new914 bytes
new9.92 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

I think this one will fix a lot of of tests. NodeFormController.php contained code for the existince of the path alias - see interdiff - but the module is not always enabled. Also, that shouldn't belong there anyway, it should belong in path.module. I assume that code is for the new content page design, because I don't those items on my screen at all - unless I miss something. Anyway, just removed it for now, let's see how many fails we have left.

@swentel Thank you! Let's see what testbot thinks…

Hm. What's visible in the OP's screenshot looks 100% like details to me? At least the shot depicts more or less exactly how user agents are supposed to render details elements...?

@sun: Yes. I want to use <details> here, but that patch is not in yet. Current implementation uses the normal collapsible fieldsets, I am hoping that it will require minimal refactoring once the details patch is in.

Status:Needs review» Needs work

The last submitted patch, core--node-form-collapsibles--1838114--11.patch, failed testing.

@ry5n - only 5 exceptions, but should be easy to find, it's only notices. I'm off to other patches again :)

Status:Needs work» Needs review
StatusFileSize
new574 bytes
new10.34 KB
PASSED: [[SimpleTest]]: [MySQL] 48,733 pass(es).
[ View ]

@swentel Thanks a million. I think I know where those remaining notices are coming from, too. Let's see:

Woohoo! I've tested in Stark and in Seven. Just needs accessibility review and code review.

Issue tags:+JavaScript

Touches a JS file.

Issue tags:-JavaScript
StatusFileSize
new1.63 KB
new8.92 KB
PASSED: [[SimpleTest]]: [MySQL] 48,754 pass(es).
[ View ]

@_nod Ah, good catch! This made me realize that this change to node.js and also one other line in NodeFormController are actually duplicate code changes from #1751606: Move published status checkbox next to "Save" and are already included there. New patch removes those lines (so no JS changes).

StatusFileSize
new46.43 KB

Again, just a functional test. Works as expected in all browsers I tested.
Hopefully this will attract some savvy people to do the code reviews.

Only local images are allowed.

The fieldsets do not have accordion functionality at the moment and all stay open. I guess that is intentional and was the last consensus, or is not part of this patch?

@eigentor Thanks! Yes, there is no accordion functionality (as in, only one open fieldset at a time); I believe this is by design.

(Just a note for reviewers: the screenshot in #21 includes changes from #1751606 as well as this patch.)

StatusFileSize
new446 bytes
new8.59 KB
PASSED: [[SimpleTest]]: [MySQL] 48,739 pass(es).
[ View ]

New patch hides the fieldset summaries with CSS display: none. I'm very much open to better ideas.

Think it would be sensible to remove the Author line from the Node->isNew.

$author = '<span class="label">' . t('Author') . ':</span> ' . user_format_name($GLOBALS['user']);

It would be one less line and considering the node isn't saved and the user can change the author to someone else, it may save some confusion brought by the autocomplete author change not being reflected in the summary.

That OR we always keep them in sync similar to the way the summaries are updated with Jquery in node.js with
drupalSetSummary()

Something to the effect of

var authorLabel = '<span class="label">' + Drupal.t('Author') + ':</span> ';
$('#edit-author').html(authorLabel + Drupal.t('@name', { '@name': name }));

StatusFileSize
new8.97 KB
PASSED: [[SimpleTest]]: [MySQL] 49,302 pass(es).
[ View ]

New patch merges in changes from HEAD, and fixes the following:

  • Fixed: Author is no longer displayed (as current user) on node add form.
  • Fixed: Node add form no longer has the 'create new revision' checkbox checked by default.
  • Fixed: CSS refactored to be simpler and more visually consistent.

Looks like there are also bugs stemming from #1168246: Freedom For Fieldsets! Long Live The DETAILS.:

  • The Form API #group property only ever applied to fieldsets, and a lot of core modules add their node settings with #group. So now, #group only works with details, therefore the collapsibles in this patch are wrapped in an outer details element (which makes no sense). What is the best way for other modules to add their fields here, without using #group? And any consequences?
  • IE8 does not like the new details markup and/or changes to collapse.js (TBD). Behaviour and styling is broken, including class="collapsed" not being applied on state change.

Why you are using the [id="edit-settings-summary"] selector and later #edit-publish-state?

@yannickoo Selector specificity. I don’t want to add specificity unless it’s needed; using id selectors carelessly can cause all sorts of problems with related styles being unexpectedly overridden. So I used the attribute selector to traget the id.

With #edit-publish-state, that is truly a unique element with no descendants, so using the id selector shouldn't be problem and is slightly more performant.

Working as expected http://screencast.com/t/FL9luIsf . Code review wanted.

#26 & #27 same question, but probably me, what is the difference between [id="edit-settings-summary"] and #edit-settings-summary

+++ b/core/themes/seven/style.cssundefined
@@ -1401,3 +1400,117 @@ details.fieldset-no-legend {
+[id="edit-settings-summary"] {

why don't your write #edit-settings-summary

+++ b/core/themes/seven/style.cssundefined
@@ -1401,3 +1400,117 @@ details.fieldset-no-legend {
+[id="edit-settings-summary"] > .fieldset-wrapper {

why don't your write #...

+++ b/core/themes/seven/style.cssundefined
@@ -1401,3 +1400,117 @@ details.fieldset-no-legend {
+[id="edit-settings-summary"] .label {

why

+++ b/core/themes/seven/style.cssundefined
@@ -1401,3 +1400,117 @@ details.fieldset-no-legend {
+[id="edit-settings-summary"] .form-item {

why don't your write #...

#28 I don't see this, am I missing something?

I tested this with a user with no access to revisions, see screenshots:

i1838114-29-node-add-regular-user-ff.pngi1838114-29-node-edit-regular-user-ff.png

@attiks [id="thing"] has specificity 0,0,1,0 (same as .thing) while #thing has 0,1,0,0. Practically speaking, that means that #thing a will override .thing .subthing a.my-special-link:hover, which can get frustrating, especially if the first selector is in another stylesheet. I've gotten into the general habit of avoiding id selectors for this reason. Also, I would not normally write styles like this, but I don't yet know how to abstract this into a reusable UI component based on classes. I know it looks a bit weird. Does that help to explain?

#30 Yes it does, I thought they were the same, thanks for explaining.

@attiks Ah, good! Also, thanks for catching that bug with user permissions and revisions. I’ll take a stab at sorting that out.

Hmmm… I’m not having the easiest time figuring out the logic for the revision fields display. Can anyone help who knows the node and revision systems? Just need to make sure the revision fields get shown/hidden for users with appropriate permissions.

@ry5n: not sure about your question on revisions. Can you elaborate a bit?

In the meantime here is a review of the PHP code :)

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.phpundefined
@@ -108,20 +108,44 @@ public function form(array $form, array &$form_state, EntityInterface $node) {
+      $last_saved = '<span class="label">' . t('Last saved') . ':</span> ' .format_date($node->changed);

Minor code style issue.

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.phpundefined
@@ -108,20 +108,44 @@ public function form(array $form, array &$form_state, EntityInterface $node) {
+    $form['settings_summary'] = array (
...
+    $form['settings_summary']['revision_information'] = array(

It looks strange to me that a settings summary is not just a summary, it contains actual settings. I'd rename this to something appropriate like "publication_settings" or somesuch.

....

Except that it looks like it only contains the revision settings. Hm? Why?

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.phpundefined
@@ -132,30 +156,35 @@ public function form(array $form, array &$form_state, EntityInterface $node) {
+    $new_revision_control_default_value = false;

As per Drupal coding standards, this should be FALSE.

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.phpundefined
@@ -132,30 +156,35 @@ public function form(array $form, array &$form_state, EntityInterface $node) {
+    $form['additional_settings'] = array(
+      '#type' => 'details',
+      '#weight' => 99,
     );

Erm, what do you want/need this additional settings item for? Would this be a place for contrib stuff? Would be good to document.

@Gábor Thanks for the review; much appreciated.

About the revision fields: the textarea (not the checkbox) seems to display to users without revision permissions (see #29). I haven't been able to figure out why yet; the logic seems to be the same as before this patch. It's probably simple, but it could use another set of eyes.

The $additional_settings is the trickiest problem. In D7 it was the vtabs container; many modules (including node module) add their node settings to this element using the Form API #group property. However, it should not be type=details. There's a separate issue for this problem: #1856178: '#group' Form API property only works with <details> elements. For now, I'll comment this with an explanation and a @todo.

Also, I agree with changing the name of $settings-summary. I'll reroll with a better name for that container and fixes for the coding standards missteps.

Code

Generally the CSS looks good. Using attribute selectors to get ids to keep the selector weight down is quite clever.

Watch for code styling issues like this. The properties should be in alphabetical order. For the most part they are. Just a few outliers:

.node-form-revision-information {
  padding: 0;
  background: transparent;
  margin-bottom: .5em;
  border: 0;
}

Webkits are the only browsers that need prefixes for gradients nowadays.

[id="edit-additional-settings"] details:first-child,
[id="edit-additional-settings"] .collapsed + details,
[id="edit-additional-settings"] details:not([open]) + details {
  background-image: -webkit-linear-gradient(top, rgba(0, 0, 0, .15), transparent 5px);
  background-image:    -moz-linear-gradient(top, rgba(0, 0, 0, .15), transparent 5px);
  background-image:      -o-linear-gradient(top, rgba(0, 0, 0, .15), transparent 5px);
  background-image:         linear-gradient(to bottom, rgba(0, 0, 0, .15), transparent 5px);
  border-top: 0 none;
  padding-top: 1px;
}

I'd rather see the selector weight and cascade placement determine style overrides and not an !important. !important is like adding 1,0,0,0,0 to your selector.

/* Force closed details elements to appear closed. */
[id="edit-additional-settings"] .collapsed,
[id="edit-additional-settings"] details:not([open]) {
  background-color: #f2f2f2 !important;
  background-image: none !important;
  border-bottom: 1px solid #a5a5a5 !important;
  border-top: 1px solid white !important;
  padding-top: 0 !important;
}

Where possible, sizing that should vary with the size of fonts should be declared in a relative unit, like em

[id="edit-settings-summary"] > .fieldset-wrapper {
  padding: 16px;
}

Decimals should be preceded by a zero.

[id="edit-settings-summary"] .form-item-log {
  margin-top: .5em;
}

Accessibility

If we're going to use the details element, we'll need to polyfill it heavily. Currently only Chrome supports it, no version of IE, and it has zero accessibility support. So as it stands, it's a little worse than a div right now :)

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

We might add a Modernizr test for details.

More info about accessibility of the details element: http://www.accessibleculture.org/articles/2012/03/screen-readers-and-det...

Webkits are the only browsers that need prefixes for gradients nowadays.

We should discuss that in a separate issue first. I already noticed that the new Toolbar landed without any non-webkit prefixes, and at least on my local copy of Firefox stable, I don't see any of the fanciness that apparently exists in Chrome there. In short, I wish that statement was true, but alas, I cannot confirm it through actual testing.

Glad to provide some test images.

Here's a red to yellow gradient using linear-gradient in Firefox 17 (stable)

red to yellow gradient

And the same in Opera 12.11 (stable)

red to yellow gradient in Opera 12.11

and in IE10

red to yellow gradient in IE10

FIrefox 17.01 in Windows

red to yellow gradient in Firefox on Windows

caniuse.com lists Firefox as being prefix free for the stable and stable-1 releases: http://caniuse.com/#search=linear-gradient

side note: the gradient in the Toolbar is very subtle. Perhaps too so: background-image: linear-gradient(rgba(255, 255, 255, 0.125) 20%, transparent 200%);

StatusFileSize
new6.52 KB
new9.23 KB
PASSED: [[SimpleTest]]: [MySQL] 49,266 pass(es).
[ View ]

Patch attached addresses Gábor’s points in #34 and Jesse’s points in #36, except with regards to vendor prefixes. I just want to double-check before I remove ones for Opera and FF. Firefox for example went unprefixed with v16, which was only two months ago. Is there an existing Drupal standard for supporting older 'modern' browsers? This may indeed be something for another issue.

About the details element, it's what Drupal now uses for collapsibles so I'm not sure there's another option for that. Work on a polyfill is happening in #1839158: Replace collapse.js with a proper polyfill for <details>. Also, this patch remains funny-looking in Chrome until #1856178: '#group' Form API property only works with <details> elements is solved. However, I think both are beyond the scope of this issue.

Thanks to all for the reviews; hopefully this is close.

I think the visibility of the revisions textfield is by design - not actually a problem? Looking at the code, the fieldset has more flexible conditions for visibility vs. the checkbox and that was like that before or after the patch either way. So if you don't have administer nodes, but you are creating a new revision (such as due to forced node type defaults), you would have the possibility to provide a log message. This sounds like a feature, not a bug. Maybe worth a comment on the code so people coming after us are aware :)

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.phpundefined
@@ -108,20 +108,44 @@ public function form(array $form, array &$form_state, EntityInterface $node) {
-    $form['revision_information'] = array(
-      '#type' => 'details',
@@ -132,30 +156,42 @@ public function form(array $form, array &$form_state, EntityInterface $node) {
       '#access' => $node->isNewRevision() || user_access('administer nodes'),
     );

The wrapper used to have this access check. So the whole fieldset either showed or not if you had administer nodes or you were creating a new revision.

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.phpundefined
@@ -132,30 +156,42 @@ public function form(array $form, array &$form_state, EntityInterface $node) {
       '#type' => 'checkbox',
       '#title' => t('Create new revision'),
...
       '#access' => user_access('administer nodes'),

However the checkbox will only ever show if you have administer nodes. So if you create a new revision but don't have admin nodes access, you see the textfield but not the checkbox.

StatusFileSize
new19.29 KB

Turns out that the Modernizr build in core is checking for details

modernizr-details-check.png

And the modules page form is using details. ry5n, did you look at this code when you were putting this patch together? Just want to make sure we using the same patterns.

@Jesse The short answer is 'yes' :)

All of core's collapsible widgets now use <details>. Instead of specifying '#type' => 'fieldset', '#collapsible' => TRUE, all that’s required is '#type' => 'details'. This was done globally in #1168246: Freedom For Fieldsets! Long Live The DETAILS. and nothing had to be changed for this patch. However, as previously noted (#39), the $additional_settings element should not be a 'details' and should be changed to a simple container once there's a fix for #1856178: '#group' Form API property only works with <details> elements.

So where are we with this? I think we’ve addressed all concerns brought up so far… Is there anything else that needs work?

StatusFileSize
new18.36 KB
new959 bytes
new8.83 KB
PASSED: [[SimpleTest]]: [MySQL] 50,553 pass(es).
[ View ]

New patch with one small change regarding the default state of the revision checkbox which now defaults back to the default behavior we have in core. @ry5n - is/was there a specific reason why this was done the way you implemented it in the patch, because I couldn't find an immediate reason for it.

Next up, I'm going to look at #1856178: '#group' Form API property only works with <details> elements, because, as seen in the screenshot underneath, this is not really nice, and probably a blocker to commit.

details.png

StatusFileSize
new32.18 KB

Posted a patch over at #1856178: '#group' Form API property only works with <details> elements which allows to 'group' elements in the 'container' type. If you change the #type in the 'additional_settings' to 'container', this result will look like screenshot underneath.

details-container.png

Status:Needs review» Needs work
Issue tags:-Usability, -needs accessibility review, -Seven, -#d8ux

The last submitted patch, core--node-form-collapsibles--1838114--44.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Usability, +needs accessibility review, +Seven, +#d8ux

@swentel Thank you for working on this, also the #group issue! About that checkbox, I find it confusing that ‘create new revision’ would be checked before the node is even created: something ha to exist before one can revise it! Perhaps this is a UI issue? I can understand why under the hood, there wouldn’t be a difference…

@ry5n - I agree it's indeed a bit confusing. It's the same behavior as in core right now, but I guess it would make sense to not toggle it when creating a new article. I'll have a look at it as we can rely on the nid being set or not.

As an addition to #48 en #49 - maybe it would make even more sense to hide the checkbox and the textfield completely on node/add/*, that was how the original patch worked right ?

Status:Needs review» Needs work

Hm. Why is this patch still implementing fieldsets? Details are in, let's use them.

With that, the majority of the CSS should be obsolete. Likewise, the functional code changes should be minimal.

Details are "Collapsibles", by design. Based on the screenshot in #0, the primary desired change here seems to be to just simply remove the vertical tabs container?

Title:Move node form’s vertical tabs to collapsiblesChange node form’s vertical tabs into "advanced" column holding meta info and details
Status:Needs work» Needs review
StatusFileSize
new8.48 KB
FAILED: [[SimpleTest]]: [MySQL] 50,675 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

I've rewritten the implementation and vastly simplified it.

Also adjusting issue title to clarify the actual goal.

I have a lot of concerns with the proposed design, but need to think about them some more. The current, prepended meta info container doesn't make sense, since it duplicates editable form controls elsewhere in the interface. I saw "Published" and wanted to click it to change it. I saw the author name, and wanted to change it. I didn't see the authored/creation date anywhere (and I wanted to change that, too).

Issue tags:-Usability, -Seven

@sun Can you detail the issues with the existing implementation? It did in fact use details for the collapsible components, as the details patch went in partway through this issue. Sorry for not updating the issue summary to reflect that. So, I was under the impression that #42/#44 was almost RTBC, with only #1856178: '#group' Form API property only works with <details> elements blocking.

As for the design, there are surely improvements to be made; some people may indeed be confused about the summary area at the top of the meta settings. However, this design was user-tested and results were positive. More importantly, this is an implementation issue and we should discuss improvements elsewhere.

Issue tags:+Usability, +Seven

Putting those tags back. Gah, why do tags remove themselves like that?

Issue tags:-Usability, -Seven

Sure:

- CSS should not rely on IDs, but use semantic/sensible + re-usable class names instead. The new code uses generic CSS classes, so the visual design can be re-applied to other content entities more easily. (But alas, also removed.)

- The PHP code contained too many specific customizations that were hardly overridable. Something as generic as the node form still has to remain easily customizable for varying use-cases.

- Less fieldsets/details, but containers instead.

- Simplified the markup as well as the CSS.

Speaking of, we need to make this work in Stark (and actually we should've done that first). I don't know what the battle plan is with regard to actual themes. A one-off implementation for Seven obviously doesn't fly. It's not easy to imagine how this approach can work in a generic fashion, as it is not based on CSS only, but involves changes to the actual form + markup.

Status:Needs review» Needs work

The last submitted patch, drupal8.entity-meta.52.patch, failed testing.

@sun I agree about ids in selectors. The previous CSS in this patch is not a paragon of clean modularization or front-end best practices. However, if we’re really going to be rigorous, selectors like 'fieldset.flat' and '.entity-meta .meta .published' aren't ideal either, since they add too much specificity, risk cross-contamination, for example from other uses of both `.meta` and `.published` and are unpredictable (I can’t look at the markup and know that '.flat' will do different things depending on the element it’s applied to).

Also the visual styling in #52 is a regression, with aesthetic issues and broken styles for open/close states (open and close a number of combinations of details, you’ll see some open ones look as if they’re still closed).

@everyone I’m just frustrated. I’ve been trying my damnedest to implement this (#1510532: [META] Implement the new create content page design) for… wow more than 6 months now, including implementing it twice along the way (with help here and there from all of you, for which I am very grateful). But geez… I’m running out of steam here. I guess what I’m asking is that we wrap up this project one way or another.

Status:Needs work» Needs review
Issue tags:-needs accessibility review, -#d8ux

#52: drupal8.entity-meta.52.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+needs accessibility review, +#d8ux

The last submitted patch, drupal8.entity-meta.52.patch, failed testing.

StatusFileSize
new7.03 KB
new14.51 KB
PASSED: [[SimpleTest]]: [MySQL] 50,764 pass(es).
[ View ]

I can understand the approach, attached patch fixes the tests +

  • Do not show 'Published' or 'Unpublished' when there's no Node ID, really makes no sense
  • Removes node.js

Should be green.

Status:Needs work» Needs review

Well, hrm... the suggested removal of node.js is nice, but that actually circles back into the major problem I mentioned already:

The proposed visual design is really very specific and oriented towards interpreting nodes as a content publisher facility, so it is changing the entire interface to focus on that concrete case. The two column layout (which has to be styled like this or it fails to work) reminds a lot of wiki system interfaces and also the Wordpress blogging interface.

The design is strongly centered around a administrative back-end vs. public front-end separation, typical for a few publishing scenarios.

So essentially, where this is going to fail is in all the cases in which these strong design assumptions don't hold true — when there is no backend/frontend separation, especially in terms of audience. E.g., when nodes are used as forum topics. Or as other community content. I.e., whenever you move away from the pure editorial use-case that this design is centered around.

I've built plenty of sites in which nodes were not used in an editorial context, and just simply as content (most often, community-contributed). Users of these sites aren't supposed to see publishing info, revisioning details, and whatnot - just content. This new visual design would force me to implement a completely custom entity type and whatnot in D8 to get rid of those editorial design assumptions - or at least, to alter the heck out of it in order to get back to a sane + simple node/content page.

In essence, the same story as usual - Drupal core making too many assumptions that are hard to undo. That's especially concerning here, since we're talking about one of the most fundamental, most generic, and most used facilities in Drupal: Nodes.

Please don't get me wrong, I'm not criticizing the proposed design itself. Instead, my concerns are the amount of use-case specific assumptions and the consequences of this change.

So to get back to the removal of node.js — I don't think we want to do that, because it literally disallows everyone to change the design back to vertical tabs. Depending on the specific use-case of your site, the vertical tabs can present a significantly more appropriate user experience than the editorial sidebar column, which might very well simply don't match your needs.

However, reverting the removal of node.js won't really be sufficient, since we're changing the underlying markup to no longer work in a generic way, but instead to work for a very specific, particular visual design implementation.

Which ultimately boils down to: The current built-in interface is generic, you can specialize it for your use-case. This patch makes the built-in specialized, making it impossible to go back to generic. (What's hard for some, is impossible for others.)

All in all, I really wonder whether it wouldn't be better to approach this direction within Seven theme only first; i.e., by implementing seven_form_node_form_alter(). — And from there, try to find ways to make it more generic.

Perhaps one way to think of this is that D8 core is providing a node entity which is explicitly designed for holding content. That's why it has a form that is optimized for content. Other entities provided by contrib or custom can provide own optimized form. We've special-cased node for a while and this provides good justification for doing so. We can go further and say that Seven is optimizing for the same reason. I'm fine with that too.

Yay, green patch! Apologies all around for being cranky in #57. I will pitch in to get the styles for details states sorted out.

#62 seems totally sensible, as does #63. I don't know enough about the relevant issues, so I'll go along with those who can advise.

@sun Is there a way we can make it more flexible, so that use cases where the "node form" is not used primarily as content delivery tool. You can easily adjust it? I feel going back to Vertical tabs for those usecases, should almost be like flipping a switch in your theme template, or perhaps even content type settings (although I'd favor, no UI settings in Drupal at all).

It seems like your concerns are mostly around making this a flexible implementation, so our assumptions which are probably accurate for ordinary content creation do not interfere when you are doing non-ordinary content creation for other purposes; e.g. forums, products, media - where indeed a sidebar might not make much sense.

by implementing seven_form_node_form_alter()

Makes sense to me, I'll patch that up next week, unless someone beats me to it this weekend.

StatusFileSize
new3.68 KB
new14.89 KB
PASSED: [[SimpleTest]]: [MySQL] 49,622 pass(es).
[ View ]

Fixed CSS for the details states and made the CSS hooks for the meta header/summary area a little more robust.

by implementing seven_form_node_form_alter()

Can we use the layout system to do the column layout if this is theme-dependent? The next step after this patch is #1838156: Implement the two-column layout for the new create content page, which right now is halfway implemented but is waiting on this to go in.

Can we use the layout system to do the column layout if this is theme-dependent?

That's going to be a tricky, nearly a no go as the layout system has no conditions support atm - and won't have very soon too I'm afraid. I'm on that issue next week as well and investigate some more.

I'd agree that "swappable and pluggable form layouts" would be the ideal implementation. But as @swentel mentioned, layouts/displays are still in their early stages + parallel work on fields/fieldgroups/form-builder isn't there yet. Thus, I wouldn't rely and wait on that.

After hacking on this patch myself I understand it much better, and would summarize the problem space like this:

  • For editorial content types, we want to provide an editing interface that's optimized for editorial content.
  • We do not want to force the interface for editorial content onto all content types, because it is not necessarily applicable for e.g. forum posts.
  • Depending on whether the editorial interface applies or not, the form should be rendered with slightly different markup. The form structure should remain the same, so as to not break hook_form_alter() implementations.

Whether the editorial interface is suitable depends business logic, which can be assumed for some built-in content, but is unknown for custom content; i.e.:

  • Modules + installation profiles that ship with predefined content types should be able to apply a sane default interface.
  • For custom content types, a default cannot be assumed, since the use-case is unknown.

I think we should do the following:

  1. Get #1856178: '#group' Form API property only works with <details> elements done, so elements other than details can be part of a wrapping container/column (via #group).
  2. Introduce a new #type 'entity_meta_sidebar' [insert name bikeshed here]. Essentially, a formal UI element #type that shares semantics with #type 'vertical_tabs'. The two #types can be swapped with each other.
  3. Add a configuration option for node/content types, which essentially controls the value of #type of the wrapping container/column under the hood:

    The value of this selection option is the actual, internal #type name; i.e., either 'vertical_tabs' or 'entity_meta_sidebar'. Contrib can introduce new container types, and even swap out the existing.

    We could even consider to add a third option of 'container' here, which apparently would render just the details elements with no vertical tabs and sidebar; i.e., essentially the D6 editing interface of a simple list of collapsible details (formerly fieldsets).

  4. Only output the additional first entity meta info container if the wrapping #type is 'entity_meta_sidebar' (via #access).

This plan of attack would resolve the problem of making too many assumptions, while keeping the implementation simple.

It would also not special-case this entire nice editorial sidebar interface for Seven theme only, which is important IMHO, because I think we want to introduce this interface concept as an official pattern that "ought" to be implemented by contributed and custom themes, in order to improve the editing interface user experience for editorial content.

As a result, this will also work in Stark, and in turn, that means it is inherently tested by more people, which means that chances of mistakenly introducing regressions for the editorial interface by another patch/change are significantly reduced.

Good to see this discussed in depth. But I would like to see one thing clarified then:

In what way is the current (with vertical tabs at the bottom) UI *a better* default for non-editorial (as you call it) content? I want to understand why having vertical tabs at the bottom of a node form means making less assumptions and thus makes it more applicable to other types of content.

UI-wise I don't see how having vertical tabs at the bottom implies making less assumptions. Is it the amount or structure of the output?

I ask because I think we have to carefully weigh how much harder it will be to undo/override the proposed new UI compared to the current situation before we add yet another setting somehwere in the admin.

We (Kevin, Roy, Bojhan, dcmistry & a bit of webchick) discussed this during UX hours in IRC. Consensus is still that we all think the sidebar is the better default for node content.

Can we defer the discussion to wether the sidebar should be the default for *any* entity creation form in a different issue and continue as planned in here?

So that would mean we do #70 1., 2. and perhaps 4., but not 3. (and perhaps discuss 3. in a follow-up). That would mean the sidebar as depicted here would show for all content types, but it would be really easy to swap that out from a module (but not from the UI! (at least not for now)). @sun, would you be on-board with that?

StatusFileSize
new29.56 KB
new8.24 KB
new15.69 KB
FAILED: [[SimpleTest]]: [MySQL] 44,938 pass(es), 2,978 fail(s), and 1,337 exception(s).
[ View ]

I would try and keep focus on the original issue and discuss 2 and 3 in the two column issue, we have less comments there, so we can bikeshed like hell ;)

Updated patch:

  • It assumes you have #1856178: '#group' Form API property only works with <details> elements also installed. The patch over there now also supports fieldsets, and has tests for vertical tabs, container, details and fieldsets, so let's get that in asap.
  • Switches between vertical_tabs or container depending on whether
    1. the current user has access to view the administration theme.
    2. and the current path is an admin path.
    3. and the node/add / edit pages are configured to be represented in the administration theme.

    This is a different approach and makes it 'administration (theme)' dependant, not '#type'. Basically : details (and then soon 2 column) in admin, vertical_tabs in 'frontend'. This should solve most objections that sun had imo.

  • Keep revision_information as a separate element and move it into the right #group (sometimes container, sometimes details) so that when it switches back to vertical tabs, it is actually in there. sun, check how cool it is that we can play with #group now :)
  • Adds isNewRevision() to Node.php as there was a logic error re: revisions - try logging in as 'normal' user and creating a forum topic.
  • Renames 'additional_settings' to 'meta'.
  • Added a border-top for now on the meta, so it looks 'nicer' and actually non blocking to commit.

Screen Shot 2013-01-15 at 17.41.36.png

Status:Needs review» Needs work

The last submitted patch, 1838114-74.patch, failed testing.

@swentel Looking good! Happy to see the #group pach is now RTBC and working well here.

Status:Needs work» Needs review
StatusFileSize
new15.67 KB
FAILED: [[SimpleTest]]: [MySQL] 44,892 pass(es), 2,953 fail(s), and 1,172 exception(s).
[ View ]

Reroll after #1856178: '#group' Form API property only works with <details> elements is in, so should be green. Slightly optimized the logic for hiding the 'meta' block when we're not in administration mode.

Status:Needs review» Needs work

The last submitted patch, 1838114-77.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new14.67 KB
PASSED: [[SimpleTest]]: [MySQL] 50,714 pass(es).
[ View ]

Bah, left out the fix re: isNewRevision() to Node.php, we can always tackle that afterwards.

Title:Change node form’s vertical tabs into "advanced" column holding meta info and detailsChange node form’s vertical tabs into details elements
Status:Needs review» Reviewed & tested by the community

Code looks good. The node form actually looks a bit uglier with this patch as we really need #1838156: Implement the two-column layout for the new create content page to fix it all up. swentel plans to work on that tomorrow.

Status:Reviewed & tested by the community» Fixed

Great intermediate step! Committed to 8.x. Hopefully the two column layout follows shortly after as that will make this look pretty again. :)

Thanks swentel, ry5n and sun!

Status:Fixed» Needs work

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.php
@@ -109,20 +123,47 @@ public function form(array $form, array &$form_state, EntityInterface $node) {
+      '#type' => 'fieldset',
+      '#attributes' => array('class' => array('entity-meta-header')),
+      '#type' => 'container',

Can we get a quick follow-up to remove the double #type? That just melted my brain... :-) Thx!

Status:Needs work» Fixed

Sure. Don't want to hold anything up. :-)

Yay! Just want to say thanks to everyone who helped with this. So happy to see this in :)

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Category:feature» bug
Status:Closed (fixed)» Active

This part of the committed code does not work like it should:

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.php
@@ -56,6 +56,20 @@ protected function prepareEntity(EntityInterface $node) {
   public function form(array $form, array &$form_state, EntityInterface $node) {
+
+    // Visual representation of the node content form depends on following
+    // parameters:
+    // - the current user has access to view the administration theme.
+    // - the current path is an admin path.
+    // - the node/add / edit pages are configured to be represented in the
+    //   administration theme.
+    $container_type = 'vertical_tabs';
+    $request = drupal_container()->get('request');
+    $path = $request->attributes->get('system_path');
+    if (user_access('view the administration theme') && path_is_admin($path)) {
+      $container_type = 'container';
+    }

You're not able to trigger the non-vertical-tabs presentation, unless you have the obscure checkbox in Appearance settings enabled that says:

"Use the administration theme when editing or creating content"

In short:

1) Install Minimal.
2) Enable Seven theme.
3) Go to a node/add or node/edit page.

People are reporting bugs against other patches, because they are getting an entirely different user interface.

Status:Active» Needs review
StatusFileSize
new5.25 KB
PASSED: [[SimpleTest]]: [MySQL] 49,175 pass(es).
[ View ]

How about this: use seven_form_alter(). I was already thinking todo that anyway in the two column patch. This removes lines from NodeFormController::form that I already felt a bit annoyed by anyway.

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new3.61 KB
new5.06 KB
PASSED: [[SimpleTest]]: [MySQL] 49,122 pass(es).
[ View ]

Works for me. As long as we don't have form displays in core, that's at least a little bit more predictable.

Merely cleaned up some minor things.

And replaced the @todo with a separate issue: #1906208: Independent .status HTML class triggers message/icon styling

StatusFileSize
new566 bytes
new5.08 KB
PASSED: [[SimpleTest]]: [MySQL] 49,115 pass(es).
[ View ]

And before anyone complains... ;)

Works perfect, thanks!

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

From #79:

+++ b/core/includes/form.inc
@@ -4670,12 +4675,18 @@ function theme_form_element($variables) {
+  // Take over any #wrapper_attributes defined by the element.
+  // @todo Temporary hack for #type 'item'.
+  // @see http://drupal.org/node/1829202
+  if (isset($element['#wrapper_attributes'])) {
+    $attributes = $element['#wrapper_attributes'];
+  }
   // Add element #id for #type 'item'.

I like that this "temporary hack" allows for adding attributes to form element containers without the need for extra wrapper elements ala #prefix/#suffix and without needing to override theme_form_element() just to customize the attributes of form element containers. So +1 for leaving this temporary hack in.

Any reason why we shouldn't keep this #wrapper_attributes support in place (and remove the @todo)?

Issue summary:View changes

Updated issue summary.