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.

Files: 
CommentFileSizeAuthor
#38 hook_search_page-v1.patch1.15 KBJeff Burnz
PASSED: [[SimpleTest]]: [MySQL] 20,544 pass(es).
[ View ]
#20 810176.patch531 bytesjhodgdon
PASSED: [[SimpleTest]]: [MySQL] 20,477 pass(es).
[ View ]
#17 search-results.patch1.97 KBbleen18
PASSED: [[SimpleTest]]: [MySQL] 20,481 pass(es).
[ View ]
#15 search-results.patch736 bytesbleen18
PASSED: [[SimpleTest]]: [MySQL] 20,475 pass(es).
[ View ]
#12 search-results.patch3.17 KBbleen18
PASSED: [[SimpleTest]]: [MySQL] 20,461 pass(es).
[ View ]
#11 screen-capture-8.png708.64 KBmgifford
#10 search-results.patch2.67 KBbleen18
PASSED: [[SimpleTest]]: [MySQL] 20,472 pass(es).
[ View ]
#3 SearchResultsWithoutPatch.png179.41 KBmgifford
#3 SearchResultsWithHeadingPatch.png194.74 KBmgifford
#2 search_results.patch679 bytesbleen18
PASSED: [[SimpleTest]]: [MySQL] 20,457 pass(es).
[ View ]

Comments

Everett Zufelt’s picture

Category:feature» bug

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.

bleen18’s picture

Status:Active» Needs review
StatusFileSize
new679 bytes
PASSED: [[SimpleTest]]: [MySQL] 20,457 pass(es).
[ View ]
mgifford’s picture

StatusFileSize
new194.74 KB
new179.41 KB

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

Everett Zufelt’s picture

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

Bojhan’s picture

I wouldn't know where else this occurs, but looks good yup.

Everett Zufelt’s picture

Status:Needs review» Reviewed & tested by the community

Doesn't look like this is overridden in core themes. Marking RTBC

Jeff Burnz’s picture

Status:Reviewed & tested by the community» Needs work

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

Everett Zufelt’s picture

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

Jeff Burnz’s picture

Google and Yahoo use an ordered list. Bing uses and unordered list. I think I prefer the Google/Yahoo structure.

bleen18’s picture

Status:Needs work» Needs review
StatusFileSize
new2.67 KB
PASSED: [[SimpleTest]]: [MySQL] 20,472 pass(es).
[ View ]
mgifford’s picture

StatusFileSize
new708.64 KB

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.

bleen18’s picture

StatusFileSize
new3.17 KB
PASSED: [[SimpleTest]]: [MySQL] 20,461 pass(es).
[ View ]

same as #10 except with a minor RTL fix

Dries’s picture

Status:Needs review» Fixed

Looks good. Committed to CVS HEAD. Thanks.

Jeff Burnz’s picture

Status:Fixed» Needs work
+++ 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.

bleen18’s picture

Status:Needs work» Needs review
StatusFileSize
new736 bytes
PASSED: [[SimpleTest]]: [MySQL] 20,475 pass(es).
[ View ]

well spotted

Jeff Burnz’s picture

Status:Needs review» Needs work
+++ 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.

bleen18’s picture

Status:Needs work» Needs review
StatusFileSize
new1.97 KB
PASSED: [[SimpleTest]]: [MySQL] 20,481 pass(es).
[ View ]
jhodgdon’s picture

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.

jhodgdon’s picture

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.

jhodgdon’s picture

StatusFileSize
new531 bytes
PASSED: [[SimpleTest]]: [MySQL] 20,477 pass(es).
[ View ]

Here's a patch for maintainers.txt.

Jeff Burnz’s picture

Status:Needs review» Reviewed & tested by the community

#17, awesome - RTBC.

aspilicious’s picture

Talked to jhodgdon on irc, please commit #20 together with #17, she is very sure about this.

jhodgdon’s picture

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.

Everett Zufelt’s picture

@jhodgdon

Can you expand on your thoughts about why the patch committed is a terrible idea?

Dries’s picture

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.

jhodgdon’s picture

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.

Jeff Burnz’s picture

can't you navigate through the items of the list

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.

putting an H3 header in the middle of a list is VERY weird semantics

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.

This would break any theme that has only overridden one of result/results...

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.

jhodgdon’s picture

Issue tags:+Needs Documentation

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.

Dries’s picture

Status:Reviewed & tested by the community» Needs work

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.

Jeff Burnz’s picture

Status:Needs work» Needs review

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?

Dries’s picture

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.

jhodgdon’s picture

Status:Needs review» Needs work

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.

jhodgdon’s picture

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.

Jeff Burnz’s picture

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

Jeff Burnz’s picture

Status:Needs work» Needs review
jhodgdon’s picture

Status:Needs review» Fixed

Agreed. Someone can now at least look at the markup and see how their CSS needs to change.

Jeff Burnz’s picture

Status:Fixed» Needs work

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

Jeff Burnz’s picture

Status:Needs work» Needs review
StatusFileSize
new1.15 KB
PASSED: [[SimpleTest]]: [MySQL] 20,544 pass(es).
[ View ]

I'll give this a crack now.

jhodgdon’s picture

Status:Needs review» Reviewed & tested by the community

OK.

Dries’s picture

Status:Reviewed & tested by the community» Fixed

Committed to CVS HEAD. Thanks.

Status:Fixed» Closed (fixed)
Issue tags:-Needs usability review, -Usability, -Needs Documentation, -accessibility

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