Improve "add" links accessibility

This is totally a WCAG 2.0 AA issue. It shows up quite a few places, but the biggest challenge is most likely going to be finding them all.

Well, the add links aren't really that hard to fine, the question is how can we

• Provide an accessible link and
• don't fill up the UI too much at the same time

Would it already help to have a proper 'title' attribute?
This patch adds

title="Add field"

instead of just "add".

@dawehner - Excellent.

Glad usability is framed here. Do we want the text to appear?

If not we can easy wrap the extra text in a span with element-invisible.

I think it might be a useful queue for everyone, but a bit of a change.

If not we can easy wrap the extra text in a span with element-invisible.

Do you know other places in core, where parts of the title is hidden to the user?
It seems to be a perfect way to solve both problems at the same time.

Longer titles here would certainly cause not only more text on the page, but potentially also mobile edit problems.

Add tag

I think "Add" is more consistent with Core. It might not be a better pattern, but it's the pattern that's there.

Well yes, most user get what "add" means as it has easier context but adding a proper 'title' even makes sense for them.
Want to review the patch to improve it?

I like it. Attaching some screen shots. I do think it helps clarify things for everyone and is a good pattern that makes it more accessible & usable for everyone.

Here we have the pattern of operations/dropbutton, which , as you see in screenshot "add_block.png" doesn't use long names.

#2: drupal-1849610-2.patch queued for re-testing.

There we go.

As far as I can tell, the screenshots in #8 aren't related to this patch. Tagging for manual testing. Let's get before-and-after screenshots of these elements in the Views UI.

Here are before and after screens shots with the related patch. I must admit when I reduced my screen width to anything less than 960px, the add buttons overlayed the various titles in a very annoying way.

1. Before

2. After

@edrupal

Is there any specific reason why we don't use just 'title' for the long description?

Think @xjm's tags got removed by @edrupal but wanted to check.

Sorry if I included the wrong screen shots in #8. Thanks for providing nice before/after @edrupal.

Oops, didn't mean to remove those tags.

#15: improve-add-links-accessibility-1849610-15.patch queued for re-testing.

Needs work based on #16

I don't know where to insert the long description here. Can you update the patch? I looked at the last two patches and couldn't figure out what suggestions you wanted implemented.

Anyway to get it pass WCAG 2.0 and no UI changes ?

There shouldn't be any UI changes necessarily. I think it would be a usability improvement to change the text from "Add" to "Add filter criteria" but as we've done in the past we can just make the contextual text " filter criteria" hidden with element-invisible.

With that change it would meet WCAG and require no UX change. I still just don't understand what's being asked in #16 related to the latest patch and would like to see a new patch so I could evaluate it.

I've redone the patch in #15, making all the contextual text hidden using element-invisible as suggested by @mgifford in #23. Additionally I've used the contextual text as the 'title' attribute for each of the 'Add' links. (Possibly what was being inferred in #16).

1. Screenshot with contextual text using element-invisible

2. Screenshot with element-invisible disabled

Nice! I looked over the code & tested it to see that the contextual is properly pronounced with ChromeVox on the Mac.

Nothing in $add_text and $rearrange_prefix_text will ever be translatable in the software this way, so this is not a good way to to make this work. The full resulting strings should be assembled and directly t()ed in the conditions even if that looks like you are repeating some text boilerplate a few times.

Drat... Forgot to check that. This works, but looks wrong.

$actions['add'] = array(
      'title' => t("Add<span class=\"element-invisible\"> $add_text</span>"),
      'href' => "admin/structure/views/nojs/add-item/{$view->id()}/{$display['id']}/$type",
      'attributes' => array('class' => array('icon compact add', 'views-ajax-link'), 'title' => t("Add $add_text"), 'id' => 'views-add-' . $type),
      'html' => TRUE,
    );

We want the string not the variable, so I think it's ok...
http://api.drupal.org/api/drupal/core!includes!bootstrap.inc/function/t/8

Hi @Gábor Hojtsy, or anyone else, a bit of advice/pointers if that's ok.

Translating all the contextual text in full would be something like

switch ($type) {
  case 'field':
    $add_text = t('add <span class="element-invisible">fields</span>');
    $add_text_link = t('add fields)';
    $rearrange_text = t('add/or rearrange <span class="element-invisible">fields</span>');
    $rearrange_text_link = t('add/or rearrange fields');
    break;
  case 'sort':
    $add_text = t('add <span class="element-invisible">sort criteria</span>');
    $add_text_link = t('add sort criteria)';
    $rearrange_text = t('rearrange <span class="element-invisible">sort criteria</span>');
    $rearrange_text_link = t('rearrange sort criteria');
    break;
and so on for other types
}
$actions['add'] = array(
  'title' => $add_text,
  'attributes' => array('title' => $add_text_link),

if ($count_handlers > 0) {
  $actions['rearrange'] = array(
     'title' => $rearrange_text,
     'attributes' => array('title' => $rearrange_text_link),

To try and shorten the code is it valid to do something like this?

$add_prefix = t('add');
$rearrange_prefix = t('rearrange');
switch ($type) {
  case 'field':
    $add_text = t('fields');
    break;
  case 'sort':
    $add_text = t('sort criteria');
    break;
and so on for other filters
}
$actions['add'] = array(
      'title' => $add_prefix . '<span class="element-invisible">' . $add_text . '</span>',
      'attributes' => array('title' => $add_prefix . $add_text),

if ($count_handlers > 0) {
  $actions['rearrange'] = array(
     'title' => $rearrange_prefix . '<span class="element-invisible">' . $add_text . '</span>',
     'attributes' => array('title' => $rearrange_prefix . $add_text),

Any advice much appreciated

Ed

I did think about the method posted in #27, and got put off by the comment in the api docs

You should never use t() to translate variables .. unless the text that the variable holds has been passed through t() elsewhere

.

If that is fine then it's much easier and please ignore my post #28

Thanks, Ed

@edrupal - I was thinking too about the line about variables you quoted, but although this is technically a variable, the scope is limited. It's just a more efficient way of producing the same number of fixed strings. I think the main goal of that warning is that we're not opening up a larger set of translations.

@falcon03 - ultimately you're the expert here. My understanding is that the link title tag is often not read by screen readers. It's there as a hover-over which is useful for folks who are mousing over a link & wanting to know more context. If it works for you, then I think we'll be fine.

How do we best improve the anchor text?

@edrupal: yes, t()-ing short pieces of text and concatenating them is not really a good idea because they will not work as a "sentence" in foreign languages. Not all languages follow the same order of words or use the same words as translations of the same word in a different context. So giving more context by showing how the text is used (in its full "sentence") is important to get translations that work. Otherwise it will just end up very unprofessional (if sort strings are t()-ed and then concatenated) or even just plain English without the possibility to translate (in the patch that was RTBC last time).

Hope this helps.

So my patch in #27 is fine? I can't see any other way to do this.

How does hte patch in #27 translate 'contextual filters' or 'Rearrange' or most other text components?!

Well, these are all define as variables:

    $rearrange_prefix_text = 'Rearrange';
    $rearrange_prefix_text = 'And/Or, Rearrange';
    $add_text .= ' criteria';
    $add_text = 'contextual filters';
    ...

Which are then rendered as strings before they are sent to t():

      $actions['rearrange'] = array(
        'title' => t("$rearrange_prefix_text<span class=\"element-invisible\"> $add_text</span>"),
        'href' => $rearrange_url,
        'attributes' => array('class' => array($class, 'views-ajax-link'), 'title' => t("$rearrange_prefix_text $add_text"), 'id' => 'views-rearrange-' . $type),
        'html' => TRUE,
      );

It's possible I'm missing something, but that seems to cover the options. This should make it easier to translate.

@edrupal quoted this pretty explicitly and very rightly in #29 (from http://api.drupal.org/api/drupal/core!includes!bootstrap.inc/function/t/8):

You should never use t() to translate variables .. unless the text that the variable holds has been passed through t() elsewhere

Problem is your approach with $whatever = 'boo'; t($whatever) makes 'boo' translatable only when this code runs. When the code is not run, there is *no way* to tell what is in $whatever when t($whatever) is looked at. Reality is modules cannot be *run* to find their translatable strings when localize.drupal.org collects translatable strings. I mean how could this code be run to collect all the possible combinations of variable values?! Right? The patch in itself has lots of them! So the code cannot be run, but the code is **statically parsed** for translatable strings. How would static code parsing identify those strings for translation? No way! It would need to parse through all the conditions and figure out possible value combinations (AKA run the code).

So while $whatever = 'boo'; t($whatever) makes 'boo' translatable *on the site*, there is no community pre-translation possible. The UI can only be translated *after the fact*, *only on the site itself* once at least one user faced the untranslated UI at least once (so the Drupal runtime figures out the string to translate dynamically and puts it into the database).

We have higher aspirations of translation completeness and distribution to settle with translations to be manually entered after the fact on a site by site basis.

I'm working on another version of the patch which I think will address all the issues. With you shortly :-)

Thanks @Gabor. I wondered if that might be the case. I do appreciate your efforts in getting i18n right.

There must be other examples of this though where programatically it makes sense to create a fixed set of strings with variables, but that it causes this translation problem. Probably not something D8, but couldn't we simply create a comment with values in the function:

/* 
 * Strings for translation
 * t("Rearrange<span class=\"element-invisible\"> criteria</span>")
 * t("Rearrange<span class=\"element-invisible\"> contextual filters</span>")
 * t("Add<span class=\"element-invisible\"> criteria</span>")
 * t("And/Or, Rearrange<span class=\"element-invisible\"> contextual filters</span>")
 */

Just an idea. Looking forward to seeing @edrupal's modifications. Keep in mind @falcon03's comment in #30.

@mgifford: right, the point is since you'd need to include the assembled strings in your case in comments anyway, why not include them in the runtime code in the first place, so there is no duplication. You get less resulting code, no ambiguity and no cross-fixes needed if you change your logic/strings.

True.

Here's the updated patch. I haven't done an interdiff as there's not much the same as before. The thinking is to explicitly define all the required text strings, and to have them all in one place so it's easy to see what might need translating. Also to move the definitions out of the main flow of the code by calling a function. A couple of questions:
Is it ok practice to call a function from within the main class?
If so is the function name ok?

If @falcon03's point in #30 requires the link anchor text to be amended/deleted it should be pretty easy with this rearrange code.

I can't speak to the function name, but I do think all of the strings need to be defined like:

+    'field' => array(
+      'add' => t('Add<span class="element-invisible"> fields</span>'),
+      'add_attr' =>  t('Add fields'),
+      'rearrange' =>  t('Rearrange<span class="element-invisible"> fields</span>'),
+      'rearrange_attr' =>  t('Rearrange fields'),
+    ),

So that they are translated.

Doh! That was the whole point of the re-write, 2/10 could do better :-). Here's what it should have been.

Looks good! Thanks @edrupal. And ya, totally understand with the re-write. That stuff happens.

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/ViewEditFormController.phpundefined
@@ -1069,3 +1073,55 @@ public static function addMicroweights(&$build) {
+function ViewEditFormController_get_add_rearrange_text($type, $text_type) {

This is not a valid method name. I'm surprised this works.
It should be something like protected function getAddRearrangeText($type, $text_type) {, and called like $this->getAddRearrangeText($type, 'add');

Also, the method needs a docblock with @param and @return. And the first line should start with Returns, not Return.

Thanks for the feedback Tim, so much to learn. Here's an updated patch.

Rather late, but I added #1919940: Build API to Replace Links using Title Attributes with Proper Accessible, Themable Tooltips - I think we need to have an API or something in Javascript to help have an organized, accessible response to this. Could play with the dom & move info out of the title attribute.

In anycase, @edrupal added some great tooltip info (for sighted users) that I've stripped out. That isn't a regression from what's in Core now (as really the use of the title attribute is now only repeating the highlighted link text).

But I do think it would be useful for everyone to have a bit more context, which we can only really get when we have some sort of tooltip api in core.

Tested #49 patch (improve-add-links-accessibility-1849610-49.patch) on simplytest.me and looks fine. Please see the screenshots below:

1. before patch - Fields
   
2. before patch - Filter criteria
   
3. before patch - Sort criteria
   

1. after patch - Fields
   
2. after patch - Filter criteria
   
3. after patch - Sort criteria
   

The before and after images show the patch is doing what is supposed.
I just wonder if more information could be added in the
List additional actions in order to press the enter key.

I do think we should probably look at creating a new issue to make a pattern so that this type of button is more discoverable for keyboard only issues.

@mgifford
Do you think we should first make a pattern and apply it here or get this in independent from that possible issue?

Let's get it in independently and then bring it up in a related issue.

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/ViewEditFormController.phpundefined
@@ -919,17 +917,21 @@ public function getFormBucket(ViewUI $view, $type, $display) {
+    // Create the title variables for the add action
...
+      // Create the title variables for the rearrange action

@@ -1068,4 +1070,53 @@ public static function addMicroweights(&$build) {
+   *   A string containing the required title text
...
+    // An array of all the possible text strings that may be required

Missing period

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/ViewEditFormController.phpundefined
@@ -1068,4 +1070,53 @@ public static function addMicroweights(&$build) {
+    // Check there is an array element that matches the supplied arguments

This is not a full sentence, nor does it end with a period.

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/ViewEditFormController.phpundefined
@@ -1068,4 +1070,53 @@ public static function addMicroweights(&$build) {
+    if (isset($text_strings[$field_type][$title_type])) {
+      return $text_strings[$field_type][$title_type];
+    } else return '';

This should be

if (isset($text_strings[$field_type][$title_type])) {
  return $text_strings[$field_type][$title_type];
}

return '';

Good catch. Think this resolves those issues.

It really looks like there should be an extra '}' in there as the spacing is off. I think it's because of the class, but it just looks like it's missing one.

Let's try that again though to try to address
Parse error: syntax error, unexpected T_RETURN, expecting T_FUNCTION in ./core/modules/views/views_ui/lib/Drupal/views_ui/ViewEditFormController.php on line 1122

Arg..

#61 didn't apply on simplytest.me, I think the file had changed in HEAD in the interim. Here's a revised patch.

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/ViewEditFormController.phpundefined
@@ -1061,4 +1063,54 @@ public static function addMicroweights(&$build) {
+    } ¶

YEAH I found an empty space :)

Oops, sorry.

Fixed, I think.

Looks good to me. Works in SimplyTest.me. @dawehner - you able to mark this RTBC?

Just tagging this as work done during the global sprint.

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/ViewEditFormController.phpundefined
@@ -1068,4 +1070,53 @@ public static function addMicroweights(&$build) {
+  protected function getAddRearrangeTitleText($field_type, $title_type) {

Can we name it $handler_type instead of $field_type, because it's really not about fields are something like that. That's an overused word all over drupal :) In general i'm wondering why we don't facilitate the information which exists in ViewExecutable::viewsHandlerTypes() to generate all this strings, but yeah I will block this patch just because of that.

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/ViewEditFormController.phpundefined
@@ -1068,4 +1070,53 @@ public static function addMicroweights(&$build) {
+    } else return '';

needs a new line and "{}" to match drupal code style.

due to #67 it needs work

@dawehner, I changed the reference to $handler_type as requested but didn't find } else return ''; in patch in #64.

good to se here an interdiff http://drupal.org/documentation/git/interdiff

#64: improve-add-links-accessibility-1849610-64.patch queued for re-testing.

#69: improve-add-links-accessibility-1849610-69.patch queued for re-testing.

@podarok - I think this is what you want.

Well, I still think we should use viewsHandlerTypes(), see comment #1849610-67: Improve "add" links accessibility.

Sorry @dawehner I totally missed that in my earlier patch (and especially the critical part about blocking this patch). So how do we incorporate viewsHandlerTypes()?

So, viewsHandlerTypes() in core/modules/views/lib/Drupal/views/ViewExecutable.php do you want us to just incorporate getAddRearrangeTitleText() into that? Or add the string translation something like:

        'argument' => array(
          'title' => t('Contextual filters'),
          'ltitle' => t('contextual filters'),
          'stitle' => t('Contextual filter'),
          'lstitle' => t('contextual filter'),
          'plural' => 'arguments',
        ),

I really have never seen this function before so really need some additional context.

$types = ViewExecutable::viewsHandlerTypes();
$info = $types[$type];
return array(
  'add' => t('Add <span class="element-invisible">$info['ltitle']</span>'),
  'rearrange' => t('Rearrange <span class="element-invisible">$info['ltitle']</span>'),
);

This function then seems so simple there there is no real need for a function anymore.

I'm having problem with my local install, but tossing it up here to test on simplytest.me quickly.

Annoying.. No idea why my local install continues to fail..

arg..

Right.. Didn't check if I had PHP 5.3.5 on install.. Anyways, sorry for the annoyance around this relatively simple patch.

I think the current solution re-raises the concerns raised by Gábor Hojtsy in #36 about strings being translatable.

Perhaps a better solution is to move all the translatable strings to the function viewExecutable::viewsHandlerTypes() and just get the strings from the function.

This patch attempts to implement this idea.

Right, @mgifford's patch versions from #81 to #85 all exhibit problems that are against the guidelines I posted above. Re-posting for re-reading :)

Problem is your approach with $whatever = 'boo'; t($whatever) makes 'boo' translatable only when this code runs. When the code is not run, there is *no way* to tell what is in $whatever when t($whatever) is looked at. Reality is modules cannot be *run* to find their translatable strings when localize.drupal.org collects translatable strings. I mean how could this code be run to collect all the possible combinations of variable values?! Right? The patch in itself has lots of them! So the code cannot be run, but the code is **statically parsed** for translatable strings. How would static code parsing identify those strings for translation? No way! It would need to parse through all the conditions and figure out possible value combinations (AKA run the code).

So while $whatever = 'boo'; t($whatever) makes 'boo' translatable *on the site*, there is no community pre-translation possible. The UI can only be translated *after the fact*, *only on the site itself* once at least one user faced the untranslated UI at least once (so the Drupal runtime figures out the string to translate dynamically and puts it into the database).

We have higher aspirations of translation completeness and distribution to settle with translations to be manually entered after the fact on a site by site basis.

Thanks @edrupal and ya @Gabor, that had occurred to me. I was just muddling through @dawehner's suggestion while my own local environment was messed up. Without having a good sense of how ViewExecutable::viewsHandlerTypes() was used, I didn't want to start there.

@falcon03, having that default $add_text string will ensure that there is always some "Add" text even if someone passes through an incorrect $type such as 'arguments'. I'm not sure when this might happen but it's .

Anyways, the patch seems to provide the desired results in SimplyTest.me and seems fairly straight forward.

What other reviews are required before this is RTBC?

Annoyingly I forgot to wrap the default add and rearrange text values in the t() function. Corrected patch here.

Couldn't apply the patch.
error: patch failed: core/modules/views/views_ui/lib/Drupal/views_ui/ViewEditFormController.php:917
Do I have something else to do to apply it?

The patch in #91 applied for me fine on SimplyTest.me, and did as it was supposed to. (The same as the outcome in #90; didn't seem worth duplicating screenshots.)

Maybe another person could test it and verify the patch applies? But both the testbot and SimplyTest pass it fine.

The idea for using viewsHandlerTypes() was brilliant, @dawehner++!

Well maybe actual thought was to use the information on viewsHandlerTypes() to generate those strings
as they use the same pattern anyway? It feels wrong to hard-code texts into the Executable, which is just used in the UI, but yeah
I don't care about the second point.

Ok, latest patch seems to remove any dynamic use of t(), per #36/88, and has been manually tested in #90.

However, I can't tell if #96 is a "needs work" or not. Settling on "needs review." I'll also send it back for a re-test, cos it's been a few days.

#91: improve-add-links-accessibility-1849610-91.patch queued for re-testing.

Set to needs work from my understanding.

That's what I have been thinking about, sorry for my behavior.

Doesn't this approach re-open the issues raised by @Gábor Hojtsy in #88?

Doesn't this approach re-open the issues raised by @Gábor Hojtsy in #1849610-88: Improve "add" links accessibility?

Okay this time without notices.

@Gabor
It's not okay to have t() with the strings directly in there? If no, cool, let's go with #91

#91: improve-add-links-accessibility-1849610-91.patch queued for re-testing.

I think the current state of the patch looks good from a translatability standpoint. The title that is put into the string is already translated prior (tim.plunkett pointed me to http://api.drupal.org/api/drupal/core%21modules%21views%21lib%21Drupal%2...). So it is similar to page titles where t('Add @type content') is used, etc.

Looking good from here.

Wow, that's much simpler. :)

Committed and pushed to 8.x. Thanks!