Posted by YaxBalamAhaw on May 26, 2010 at 6:48pm
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | search.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | accessibility, Needs Documentation, Needs usability review, Usability |
Issue Summary
This seems to be the standard for search engine results (see Google, Yahoo, Bing). It allows for easy keyboard navigation with accessibility software. Right now the output is like this:
<dt class="title">
<a href="http://dev.drupal7/node/23">Ratis Iriure</a>
</dt>Something like this would be better:
<dt class="title">
<h3><a href="http://dev.drupal7/node/23">Ratis Iriure</a></h3>
</dt>Instead of tabbing through all the links, users of accessibility software could quickly skip directly from heading to heading. This is related to WCAG 2 2.4.6.
Comments
#1
Agreed. Setting as bug as this is a suggested success criteria for meeting WCAG 2.0 level AA.
Perhaps someone can role a patch for this and make sure it isn't overridden in any core themes.
#2
#3
This looks good to me. I applied it here with some sample search data:
http://drupal7.dev.openconcept.ca/search/node/magna
It would be great if this got into core. There may be some theming
I've attached the screenshots as there are some differences in styling. I prefer the H3 version, but...
#4
If we are certain that no core theme overrides this tpl and if there is no theming issue then I think that this is RTBC.
Tagging with needs usability review
#5
I wouldn't know where else this occurs, but looks good yup.
#6
Doesn't look like this is overridden in core themes. Marking RTBC
#7
According to the W3C this is invalid. Also the semantics of it irk me, which are dubious to begin with (use of the definition list for search results).
#8
@Jeff
Thanks for pointing out the validity issue here. I would tend to agree that search results aren't a definition list. They are however a list.
I checked the spec and dt elements can only contain inline elements.
So, should we switch the search results to a different type of list?
#9
Google and Yahoo use an ordered list. Bing uses and unordered list. I think I prefer the Google/Yahoo structure.
#10
#11
OK, I just applied patch #10 here:
Multiple results:
http://drupal7.dev.openconcept.ca/search/node/occuro
Single result:
http://drupal7.dev.openconcept.ca/search/node/Blanditsimilismetuo
Both of these were validated via the w3c: "This document was successfully checked as XHTML + RDFa!"
I've attached a screenshot (one in Hebrew) with all major Mac browsers.
#12
same as #10 except with a minor RTL fix
#13
Looks good. Committed to CVS HEAD. Thanks.
#14
+++ modules/search/search-result.tpl.php 28 May 2010 03:05:43 -0000@@ -46,14 +46,16 @@
+ <?php if ($info) : ?>
+ <p class="search-info"><?php print $info; ?></p>
+ <?php endif; ?>
Indentation?
Powered by Dreditor.
#15
well spotted
#16
+++ modules/search/search-result.tpl.php 28 May 2010 03:05:43 -0000@@ -46,14 +46,16 @@
+ <div class="search-info-wrap">
Dont want to start a bikeshed but if we follow other core examples for class naming this would be "search-snippet-info". Lets not bikeshed eh fellas? But "wrap" seems out of place.
(sorry bleen, I just spotted this after your latest patch!).
Powered by Dreditor.
#17
#18
I can't really believe all of the above was committed without any consultation of the search module maintainers.
I am not even going to say any more. This was quick and we are both fairly busy and no one even pinged us and it all happened in less than 1 day. Sigh.
#19
I would also just like to say that we had an issue about this a WHILE back and it was pushed off to D8 as being too major of a change.
#20
Here's a patch for maintainers.txt.
#21
#17, awesome - RTBC.
#22
Talked to jhodgdon on irc, please commit #20 together with #17, she is very sure about this.
#23
Just to clarify:
I think the patch committed above was a terrible idea.
I have been increasingly frustrated by the lack of any process/consistency/policy in D7 dev. I am not willing to continue to be the official maintainer of the search module.
I plan to continue to contribute patches to D7 and in particular to the search module. And to review patches. I just don't want to be listed as the official maintainer of the search module.
Thanks.
#24
@jhodgdon
Can you expand on your thoughts about why the patch committed is a terrible idea?
#25
jhodgdon, I think you are exaggerating when you wrote "terrible idea".
1. It is perfectly OK to commit patches in one day.
2. There has never been a rule that _requires_ consultation of module maintainers (and there never will be).
3. This patch didn't change the search module internals or algorithms; it changed the generated HTML to improve accessibility. In my mind, this patch has almost nothing to do with search module. Furthermore, the accessibility changes were verified by a couple of accessibility experts, is based of Google/Yahoo's approach, and passed W3C validation.
4. There only seems to be disagreement on the names of the CSS classes -- that is hardly a "terrible idea" in my book.
5. It is perfectly OK to rollback a patch that was a terrible idea. That is not a shame. We can talk about it.
Maybe we missed or overlooked something? I, too, would like to understand why this patch was a terrible idea.
#26
Sorry, I probably overreacted in saying it was a terrible idea. I have been rather (understatement) stressed out today, and I apologize. I'm also sorry that I have been too busy with client/paying work the last few days to have noticed this very quick patch turn-around, so that I could have commented before it was committed.
So, to address the patch itself.
I am not against accessibility improvements, of course, but I think this particular one was misguided. There is already an H2 heading before the list of search results. It's the header for a list (in this case, before the patch, a DL list). Then what follows is a list -- can't you navigate through the items of the list? To me, putting an H3 header in the middle of a list is VERY weird semantics. It may help screen readers bounce through the page, but it isn't semantically correct in any way that I can understand. Headers are supposed to be section headers, not nested within LI items in a list. It just seems wrong, and I don't see the WCAG guidelines actually suggesting this practice.
The other reason I am unhappy with the committed patch is that I think that changing the list from a DL to an OL was a bad idea at this stage of the D7 process. This would break any theme that has only overridden one of result/results (because they were assuming one structure and it has changed in an incompatible way). Also, the markup changed completely, so any theme that had CSS customizations for search results will have to redo them. It is not really all that minor a change to the search module from the point of view of a theme -- it is a complete change in the default search results presentation.
At a minimum, it needs to be documented in the theme update guide - has someone done that? If this patch stays, it needs to be done.
#27
Not via headings since there are no heading elements to jump through, this is what the whole focus of the patch is about, providing AT users with heading based navigation. Please read up about this if you are unsure what this is.
H3 follows H2 and is correctly nested in this context. LI is merely a generic container, somewhat like a DIV but with different semantics (its a list-item as opposed to a division) - each item can have a heading, divisions, paragraphs, images or whatever. There is nothing wierd about this at all and is a common everyday usage of the LI, its valid and in use by the major search engines.
No core themes do this. If others are building D7 themes and making assumptions before a stable release that's not really our problem. Its not our job to account for the crystal ball gazers. In any case, as a themer speaking here, the changes are quite trivial and I was able to write a patch for Corolla in about 1 minute.
When the patch in #17 lands we'll switch to a documentation issue and write the update.
#28
OK. Just someone PLEASE put this into the theme update guide, once the follow-up patch has been done, and please don't mark this patch Fixed until that has been done. Thanks.
There's no need to change this to a doc component issue. Just mark it "needs work" and I've already added the "needs doc" tag.
#29
jhodgdon -- no worries, your apologies are obviously accepted. Happy to see we can have a constructive dialogue.
I committed the patch in #17 but not the patch in #20. The patch in #17 is a clean-up so it does make things regress.
Setting this to 'needs work' for the documentation updates and further discussion.
#30
Documentation added: http://drupal.org/update/theme/6/7#search-result-headings
Could someone please review the docs, won't set as fixed just yet.
The patch in #20 should perhaps be in its own issue?
#31
Re: #20: I'm hopping jhodgdon wants to continue to be a co-maintainer of the search module. jhodgdon, do you still want me to commit that patch? Let's hope not.
#32
I don't have any intention of changing the way/amount I am contributing to Drupal, even if I remove my name as co-maintainer of the search module as a formal protest and expression of my long-term extreme frustration around the lack of consistency in, transparency in, and existence of the process and standards that are used to decide which patches get committed to Drupal.
But this issue is not the place to discuss that (I am not sure what is, but this issue isn't), so let's leave it at that for now.
Regarding #30:
- Both search-result and search-results TPL files changed. You only mentioned search-result.
- CSS changes will be needed, as the HTML markup and classes have changed for both result and results. It would be useful to mention the CSS classes/IDs that no longer exist, and the new ones.
#33
Oh, and one other thing: It would probably be helpful to include a before/after (D6 vs. D7) for the entire search results/result so they could easily be compared.
#34
Updated the docs as per #32, #33 - good point about adding the info on the other template and the D6 stuff.
Pretty reluctant to add much if anything about CSS as this could get long winded. I think its pretty evident to themers that they may need to make CSS updates and we can't really predict what they might be - really depends if they styled using element selectors or class selectors (if the latter they might not need to do much at all, as in Corolla - it was very easy since we were using class name selectors).
#35
#36
Agreed. Someone can now at least look at the markup and see how their CSS needs to change.
#37
We need to update hook_search_page also, and document this both in new comments and the theme changes page, currently this is:
http://api.drupal.org/api/function/hook_search_page/7
#38
I'll give this a crack now.
#39
OK.
#40
Committed to CVS HEAD. Thanks.
#41
Automatically closed -- issue fixed for 2 weeks with no activity.