Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
theme_facetapi_link_inactive()andtheme_facetapi_link_active()do similar things but parts of the code are in different order or achieve the same things in different ways.- It's hard to see how
theme_facetapi_link_inactive()andtheme_facetapi_link_active()construct their markup. - There is a
theme_facetapi_deactivate_widget()but not atheme_facetapi_activate_widget() - The deactivate widget is visible to screenreaders.
- The deactivate widget is not wrapped in a span so it can be themed / distinguished from the other parts of the facet link.
- The facet count widget is not wrapped in a span so it can be themed / distinguished from the other parts of the facet link.
- There is a string
t('!facetapi_deactivate_widget !facetapi_accessible_markup')for active links (presumably so it can be re-ordered) but no corresponding one for inactive links.
Proposed resolution
Clean up all these functions. Add a theme_facetapi_activate_widget(). Wrap the widgets in spans so they can be themed.
Remaining tasks
- Write the patch
- Review and RTBC
- Commit
User interface changes
None.
API changes
- API Addition: add activate widgets
- API Addition: spans to make theming easier
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | make_inactive_active-2290031-7.patch | 6.01 KB | mparker17 |
Comments
Comment #1
mparker17Comment #2
mparker17Comment #3
mparker17I've attached a patch. Feedback welcome.
Comment #4
mparker17Noticed an incorrect function docblock.
Comment #5
cpliakas CreditAttribution: cpliakas commentedHi mparker17.
Thanks for the contribution! I am all for improving accessibility as long as we are able to maintain some form of backwards compatibility.
Marking as needs work because it appears that patch no longer applies to HEAD.
Thanks!
Chris
Comment #6
mparker17Sorry for the delay in getting the re-rolled patch to you: it took me a little longer to verify that the re-rolled patch had the same result... apparently the merge conflict was due to #2166095: Inline documentation typo in facetapi.theme.inc, but that one-line-change threw off the diff algorithm and caused it to rearrange the whole patchfile. :P
Straight re-roll, so no interdiff.
This change shouldn't break backwards compatibility:
theme_facetapi_link_inactive()andtheme_facetapi_link_active()used to generate nearly-identical HTML in different ways. This patch fixes it so they generate the same HTML they did before, but in the same way. — EDIT: this is wrong.theme_facetapi_deactivate_widget()now puts an span around the(-)in active links so it can be themed. The active links will look the same, and the span won't show up unless you add CSS to make it do so.theme_facetapi_activate_widget()puts an empty span in inactive links (in the same place that the deactivate widget goes in active links) so it can be themed. The inactive links will look the same, and the span won't show up unless you add CSS to make it do so.Comment #7
mparker17Adding a class to the activate and deactivate links so it's easier to write CSS for them.
Comment #8
mparker17I was wrong. Active links now look like inactive links, i.e.: the format is
<a>{term} {accessible_markup} ({count}) {widget}</a>where the widget is an empty span with it's own theming function so it can be modified further down the theming layer.Comment #9
mgifford