Link titles on Create Content page, non-helpful and too long
mikey_p - May 16, 2009 - 22:58
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | node system |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | accessibility, Needs usability review |
Description
See:
I'm thinking something along the lines of 'Create ' since that is the title of the empty node form.
#1
Forgot to change the format, the image is:
and the proposed title is 'Create NODETYPE'
#2
in http://api.drupal.org/api/function/system_admin_menu_block/7
I suggest just adding:
unset($item['localized_options']['attributes']['title']);as a 1st pass.#3
This is also true throughout the entire admin section, due to the behavior of system_admin_menu_block().
pwolanin suggested adding unset($item['localized_options']['attributes']['title']); to system_admin_menu_block, which works, but unsets the title for all items. Should we add a default title in for items on the admin page, or just unset it and let the theme functions handle the title for each use of system_admin_menu_block (all uses return output through a theme function).
#4
Here's a patch that fixes the Create content page at least.
#5
New version:
Also need to figure out what would work best on admin overview page.
#6
that doesn't work, you are throwing away unknown data in the options array. Also, why set 'html' to true? Just directly set the title attribute in the localized options if that's the intent.
#7
tagging
#8
Everett explained in IRC that some screenreaders will only read the title text of an anchor, and not the link text itself. This patches core and seven.
See http://openconcept.ca/blog/everett/first_glance_accessibility_evaluation... item number 13.
#9
+1
This certainly makes the experience better for VoiceOver users who now don't have to hear the description of the content type read twice. Also, it is better markup, as the title attribute is not meant to convey as much information about a link as it was.
#10
Looks good - make sure to apply all new standards. it would be Create new page.
#11
@Bojhan: This patch actually removes the title attribute altogether for these links, and on the admin dashboard. Ideally a fix for providing a title that's useful and accessible could be rolled into #526712: l() does not accept a parameter for supplemental text for screen-readers
#12
Yup. This applies well and removes a silly duplication.
#13
The last submitted patch failed testing.
#14
this ran fine locally, guessing it was a bad bot. Setting back to RTBC.
#15
While I can see that this does fix the problem, it seems to do so in a sub-optimal way. If that many places are unsetting this title attribute, it begs the question of whether that title attribute ought to be set in the first place and instead explicitly specified in places that warrant it.
Could you provide a bit more data here? Where is this property being set initially, and would it be better for it to not be set there, and instead let anyone who explicitly wants a title attribute set it in their calling code?
#16
I agree with webchick in #15. Let's not set the attribute to begin with.
#17
Well, I'm pretty sure there is even a more direct way to do this, but I can't seem to get further up the path.
In anycase, it substantially reduces the size of the patch & stops the problem of unsetting the title attribute in many different places.
#18
setting it back to RTBC with input from Dries & webchick applied to the above patch.
#19
Please don't mark patches RTBC until they've been reviewed.
That said, that looks like a ... very strange way to solve this. You shouldn't have to resort to populating serialized values, and doing so this way seems like it'll effectively blank all of the options when all you really want is title.
#20
Sorry. Will seek out more reviews.