Remove theme_form_required_marker() from the theme system - use CSS instead

Split from form.inc twig conversions. Also rewritten to remove building the attributes in preprocess, it's wasteful.

Here's a completely new approach. Replacing this with CSS as it's presentational and not content. No more theme_ function. Inline with what is proposed for the tablesort indicator. #1973418: Remove tablesort-indicator.html.twig, use CSS instead #855332: Make Table Column Sort Arrows Themeable #1986400: Table style update

On the whole I'm fine with these changes. They won't impact non-visual accessibility.

This just excludes checkboxes and radios labels with :not(). We could also use direct child selector > if :not() is a no go.

This is brilliant. Something as tiny as a required marker never should have had a theme function in the first place, and certainly doesn't need a whole template file. Doing this with CSS is a great idea, +1

This should fix the tests, disregard my note about descendant selectors, that won't work for datetime because it has another wrapper, the :not() shall rule.

This should fix the tests with a pinch of xpath and dab of preg_match.

This would be a nice change, CSS is always more visible to front-enders instead of digging around for templates. I thought the main reason why we used mark up is so we could provide a meaningful title attribute for screen readers. Tagging for accessibility review.

Re #11 see #6, it's aria-hidden and I added speak:none to the CSS. The other aria-required attribute I believe is on the label and/or input. hope that covers it?

10: 2152217-twig-theme_form_required_marker-10.patch queued for re-testing.

Re-rolled from the theme() to drupal_render() stuff that got in last night.

Removed form-required-marker.html.twig, cleared cache and still all looks good.

looks very good to me :)

It certainly simplifies things. I'll look at getting testing on this issue with a screen reader.

1. +++ b/core/includes/form.inc
   @@ -2866,7 +2843,7 @@ function theme_form_element_label($variables) {
   +  return '<label' . new Attribute($attributes) . '>' . t('!title', array('!title' => $title)) . '</label>';
   
   +++ b/core/modules/field/field.form.inc
   @@ -26,14 +26,12 @@ function theme_field_multiple_value_form($variables) {
   +        'data' => '<h4 class="label">' . t('!title', array('!title' => $element['#title'])) . "</h4>",
   

   All of these t() instances are obsolete with this change proposal — they only existed to make the required marker translatable, so that it appears left to the label in some Right-To-Left languages.

   This aspect needs to be resembled via -rtl styles now, which does not appear to be part of the patch yet.

2. +++ b/core/modules/system/css/system.theme.css
   @@ -77,13 +77,23 @@ h4.label {
   +.form-required label:not(.option):after,
   +.form-required .label:after {
   

   These should be limited to immediate children of .form-required to prevent false-positive matches.

   Wondering whether we can find a better solution for excluding #type radio and #type checkbox... — for example, could we skip the .form-required class on the form element for those two types in template_preprocess_form_element() already?

3. +++ b/core/modules/system/css/system.theme.css
   @@ -77,13 +77,23 @@ h4.label {
   +  speak: none;
   

   Hm. This might need a review from the accessibility team — the previously existing abbr.form-required contained an invisible explanation that a form control is required.

   Technically, that should be covered by the 'required' property + aria-required attribute already, but I'm not sure.

4. +++ b/core/modules/system/css/system.theme.css
   @@ -77,13 +77,23 @@ h4.label {
   +/*
   +Not sure how to deal with this...
   +abbr.form-required, */
   

   This should be obsolete, since you are removing the HTML markup, no?

   Or what's the remaining todo, what are you not sure "how to deal with"?

This might need a review from the accessibility team — the previously existing abbr.form-required contained an invisible explanation that a form control is required.

Technically, that should be covered by the 'required' property + aria-required attribute already, but I'm not sure.

The required="required" attribute is sufficient to trigger the assistive user agent to report the field being required. The text is redundant.

Re: #20

Thanks for the review @sun. Here's the responses.

1. All of these t() instances are obsolete with this change proposal — they only existed to make the required marker translatable, so that it appears left to the label in some Right-To-Left languages.

   Great point I didn't think of that, should be fairly easy to accommodate, is it just :before vs :after for the rtl ltr?

2. These should be limited to immediate children of .form-required to prevent false-positive matches.

   I tried immediate children but it gave more false positives than the :not() because some had double wrappers on 'datetime'. @see #10

3. Hm. This might need a review from the accessibility team — the previously existing abbr.form-required contained an invisible explanation that a form control is required.

   I think mgifford is on that speak:none case. Though I believe it is correct conversion of 'aria-hidden' => 'true' to CSS. And what @jessebeach said in #22.

4. This should be obsolete, since you are removing the HTML markup, no?

   Yeah you are right, should probably just remove that too.

Re-rolled. Adding rtl ltr, and fieldset-legend required.

From 2010, this approach doesn't look like it would be sufficient:
http://simplyaccessible.com/article/required-fields-right/

It looks better in 2012 & 2013:
http://john.foliot.ca/required-inputs/
http://developer.yahoo.com/blogs/ydn/define-required-inputs-aria-html5-5...
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Tec...
http://usability.com.au/2013/05/accessible-forms-2-required-fields-and-e...

Let me reach out to a few folks about this.

The color is missing from this patch. It should be red. Easy to add to the patch. It might also be useful to add more space.

I tried the rtl out and I'm kinda thinking that is not needed for this, but would love someone who reads a right-to-left language to let us know.

Added some space in thanks for the review @mgifford, does that space work for you?

Fixed the CSS typos and such.

The test failure above looks random and had no effect when tested locally.

Yes, it is possible that the RTL styles are not necessary — I'm not sure how browsers are treating pseudo :before CSS styles in case of [dir="rtl"].

You can actually test this yourself very easily — just install Language module, add a new custom language with code "rtl" and name "RTL" and select the RTL option. When you switch to that language, you see the entire output still in English, but in RTL.

When doing so, screenshots would be nice.

Aside from that, I'm still concerned about the unlimited inheritance of the .form-required label selector, as well as the second :not(.option) selector, because .option does not actually imply the meaning you're shooting for — its only meaning is that the label ought to be styled to appear after the input element (instead of above), which can be applied to whichever element you want; it does not strictly imply that the element is a radio or a checkbox. I can see two options:

1. Either we make .form-required > label work; resolving the datetime element problem somehow.
2. Or let's simply add a class to the LABEL element itself, so that the CSS can simply target label.required, .label.required, legend.required.

I'd personally prefer option 2) — I know this ought to be resolvable via CSS selectors, but as long as edge-cases like that datetime markup exist, we should simply resort to classes. 2) will also make the CSS selectors much more simple.

Btw, I also think you're missing .form-required legend here?

+++ b/core/modules/field/field.form.inc
@@ -26,14 +26,12 @@ function theme_field_multiple_value_form($variables) {
+        'data' => '<h4 class="label">' . t('!title', array('!title' => $element['#title'])) . "</h4>",

t() is also obsolete here.

Thanks again for the review.

The reason I put the class on the containing element was to provide flexibility to how the required indication is presented by CSS. If I target the label/legend directly with a class. I couldn't easily let a themer put a required red right border on a input/textarea for example. Moving the class up to the wrapper provides this sort of flexibility. Though I'm probably trying to solve too many problems at once so I'll try the label class idea.

Thanks for the note on the theme_field_multiple_value_form, I've fixed that one.

Removed the rtl for now, here's the screenshot before and after rtl.

LTR

RTL

Totally, thanks @Cottser.

Thanks! I think that looks much cleaner now.

The reason I put the class on the containing element was to provide flexibility to how the required indication is presented by CSS. If I target the label/legend directly with a class. I couldn't easily let a themer put a required red right border on a input/textarea for example. Moving the class up to the wrapper provides this sort of flexibility. Though I'm probably trying to solve too many problems at once so I'll try the label class idea.

I think you're making sense this might actually be required for #1493324: Inline form errors for accessibility and UX

To resolve that potential use-case, we could simply retain the .form-required class on the wrapping element (as in HEAD), in addition to setting the .required class on label | .label | .fieldgroup > legend > span ?

FWIW, I think that a class name of just .required (sans form- prefix) might be acceptable in this case.

a class name of just .required (sans form- prefix) might be acceptable in this case

Or alternatively, just simply .label-required — so that you can still uniquely query for this class without having to use more specific selectors and avoiding false-positive clashes.

Assigning to tackle #36/37.

Would it be correct to use the ‘state’ category in the CSS standards? So .is-required.

State selectors should only ever be combined with another selector, which avoids any false positive clashes.

Starting to work on that.

Reroll made... hope it works ok!

keep working

I have changed the patch and now it passes the test locally.

last patch was empty :(

keep working

Here is the patch.

48: 2152217-twig-theme_form_required_marker-48.patch queued for re-testing.

I've not gotten around to tackling @sun's notes in #36/37 and @LewisNyman's notes in #39 so anybody is welcome to jump in here. Thank you very much for the reroll/bump @jjcarrion!

I think this should do the trick. Came down to:

89c89
<    $title = filter_xss_admin($element['#title']);
---
>    $title = Xss::filterAdmin($element['#title']);

Another reroll...

1 fail is date test is valid as it's looking for the required marker which has been removed by this patch, not sure how to check the presence of the marker as it is coming from CSS now. Also would be great to add some test cases (if possible).

+.form-required:after {
+  content: "*";
+  margin-left: 0.25em;
+  margin-right: 0.25em; /* RTL */
+}

I wonder whether simply adding a space before and after the asterisk would work, too? (or are the 0.25em extraordinarily polished? :-))

@sun they are not polished, those are rough approx of an en-space. So yeah we can just put that on the * and remove a couple properties.

Fixed test fail at #56 and the styling issue mentioned #59 and manually tested. Both space and margin looks the same (attached results).

Thanks @vijaycs85

Here's some more screenshots for manual testing.
I think this is good to go:

• We've removed the function that creates a span and a * with CSS for presentational elements that should make theming how these required marker is styled.
• We've accounted for aria-hidden with speak: none.
• Fixed up the tests to find the classes.
• The JS has to do a few less dom searches with the removal of the .find() call.
• One less micro twig template/theme function provides faster rendering/less theme system processing
• All win?

Updating commit message...

Awesome. I really like this improvement now. Great work, everyone :-)

We've accounted for aria-hidden with speak: none.

I'm so sorry to do this, but this is the fundamental reason for the change. The speak property isn't respected by screen readers. If you test the patch in #61 with a screen reader, you get the following announcement on the Title field of an article add form.

"Title star"

The reason for making this change is to not have the screen reader read the star as if it is part of the label.

We could use a small star SVG as a background image to achieve the same effect and then it won't get read out. In fact, we could probably just use a base64 encoded background image here because it will be super tiny.

And I don't mean to diminish the great work that has been done in this issue, as sun noted. I truly want to make sure it solves the issue :)

@jessebeach thanks for testing that, that is a real bummer that the screen reader isn't respecting that speak: none; value:(

With an image that will be annoying to change the color, though we could do that, I'd rather stick to my guns on this one a bit more...

Is there any references that explain why speak: none; wouldn't work or doesn't work?

Thanks for finding that @jessebeach!

Could we not just use a less common UTF8 character like one of these stars https://en.wikipedia.org/wiki/Star_%28glyph%29

⁕ U+2055

This one isn't bad, I wonder how that would work with the reader?

Sure, that'll work. The character doesn't get read out.

.form-required:after {
  color: #e00;
  content: " \2055 ";
  font-size: 1em;
  line-height: 1;
  vertical-align: text-top;
  font-weight: 400;
}

@jessebeach is there any way I can test this without buying Jaws?

I do own OS X GhostReader(Long documents are easier to retain when listening and reading to them at the same time) and it ignores ⁕ and speaks * as asterisk.

There is also U+2217 ∗ asterisk operator. http://en.wikipedia.org/wiki/Asterisk
Or even closer "low asterisk" U+204E ⁎

Thanks again for testing @jessebeach! I'd like to leave the speak: none; with maybe a comment about it's usefulness for posterity.

To show the differences:

time:before {
    color: #EE0000;
    content: " * ";
}

time:after {
    color: #EE0000;
    content: " ⁕∗⁎";
    font-size: 1em;
    line-height: 1;
    vertical-align: top;
}

joelpittet, if you post a patch with the change, I'll put this back at RTBC :)

@joelpittet - "is there any way I can test this without buying Jaws?"

VoiceOver is pretty good if you have a Mac.

On Windows you have a choice of:
http://www.chromevox.com/
http://www.nvaccess.org/

They are all a bit different sadly. JAWS is still the most powerful.

Here's a new one with the super lower astrix .

I tested the latest patch with VoiceOver and there's just a pause now which is fine.

Would have been fun to have a more unique asterisk ✼ but no big deal.

Here it is really big:

Also the superscript is good vertical-align: super;.

Sorry, not working for Right to left :(

Damn.. Thanks for catching that. I was also wondering if we should use ::after instead of :after https://developer.mozilla.org/en-US/docs/Web/CSS/::after

Wondering too if we should get rid of: speak: none;

It isn't needed and isn't well supported.

@mgifford I think a fancy character would be nice, but I just want to get this through then make it cool, so I'd +1 an issue for that flower character if you opened a followup?

I'd really like to keep that speak: none; in there as I know icon fonts use it and I'm kinda crossing my fingers that more browsers and readers will too. I think opera does take it into account.
@see
http://css-tricks.com/html-for-icon-font-usage/
https://github.com/FortAwesome/Font-Awesome/blob/master/src/3.2.1/assets...
http://www.opera.com/docs/specs/presto23/css/properties/
http://lab.dotjay.co.uk/notes/css/aural-speech/

Regarding the ::after vs :after. I'd like to leave it as :after until something is set in stone so there is consistency when moving to CSS3. Also, if that is what we want to strive through it should be written on this stone ;) https://drupal.org/node/1887918

@vijaycs85, Any hints at why the unicode character isn't showing up in that language? Can you hook me up with what you did to make the UI show up in that language?

Just checked using Persian/Farsi RTL language and seems to work correctly even in the installer (check screenshots).

It's a RTBC for me :)

Quickly added a CSS comment directly above the content property to explain for posterity why we're not using a regular asterisk.

Otherwise, there's a good chance that someone will patch this to "fix the strange character" in the future ;)

@joelpittet I took more of a look at support for speak: none; and really, in 2014 it's essentially non-existent from what I can tell.

I don't know when Opera Presto 2.3 was out, but 2.12 no longer seems to support it http://www.opera.com/docs/specs/presto2.12/css/properties/

Opera 8 seems to have supported it, but we're at Opera 12 right now.

There's an outstanding issue in the FF Issue queue that's been open since 2000. I bumped it and started some others in the Chromium bug tracker too.

https://en.wikipedia.org/wiki/Comparison_of_layout_engines_%28Cascading_...

I couldn't get any of my browsers to display hopeful colors with speak or speak-as here http://css3test.com and that seems to be confirmed here http://www.cssportal.com/css-properties/speak.php

I tested it with ChromeVOX.

There are lots of good aural CSS2 features defined. But don't think any of them have been implemented.

The CSS 3 Speech Module article was last updated in 2010.

We can't start putting in CSS that isn't supported in any of our officially supported browsers. Even if it would be a nice feature to support.

I've taken @sun's patch & just removed speak: none;

84: drupal8.form-required.84.patch queued for re-testing.

Bad bot.. But here's the interdiff.

@mgifford ok, I just took it from the prospective that content from CSS should have these controls as aria-hidden attributes for HTML and have existed in the definition for a long while. And that even though there is no good support, there should be and with that hope that one day there will be.

I'm fine with not having it and can easily enough add it in as a bug fix if browsers start picking up support for it. I was adamant it be there for future support and "the right way™" according to me:P And it seems testbot agrees;)

84: drupal8.form-required.84.patch queued for re-testing.

RTBC even.

@joelpittet Thanks.. Absolutely, "content from CSS should have these controls". Hopefully the browsers will start speeding up their implementation of the guidelines from the W3C. At which point +1 to adding it back in. :)

Still applies... testbot back at you! 4 <3, your serve

84: drupal8.form-required.84.patch queued for re-testing.

NIce one moving this out of a theme function to CSS.

Committed/pushed to 8.x, thanks!

We found a bug that was found in testing: #1986418: Update textfield & textarea style

The required marker is not rendered in windows

This is on IE9 from re-testing it. And not on IE10. Thanks for spotting that, we'll need to find an alternative character or go back to the drawing board:(

Ok so the one character we chose is the only one out of all the stars that doesn't display on IE9:(

http://jsfiddle.net/7kxHd/
http://en.wikipedia.org/wiki/Asterisk

Maybe we can use one of the others? Thoughts @mgifford maybe one of the fancier ones like you suggested?

Pretty low odds on that.. Damn you IE!

How about: ✻

It's different enough, but still clearly an asterisk.

For me the low asterisk is not rendering in Chrome 35 on Windows 7. It appears fine in Firefox 29, Opera 12 and IE 11.

@mgifford I'm game for that.
@eft does that one render for your chrome?

If that is a "Teardrop-Spoked Asterisk" it does render in Chrome but it's a pretty large glyph.

@Cottser - yes, I'll do that and add link here. #2282819: The low asterisks in theme_form_required_marker() doesn't work in IE

@eft, yes it is the "Teardrop-Spoked Asterisk" I picked out, but it's all just css so we can control the size easily enough, right?

We overlooked one instance here; see #2301245: Entity system invokes non-existing theme hooks: "Theme hook $entity_type_id not found."

Minor tag cleanup - please ignore.

Minor tag cleanup - please ignore.