Follow-up to #2052473: Add aria-label or aria-describedby attributes to all <nav> elements

+++ b/core/includes/theme.inc
@@ -2135,6 +2135,7 @@ function template_preprocess_page(&$variables) {
         'class' => array('visually-hidden'),
+        'attributes' => array('id' => 'links__system_main_menu'),

@@ -2146,6 +2147,7 @@ function template_preprocess_page(&$variables) {
         'class' => array('visually-hidden'),
+        'attributes' => array('id' => 'links__system_secondary_menu'),

We should move the class into the attributes array here as $header['class'] is effectively deprecated for consistency.

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Reroll the patch if it no longer applies. Yes Instructions
Manually test the patch Yes Instructions
Embed before and after screenshots in the issue summary Yes Instructions
CommentFileSizeAuthor
#14 Screen Shot 2014-08-30 at 15.24.09.png189.78 KBlauriii
#12 2285493-class-property-deprecated-12.patch3.63 KBlanchez
#7 2285493-class-property-deprecated-7.patch2.94 KBjoelpittet
#1 2285493-class-property-deprecated-1.patch3.07 KBjoelpittet
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Component: system.module » theme system
Status: Active » Needs review
Issue tags: -Twig +Needs manual testing, +consistency
FileSize
2285493-class-property-deprecated-1.patch3.07 KB

Here's an attempt to remove the class property from links and use attributes.class instead.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Issue tags: +theme system cleanup, +Needs change record

I think this would need a small change record. Looks good at a glance.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Issue tags: -Needs change record

Drafted
https://drupal.org/node/2286175

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii commented
Issue summary: View changes

joelpittet queued 1: 2285493-class-property-deprecated-1.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1: 2285493-class-property-deprecated-1.patch, failed testing.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs work » Needs review
FileSize
2285493-class-property-deprecated-7.patch2.94 KB

Re-roll

peezy’s picture

peezy CreditAttribution: peezy commented

Me and my colleague can't get the patch to apply cleanly, so we're submitting for retesting.

peezy queued 7: 2285493-class-property-deprecated-7.patch for re-testing.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

@peezy seems to still pass and apply here. You are trying to apply it from your drupal root on 8.0.x branch?

This command if you have git checked out 8.0.x branch
curl https://www.drupal.org/files/issues/2285493-class-property-deprecated-7.patch | git apply --index

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Title: Remove deprecated 'class' property from #theme links header. » Remove deprecated 'class' property from #theme 'links' heading array
Status: Needs review » Needs work

When reviewing this more thoroughly I noticed that the menu_tree theme hook has this same pattern. This issue can be expanded to include that or we can create a separate issue to tackle menu_tree. I am leaning towards handling both here and tweaking the change record to include both.

The changes in template_preprocess_page() here are actually for menu_tree render arrays so if this issue ends up being for just #theme links those changes need to be removed from the patch. The changes in \Drupal\system\Tests\Theme\FunctionsTest are good for #theme links though.

lanchez’s picture

lanchez CreditAttribution: lanchez commented
Status: Needs work » Needs review
FileSize
2285493-class-property-deprecated-12.patch3.63 KB

I just removed the backwards compatibility in template_preprocess_menu_tree. Tests seems to be fine in local environment so it might not be used anymore? This was basically what this https://www.drupal.org/node/2310341 issue was aimed for. Lets see what full tests shows.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Related issues: +#2310341: Remove backwards compatibility for $heading['class'] in template_preprocess_menu_tree()

I'm ok with putting them both in this issue. Thanks for patch @lanchez!

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii commented
Status: Needs review » Reviewed & tested by the community
FileSize
Screen Shot 2014-08-30 at 15.24.09.png189.78 KB

Tested this manually and it doesn't break anything.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Title: Remove deprecated 'class' property from #theme 'links' heading array » Remove deprecated 'class' property from #theme 'links' and #theme 'menu_tree' heading arrays

This looks good! Tweaking the title and I updated the change record to encompass both.

Sutharsan’s picture

Sutharsan CreditAttribution: Sutharsan commented

And what about the below code from template_preprocess_link()?

      // @todo Remove backwards compatibility for $heading['class'].
      if (isset($heading['class'])) {
        $heading['attributes']['class'] = $heading['class'];
      }

It was added there in #98696-51: Various bugs in theme_links()

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Reviewed & tested by the community » Needs work

Great, didn't know about that one. Let's grab that here too, then.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii commented
Status: Needs work » Needs review

I'm sorry but am I missing something? isn't that already removed in the patch #12?

Sutharsan’s picture

Sutharsan CreditAttribution: Sutharsan commented
Status: Needs review » Reviewed & tested by the community

@lauriii, you are right. I overlooked it in the patch.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Issue tags: -Needs manual testing

Ah ok, I thought it was link vs. links. Posted from my phone. Carry on :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed b48d098 and pushed to 8.0.x. Thanks!

+++ b/core/modules/system/src/Tests/Theme/FunctionsTest.php
@@ -234,7 +234,11 @@ function testLinks() {
-    $variables['heading'] = array('text' => 'Links heading', 'level' => 'h3', 'class' => 'heading');
+    $variables['heading'] = array(
+      'text' => 'Links heading',
+      'level' => 'h3',
+      'attributes' => array('class' => array('heading')),
+    );

Changes like this are super annoying because I have to work out that actually nothing has changed.

  • alexpott committed b48d098 on 8.0.x 
    Issue #2285493 by joelpittet, lanchez: Remove deprecated 'class'...
hass’s picture

hass CreditAttribution: hass commented

Looks like a good code style bug fix. :-)

Status: Fixed » Closed (fixed)

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