Task

Convert theme_form_required_marker() to a Twig template.

Remaining tasks

  • Patch
  • Patch review
  • Manual testing

Steps to test

@todo

Files: 
CommentFileSizeAuthor
#101 Win7_–_IE_9.png14.12 KBjoelpittet
#99 6-6-2014 2-31-46 PM.jpg17.07 KBLewisNyman
#87 interdiff-2152217-83-84.txt321 bytesmgifford
#84 drupal8.form-required.84.patch13.66 KBmgifford
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,606 pass(es). View
#83 interdiff.txt398 bytessun
#83 drupal8.form-required.83.patch13.68 KBsun
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,323 pass(es). View
#82 rtl-farsi-installer-required-fields.png583.21 KBrteijeiro
#82 rtl-farsi-required-fields.png90.94 KBrteijeiro
#79 Screen Shot 2014-05-03 at 19.00.26.png74.67 KBvijaycs85
#78 2152217-interdiff.txt356 bytesmgifford
#78 Asterisks-Zoom.png32.09 KBmgifford
#77 interdiff.txt590 bytesjoelpittet
#77 2152217-twig-theme_form_required_marker-76.patch13.6 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,321 pass(es). View
#74 Remove_theme_form_required_marker___from_the_theme_system_-_use_CSS_instead___2152217____Drupal_org.png7.68 KBjoelpittet
#62 install-rtl.png590.35 KBjoelpittet
#62 create-article-bottom.png95.42 KBjoelpittet
#62 create-article-top.png78.38 KBjoelpittet
#62 create-article-rtl.png67.42 KBjoelpittet
#61 2152217-required-marker-with-margin-61.png46.36 KBvijaycs85
#61 2152217-required-marker-with-space-61.png39.34 KBvijaycs85
#61 2152217-diff-56-61.txt1.37 KBvijaycs85
#61 2152217-theme_required_marker-61.patch13.55 KBvijaycs85
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,311 pass(es). View
#56 2152217-theme_required_marker-56.patch12.67 KBvijaycs85
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,693 pass(es), 1 fail(s), and 0 exception(s). View
#35 2152217-twig-theme_form_required_marker-35.patch12.94 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 64,760 pass(es). View
#35 interdiff.txt575 bytesjoelpittet
#33 Create_Article___Site-Install.png93.6 KBjoelpittet
#33 Create_Article___Site-Install-rtl.png74.64 KBjoelpittet
#33 interdiff.txt11.2 KBjoelpittet
#33 2152217-twig-theme_form_required_marker-33.patch12.9 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 64,719 pass(es). View
#29 2152217-twig-theme_form_required_marker-29.patch12.56 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 64,617 pass(es). View
#29 interdiff.txt1.35 KBjoelpittet
#28 Required-Missing-Color.png185.51 KBmgifford
#25 2152217-remove-required-marker-25.patch12.57 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 64,560 pass(es), 2 fail(s), and 1 exception(s). View
#25 interdiff.txt4.11 KBjoelpittet
#17 2152217-diff-15-17.txt553 bytesvijaycs85
#17 2152217-remove-required-marker-17.patch11.01 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 64,171 pass(es). View
#15 2152217-theme_form_required_marker-css-15.patch11.54 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 63,744 pass(es). View
#10 interdiff.txt3.14 KBjoelpittet
#10 2152217-twig-theme_form_required_marker-10.patch11.45 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2152217-twig-theme_form_required_marker-10.patch. Unable to apply patch. See the log in the details link for more information. View
#7 interdiff.txt1.03 KBjoelpittet
#7 2152217-twig-theme_form_required_marker-7.patch9.58 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 63,291 pass(es), 106 fail(s), and 0 exception(s). View
#5 2152217-twig-theme_form_required_marker-5.patch9.55 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2152217-twig-theme_form_required_marker-5.patch. Unable to apply patch. See the log in the details link for more information. View
#2 2152217-2-twig-theme_form_required_marker.patch2.55 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 59,388 pass(es). View

Comments

Cottser’s picture

Issue summary: View changes

Adding a commit message to the issue summary so the folks who already worked on #1898480: [meta] form.inc - Convert theme_ functions to Twig and in the Twig sandbox get credit.

joelpittet’s picture

Status: Active » Needs review
FileSize
2.55 KB
PASSED: [[SimpleTest]]: [MySQL] 59,388 pass(es). View

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

michamilz’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +#SprintWeekend2014

Twig file is used and tests passes after applying the patch.

Cottser’s picture

Status: Reviewed & tested by the community » Needs work

This one still needs some profiling. Thanks for taking a look @michamilz!

joelpittet’s picture

FileSize
9.55 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2152217-twig-theme_form_required_marker-5.patch. Unable to apply patch. See the log in the details link for more information. View

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

jessebeach’s picture

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

joelpittet’s picture

Status: Needs work » Needs review
FileSize
9.58 KB
FAILED: [[SimpleTest]]: [MySQL] 63,291 pass(es), 106 fail(s), and 0 exception(s). View
1.03 KB

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

Status: Needs review » Needs work

The last submitted patch, 7: 2152217-twig-theme_form_required_marker-7.patch, failed testing.

jenlampton’s picture

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

joelpittet’s picture

Status: Needs work » Needs review
FileSize
11.45 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2152217-twig-theme_form_required_marker-10.patch. Unable to apply patch. See the log in the details link for more information. View
3.14 KB

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.

LewisNyman’s picture

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.

joelpittet’s picture

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?

LewisNyman’s picture

Status: Needs review » Needs work

The last submitted patch, 10: 2152217-twig-theme_form_required_marker-10.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
11.54 KB
PASSED: [[SimpleTest]]: [MySQL] 63,744 pass(es). View

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

Cottser’s picture

Title: Convert theme_form_required_marker() to Twig » Remove theme_form_required_marker() from the theme system - use CSS instead

Retitling.

+++ b/core/modules/system/templates/form-required-marker.html.twig
@@ -0,0 +1,12 @@
+{#
+/**
+ * @file
+ * Default theme implementation for a form element required marker.
+ *
+ * Available variables:
+ * - attributes: A list of HTML attributes for the marker.
+ *
+ * @ingroup themeable
+ */
+#}
+<span class="form-required" aria-hidden="true">*</span>

This template would need to go :)

vijaycs85’s picture

FileSize
11.01 KB
PASSED: [[SimpleTest]]: [MySQL] 64,171 pass(es). View
553 bytes

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

mortendk’s picture

looks very good to me :)

mgifford’s picture

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

sun’s picture

  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"?

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Tagging for reroll.

jessebeach’s picture

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.

joelpittet’s picture

Status: Needs work » Needs review

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.

The last submitted patch, 5: 2152217-twig-theme_form_required_marker-5.patch, failed testing.

joelpittet’s picture

Issue tags: -Needs reroll
FileSize
4.11 KB
12.57 KB
FAILED: [[SimpleTest]]: [MySQL] 64,560 pass(es), 2 fail(s), and 1 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 25: 2152217-remove-required-marker-25.patch, failed testing.

mgifford’s picture

FileSize
185.51 KB

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.

No color with the patch for required element.

joelpittet’s picture

FileSize
1.35 KB
12.56 KB
PASSED: [[SimpleTest]]: [MySQL] 64,617 pass(es). View

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.

mgifford’s picture

Cottser’s picture

Status: Needs work » Needs review
sun’s picture

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.

joelpittet’s picture

Issue tags: -Needs profiling
FileSize
12.9 KB
PASSED: [[SimpleTest]]: [MySQL] 64,719 pass(es). View
11.2 KB
74.64 KB
93.6 KB

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

Cottser’s picture

Seems we'd still want the space after (margin-right) for RTL though, as we had before?

joelpittet’s picture

FileSize
575 bytes
12.94 KB
PASSED: [[SimpleTest]]: [MySQL] 64,760 pass(es). View

Totally, thanks @Cottser.

sun’s picture

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.

sun’s picture

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.

joelpittet’s picture

Assigned: Unassigned » joelpittet

Assigning to tackle #36/37.

LewisNyman’s picture

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.

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Tagging for reroll.

ckrina’s picture

Assigned: joelpittet » ckrina

Starting to work on that.

jjcarrion’s picture

Assigned: ckrina » jjcarrion
jjcarrion’s picture

Assigned: jjcarrion » Unassigned
Status: Needs work » Needs review
FileSize
12.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,803 pass(es), 1 fail(s), and 0 exception(s). View

Reroll made... hope it works ok!

Status: Needs review » Needs work

The last submitted patch, 43: 2152217-twig-theme_form_required_marker-43.patch, failed testing.

jjcarrion’s picture

Assigned: Unassigned » jjcarrion

keep working

jjcarrion’s picture

Assigned: jjcarrion » Unassigned
Status: Needs work » Needs review
FileSize
0 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,862 pass(es). View

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

jjcarrion’s picture

Assigned: Unassigned » jjcarrion
Status: Needs review » Needs work

last patch was empty :(

keep working

jjcarrion’s picture

Assigned: jjcarrion » Unassigned
Status: Needs work » Needs review
FileSize
12.65 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,828 pass(es). View

Here is the patch.

Status: Needs review » Needs work

The last submitted patch, 48: 2152217-twig-theme_form_required_marker-48.patch, failed testing.

jjcarrion’s picture

Status: Needs work » Needs review
joelpittet’s picture

Issue tags: -Needs reroll

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!

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Tagging for another reroll.

mgifford’s picture

Status: Needs work » Needs review
FileSize
12.65 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,072 pass(es). View

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

89c89
<    $title = filter_xss_admin($element['#title']);
---
>    $title = Xss::filterAdmin($element['#title']);
mark.labrecque’s picture

Assigned: Unassigned » mark.labrecque
mark.labrecque’s picture

Assigned: mark.labrecque » Unassigned
vijaycs85’s picture

Issue tags: -Needs reroll +MANCHESTER_2014_APRIL
FileSize
12.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,693 pass(es), 1 fail(s), and 0 exception(s). View

Another reroll...

Status: Needs review » Needs work

The last submitted patch, 56: 2152217-theme_required_marker-56.patch, failed testing.

vijaycs85’s picture

Issue tags: +Needs tests

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

sun’s picture

+.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? :-))

joelpittet’s picture

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

vijaycs85’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
13.55 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,311 pass(es). View
1.37 KB
39.34 KB
46.36 KB

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

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing
FileSize
67.42 KB
78.38 KB
95.42 KB
590.35 KB

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?
vijaycs85’s picture

Issue summary: View changes

Updating commit message...

Cottser’s picture

Issue summary: View changes

Let's just strip it, the patch here has very little in common with the work that was originally done in #1898480: [meta] form.inc - Convert theme_ functions to Twig.

sun’s picture

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

jessebeach’s picture

Status: Reviewed & tested by the community » Needs work

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.

jessebeach’s picture

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

joelpittet’s picture

@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?

mgifford’s picture

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

joelpittet’s picture

⁕ U+2055

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

jessebeach’s picture

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;
}
joelpittet’s picture

@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 ⁎

joelpittet’s picture

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

joelpittet’s picture

To show the differences:

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

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

mgifford’s picture

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

joelpittet’s picture

Status: Needs work » Needs review
FileSize
13.6 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,321 pass(es). View
590 bytes

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

mgifford’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
32.09 KB
356 bytes

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:
Zoomed asterisks.

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

vijaycs85’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
74.67 KB

Sorry, not working for Right to left :(

mgifford’s picture

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.

joelpittet’s picture

@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?

rteijeiro’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
90.94 KB
583.21 KB

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

It's a RTBC for me :)

sun’s picture

FileSize
13.68 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,323 pass(es). View
398 bytes

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

mgifford’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
13.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,606 pass(es). View

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

Status: Needs review » Needs work

The last submitted patch, 84: drupal8.form-required.84.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review

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

mgifford’s picture

FileSize
321 bytes

Bad bot.. But here's the interdiff.

Status: Needs review » Needs work

The last submitted patch, 84: drupal8.form-required.84.patch, failed testing.

joelpittet’s picture

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

joelpittet’s picture

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

joelpittet’s picture

Status: Needs work » Reviewed & tested by the community

RTBC even.

mgifford’s picture

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 84: drupal8.form-required.84.patch, failed testing.

joelpittet’s picture

Status: Needs work » Reviewed & tested by the community

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

vijaycs85’s picture

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

  • Commit 82aaa8a on 8.x by catch:
    Issue #2152217 by joelpittet, mgifford, vijaycs85, jjcarrion, sun:...
catch’s picture

Status: Reviewed & tested by the community » Fixed

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

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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

LewisNyman’s picture

Status: Closed (fixed) » Needs work
FileSize
17.07 KB

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

The required marker is not rendered in windows

joelpittet’s picture

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

joelpittet’s picture

Issue summary: View changes
FileSize
14.12 KB

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?

mgifford’s picture

Pretty low odds on that.. Damn you IE!

How about: ✻

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

eft’s picture

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.

joelpittet’s picture

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

eft’s picture

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

Cottser’s picture

Can we please open a follow up issue instead? Thanks!

mgifford’s picture

Status: Needs work » Closed (fixed)

@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?

sun’s picture

alimac’s picture

Issue tags: -#SprintWeekend2014 +SprintWeekend2014

Minor tag cleanup - please ignore.

alimac’s picture

Minor tag cleanup - please ignore.