When adding/editing a view, you can find a lot of useful "add" links (related to sort criteria, filter criteria, fields, etc).
A great accessibility/usability improvement (IMO) would be to make the purpose of these links understandable out of their context. So, for instance, the "add" link text of the link that let users add a sort criteria should become "Add sort criteria", the "add" link text of the link that let users add a filter criteria should become "Add filter criteria", the "Add" link text of the link that let users add a field should become "Add field" and so on.
Also, we should consider that this improvement can be a step up to WCAG 2.0 conformance for the Views UI (WCAG 2.0 2.4.4 guideline).
BTW, sorry for the various "link" repetitions, I didn't know how to avoid them while keeping the explanation clear). I hope the summary is understandable, otherwise here I am! ;)
Comment | File | Size | Author |
---|---|---|---|
#105 | drupal-1849610-105.patch | 1.94 KB | dawehner |
#101 | drupal-1849610-101.patch | 1.92 KB | dawehner |
#91 | improve-add-links-accessibility-1849610-91.patch | 6.18 KB | edrupal |
#91 | interdiff-87-91.txt | 1.12 KB | edrupal |
#90 | Screen Shot 2013-03-25 at 8.14.39 AM.png | 287.93 KB | mgifford |
Comments
Comment #1
mgiffordThis 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.
Comment #2
dawehnerWell, the add links aren't really that hard to fine, the question is how can we
Would it already help to have a proper 'title' attribute?
This patch adds
instead of just "add".
Comment #3
mgifford@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.
Comment #4
dawehnerDo 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.
Comment #5
dawehnerAdd tag
Comment #6
mgiffordI think "Add" is more consistent with Core. It might not be a better pattern, but it's the pattern that's there.
Comment #7
dawehnerWell 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?
Comment #8
mgiffordI 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.
Comment #9
dawehnerHere we have the pattern of operations/dropbutton, which , as you see in screenshot "add_block.png" doesn't use long names.
Comment #10
mgifford#2: drupal-1849610-2.patch queued for re-testing.
Comment #12
xjmComment #13
dawehnerThere we go.
Comment #14
xjmAs 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.
Comment #15
edrupal CreditAttribution: edrupal commentedHere 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
Comment #16
dawehner@edrupal
Is there any specific reason why we don't use just 'title' for the long description?
Comment #17
mgiffordThink @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.
Comment #18
edrupal CreditAttribution: edrupal commentedOops, didn't mean to remove those tags.
Comment #19
mgifford#15: improve-add-links-accessibility-1849610-15.patch queued for re-testing.
Comment #20
dawehnerNeeds work based on #16
Comment #21
mgiffordI 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.
Comment #22
droplet CreditAttribution: droplet commentedAnyway to get it pass WCAG 2.0 and no UI changes ?
Comment #23
mgiffordThere 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.
Comment #24
edrupal CreditAttribution: edrupal commentedI'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
Comment #25
mgiffordNice! I looked over the code & tested it to see that the contextual is properly pronounced with ChromeVox on the Mac.
Comment #26
Gábor HojtsyNothing 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.
Comment #27
mgiffordDrat... Forgot to check that. This works, but looks wrong.
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
Comment #28
edrupal CreditAttribution: edrupal commentedHi @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
To try and shorten the code is it valid to do something like this?
Any advice much appreciated
Ed
Comment #29
edrupal CreditAttribution: edrupal commentedI did think about the method posted in #27, and got put off by the comment in the api docs
.
If that is fine then it's much easier and please ignore my post #28
Thanks, Ed
Comment #30
falcon03 CreditAttribution: falcon03 commented@mgifford, I think we shouldn't use the same string as the link anchor text and the link title, since the title attribute value wouldn't be very useful this way.
IMO improving the link anchor text is enough to solve this issue! What do you think?
Comment #31
mgifford@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?
Comment #32
Gábor Hojtsy@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.
Comment #33
mgiffordSo my patch in #27 is fine? I can't see any other way to do this.
Comment #34
Gábor HojtsyHow does hte patch in #27 translate 'contextual filters' or 'Rearrange' or most other text components?!
Comment #35
mgiffordWell, these are all define as variables:
Which are then rendered as strings before they are sent to t():
It's possible I'm missing something, but that seems to cover the options. This should make it easier to translate.
Comment #36
Gábor Hojtsy@edrupal quoted this pretty explicitly and very rightly in #29 (from http://api.drupal.org/api/drupal/core!includes!bootstrap.inc/function/t/8):
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.
Comment #37
edrupal CreditAttribution: edrupal commentedI'm working on another version of the patch which I think will address all the issues. With you shortly :-)
Comment #38
mgiffordThanks @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:
Just an idea. Looking forward to seeing @edrupal's modifications. Keep in mind @falcon03's comment in #30.
Comment #39
Gábor Hojtsy@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.
Comment #40
mgiffordTrue.
Comment #41
edrupal CreditAttribution: edrupal commentedHere'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.
Comment #42
mgiffordI can't speak to the function name, but I do think all of the strings need to be defined like:
So that they are translated.
Comment #43
edrupal CreditAttribution: edrupal commentedDoh! That was the whole point of the re-write, 2/10 could do better :-). Here's what it should have been.
Comment #44
mgiffordLooks good! Thanks @edrupal. And ya, totally understand with the re-write. That stuff happens.
Comment #45
tim.plunkettThis 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.
Comment #46
edrupal CreditAttribution: edrupal commentedThanks for the feedback Tim, so much to learn. Here's an updated patch.
Comment #47
edrupal CreditAttribution: edrupal commentedComment #48
falcon03 CreditAttribution: falcon03 commented@mgifford#31: yes, but adding the same value for the anchor text and the link title attribute is really annoying for blind users, especially Mac OS X ones (Voiceover reads the title attribute very well :-)).
So if it is not a problem we can simply use the new string only for the link anchor text and avoid using the title attribute at all..
Comment #49
mgiffordRather 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.
Comment #50
druplr CreditAttribution: druplr commentedTested #49 patch (improve-add-links-accessibility-1849610-49.patch) on simplytest.me and looks fine. Please see the screenshots below:
Comment #51
solange_sari CreditAttribution: solange_sari commentedThe 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.
Comment #52
solange_sari CreditAttribution: solange_sari commentedComment #53
mgiffordI 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.
Comment #54
dawehner@mgifford
Do you think we should first make a pattern and apply it here or get this in independent from that possible issue?
Comment #55
mgiffordLet's get it in independently and then bring it up in a related issue.
Comment #56
tim.plunkettMissing period
This is not a full sentence, nor does it end with a period.
This should be
Comment #57
mgiffordGood catch. Think this resolves those issues.
Comment #59
mgiffordIt 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
Comment #61
mgiffordArg..
Comment #62
rootwork#61 didn't apply on simplytest.me, I think the file had changed in HEAD in the interim. Here's a revised patch.
Comment #63
dawehnerYEAH I found an empty space :)
Comment #64
rootworkOops, sorry.
Fixed, I think.
Comment #65
mgiffordLooks good to me. Works in SimplyTest.me. @dawehner - you able to mark this RTBC?
Comment #66
rootworkJust tagging this as work done during the global sprint.
Comment #67
dawehnerCan 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.
needs a new line and "{}" to match drupal code style.
Comment #68
podarokdue to #67 it needs work
Comment #69
mgifford@dawehner, I changed the reference to $handler_type as requested but didn't find
} else return '';
in patch in #64.Comment #70
podarokgood to se here an interdiff http://drupal.org/documentation/git/interdiff
Comment #72
mgifford#64: improve-add-links-accessibility-1849610-64.patch queued for re-testing.
Comment #73
mgifford#69: improve-add-links-accessibility-1849610-69.patch queued for re-testing.
Comment #74
mgifford@podarok - I think this is what you want.
Comment #75
falcon03 CreditAttribution: falcon03 commentedFrom a functional point of view, this is really close to RTBC for me.
The only problem I found is that there is not a "whitespace" ( ) between the "Add" word and the object that it refers to.
E.G. The anchor text of the link to add a filter criteria is "Addsort criteria", but it should be "Add sort criteria".
Comment #76
falcon03 CreditAttribution: falcon03 commentedfixed #75. Not marking RTBC to wait for a code review!
I've changed the markup this way.
Before this patch:
<a ...>Add<span class§="element-invisible"> object</span></a>
After this patch:
<a ...>Add <span class="element-invisible">object</span></a>
Comment #77
falcon03 CreditAttribution: falcon03 commentedsorry, forgot patch!
Comment #78
dawehnerWell, I still think we should use viewsHandlerTypes(), see comment #1849610-67: Improve "add" links accessibility.
Comment #79
mgiffordSorry @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:
I really have never seen this function before so really need some additional context.
Comment #80
dawehnerThis function then seems so simple there there is no real need for a function anymore.
Comment #81
mgiffordI'm having problem with my local install, but tossing it up here to test on simplytest.me quickly.
Comment #83
mgiffordAnnoying.. No idea why my local install continues to fail..
Comment #84
mgiffordarg..
Comment #85
mgiffordRight.. Didn't check if I had PHP 5.3.5 on install.. Anyways, sorry for the annoyance around this relatively simple patch.
Comment #87
edrupal CreditAttribution: edrupal commentedI 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.
Comment #88
Gábor HojtsyRight, @mgifford's patch versions from #81 to #85 all exhibit problems that are against the guidelines I posted above. Re-posting for re-reading :)
Comment #89
falcon03 CreditAttribution: falcon03 commented#87 looks good to me, but viewEditController.php (patched) shows:
+ $add_text = 'Add';
+ if (isset($types[$type]['add_text'])) {
Why do we need to initialize the $add_text variable and check if $types[$type]['add_text'] is set?
Comment #90
mgiffordThanks @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?
Comment #91
edrupal CreditAttribution: edrupal commentedAnnoyingly I forgot to wrap the default add and rearrange text values in the t() function. Corrected patch here.
Comment #92
oresh CreditAttribution: oresh commentedCouldn'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?
Comment #93
rootworkThe 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.
Comment #94
rootworkComment #95
tim.plunkettThe idea for using viewsHandlerTypes() was brilliant, @dawehner++!
Comment #96
dawehnerWell 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.
Comment #97
webchickOk, 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.
Comment #98
webchick#91: improve-add-links-accessibility-1849610-91.patch queued for re-testing.
Comment #99
dawehnerSet to needs work from my understanding.
Comment #100
falcon03 CreditAttribution: falcon03 commented@everyone: what review do we need to get this patch in core?
Comment #101
dawehnerThat's what I have been thinking about, sorry for my behavior.
Comment #103
edrupal CreditAttribution: edrupal commentedDoesn't this approach re-open the issues raised by @Gábor Hojtsy in #88?
Comment #104
edrupal CreditAttribution: edrupal commentedDoesn't this approach re-open the issues raised by @Gábor Hojtsy in #1849610-88: Improve "add" links accessibility?Comment #105
dawehnerOkay 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
Comment #106
mgifford#91: improve-add-links-accessibility-1849610-91.patch queued for re-testing.
Comment #107
Gábor HojtsyI 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.
Comment #108
tim.plunkettLooking good from here.
Comment #109
webchickWow, that's much simpler. :)
Committed and pushed to 8.x. Thanks!