In the Node edit sidebar, we shouldn't be using a label where there should just be markup. Labels need to be associated with form elements. This should be a heading styled to look like the rest of the labels in order to be semantically correct:

      <label for="edit-changed--2">Last saved</label>
      Not saved yet
      </div>
<div id="edit-author--2" class="author container-inline form-item form-type-item form-item-author">
      <label for="edit-author--2">Author</label>
      root
      </div>

should be:

      <h4 class="label">Last saved</h4>
      Not saved yet
      </div>
<div id="edit-author--2" class="author container-inline form-item form-type-item form-item-author">
      <h4 class="label">Author</h4>
      root
      </div>

results of WAVE toolbar

CommentFileSizeAuthor
#33 2249089-label-author-last-saved-33.patch1.99 KBmgifford
#30 Screen Shot 2014-06-06 at 22.56.26.png28.24 KBlauriii
#30 Screen Shot 2014-06-06 at 22.56.14.png27.64 KBlauriii
#29 2249089-label-author-last-saved-29.patch1.99 KBmgifford
#27 2249089-label-author-last-saved-27.patch1.99 KBchaquea
#27 h4-with-display-inline.jpg55.31 KBchaquea
#20 interdiff-2249089-1-20.txt940 bytesmgifford
#20 2249089-label-author-last-saved-20.patch1.98 KBmgifford
#6 corrected-last-saved-author.png189.59 KBmgifford
#4 2249089-label-author-last-saved-1.patch2.05 KBmgifford
improper-use-of-labels.png42.28 KBmgifford

Comments

mgifford’s picture

mgifford
he/him
Primary language English
commented
Issue summary: View changes
mgifford’s picture

mgifford
he/him
Primary language English
commented
Issue summary: View changes
mgifford’s picture

mgifford
he/him
Primary language English
commented
Issue summary: View changes
mgifford’s picture

mgifford
he/him
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new 2249089-label-author-last-saved-1.patch2.05 KB

Ok.. I think this works.

EDIT: 2 Little CSS Notes:

I don't think there was any way to do this:

+h4.label.inline {
+  display: inline;
+  padding-left: 0;
+}

Also, this should apply to things styled like labels too.

 /**
  * Inline items.
  */
-.container-inline label:after {
+.container-inline label:after,
+.container-inline h4.label:after {
   content: ':';
 }
mgifford’s picture

mgifford
he/him
Primary language English
commented
Issue tags: +Accessibility
mgifford’s picture

mgifford
he/him
Primary language English
commented
StatusFileSize
new corrected-last-saved-author.png189.59 KB

Here's a view with the patch.
Corrected with patch

lewisnyman’s picture

lewisnyman
he/him
commented
Status: Needs review » Needs work 
+++ b/core/modules/system/css/system.theme.css
@@ -70,6 +70,10 @@ h4.label {
+h4.label.inline {

@@ -105,7 +109,8 @@ abbr.form-required, abbr.tabledrag-changed, abbr.ajax-changed {
+.container-inline h4.label:after {

Just one thing, can we remove the h4? We shouldn't need it.

mgifford’s picture

mgifford
he/him
Primary language English
commented

There is a precedent for h4.label, should we just have a generic label CSS class?

It should be at least <strong> which has some semantic meaning.

lokapujya’s picture

lokapujya
Primary language English
Location Boston
commented

Are "Last Saved" and "Author" just read-only form items with a label?

mgifford’s picture

mgifford
he/him
Primary language English
commented

Yup. Jut read only. No need for a label as there is no associated form element. Just markup and styling.

lokapujya’s picture

lokapujya
Primary language English
Location Boston
commented

Maybe "Last Saved" and "Author" are form elements?

lokapujya’s picture

lokapujya
Primary language English
Location Boston
commented 
+++ b/core/themes/seven/seven.theme
@@ -340,14 +341,12 @@ function seven_form_node_form_alter(&$form, &$form_state) {
       '#type' => 'item',

I mean, it is still #type=item.

lewisnyman’s picture

lewisnyman
he/him
commented

There is a precedent for h4.label, should we just have a generic label CSS class?

Yeah, let's create a reusable .label class for any element.

mgifford’s picture

mgifford
he/him
Primary language English
commented

@lokapujya I'm going to have to open up a new issue about the Forms API I think:
https://api.drupal.org/api/drupal/developer!topics!forms_api_reference.h...

This example will produce a Label without an a related text input. It's a bad example. Whether we need #type=item or not I don't know.

We'd need to insert some line breaks though if we were to just remove the item line. It works, but produces:

<h4 class="label inline">Last saved</h4> 04/28/2014 - 13:32<h4 class="label inline">Author</h4> root<div id="edit-revision-information" class="node-form-revision-information form-wrapper"><div class="form-item form-type-checkbox form-item-revision">

@LewisNyman would that just be changing h4.label to:

.label {
  font-size: inherit;
  font-weight: bold;
  margin: 0;
  padding: 0;
}

That would eliminate the need for the .label.inline too.

lokapujya’s picture

lokapujya
Primary language English
Location Boston
commented

I noticed the items such as "Menu Settings" are not blue in Chrome. Maybe we should open another issue for that. I was going to fix it with CSS, but it actually seems like the markup is different. Not sure how using Chrome changes the markup.

lokapujya’s picture

lokapujya
Primary language English
Location Boston
commented

@14 What I am asking is that "Last Saved" and "Author" are actually form elements (just displayed as read only here) and so: would label actually be semantically correct?

mgifford’s picture

mgifford
he/him
Primary language English
commented

Regarding the menu settings in Chrome, ya would make sense to make a new issue for that.

It would be fine if we were using the readonly attribute <input type="text" readonly="readonly" value="myValue"> but that doesn't seem to be getting into Core any time soon #500868: Forms API is missing the readonly form field option.

Otherwise it shouldn't have a label.

lewisnyman’s picture

lewisnyman
he/him
commented
Issue tags: +frontend, +html
lewisnyman’s picture

lewisnyman
he/him
commented

@mgifford Sorry I never got back to you, yep. It would be great if we could kill two birds with one stone.

mgifford’s picture

mgifford
he/him
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new 2249089-label-author-last-saved-20.patch1.98 KB
new interdiff-2249089-1-20.txt940 bytes

How's this as a generalized .label version of this patch?

Status: Needs review » Needs work

The last submitted patch, 20: 2249089-label-author-last-saved-20.patch, failed testing.

mgifford’s picture

mgifford
he/him
Primary language English
commented

20: 2249089-label-author-last-saved-20.patch queued for re-testing.

The last submitted patch, 20: 2249089-label-author-last-saved-20.patch, failed testing.

lewisnyman’s picture

lewisnyman
he/him
commented

20: 2249089-label-author-last-saved-20.patch queued for re-testing.

crasx’s picture

crasx commented

@mgifford that looks good to me!

however the h4's should float left and maybe have some right padding to match current designs

also, is this the correct spot for a header? should it be strong instead?

chaquea’s picture

chaquea commented
Assigned: Unassigned » chaquea

Looks good, will just make it display inline so it looks the same as the previous label appearance.

chaquea’s picture

chaquea commented
Assigned: chaquea » Unassigned
Status: Needs work » Needs review
StatusFileSize
new h4-with-display-inline.jpg55.31 KB
new 2249089-label-author-last-saved-27.patch1.99 KB

Added the inline style, attaching an image of the result.

lewisnyman’s picture

lewisnyman
he/him
commented
Status: Needs review » Needs work

Thanks. I found a minor problem related to coding standards:

+++ b/core/modules/system/css/system.theme.css
@@ -64,7 +64,8 @@ label.option {
+	display:inline;

This property is incorrectly indented, it should be two spaces as per our coding standards.

mgifford’s picture

mgifford
he/him
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new 2249089-label-author-last-saved-29.patch1.99 KB

I removed the tab...

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new Screen Shot 2014-06-06 at 22.56.14.png27.64 KB
new Screen Shot 2014-06-06 at 22.56.26.png28.24 KB

Tested this manually and this seems to work.

Here's pictures of this:

Before:
Before
After:
After

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 29: 2249089-label-author-last-saved-29.patch, failed testing.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Issue tags: +Needs reroll
mgifford’s picture

mgifford
he/him
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new 2249089-label-author-last-saved-33.patch1.99 KB

this should be able to be set back to rtbc when green. It's just a reroll. I think for spacing.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll

Tested this and seems to work as expected.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Hey, folks. I'm really sorry about the delay on this. My post-DrupalCon life got a bit hectic. :(

Looks like this issue is specific to Seven and the manipulations it does to the node form, so this fix should be sufficient.

Committed and pushed to 8.x. Thanks!

  • webchick committed 72c229e on 8.x 
    Issue #2249089 by ivan.chaquea, mgifford: Fixed Improper use of a LABEL...
mgifford’s picture

mgifford
he/him
Primary language English
commented

Thanks @webchick. Life sometimes does that. Happy to have you back.

Status: Fixed » Closed (fixed)

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