Change active HTML class to is-active according new CSS architecture (for Drupal 8).

This change requires change in the functions as well as in all CSS files where active class is styled.

Related issues

#1979468: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug/Task/Feature because ...
Issue priority Major because ... Critical/Not critical because ...
Unfrozen changes Unfrozen because it only changes markup and CSS
CommentFileSizeAuthor
#174 Screenshot 2015-04-16 16.07.24.jpg654.78 KBLewisNyman
#173 change_active_class_to-2031641-173.patch49.87 KBnlisgo
#173 interdiff-2031641-164-173.txt564 bytesnlisgo
#171 admin_content_sort.gif69.97 KBti2m
#3 2031641-changeactiveclass-2.patch537 bytesaburrows
#4 2031641-changeactiveclass-3.patch727 bytesaburrows
#8 2031641-changeactiveclass-7.patch24.36 KBaburrows
#11 2031641-changeactiveclass-10.patch29.35 KBaburrows
#14 2031641-changeactiveclass-13.patch49.12 KBaburrows
#17 2031641-changeactiveclass-16.patch44.58 KBaburrows
#19 2031641-changeactiveclass-17.patch46.38 KBaburrows
#21 interdiff.txt1 KBPol
#21 2031641-changeactiveclass-18.patch46.99 KBPol
#23 interdiff.txt1.13 KBPol
#23 2031641-changeactiveclass-19.patch1.13 KBPol
#24 interdiff.txt1.13 KBPol
#24 2031641-changeactiveclass-20.patch46.39 KBPol
#26 2031641-changeactiveclass-21.patch47.08 KBPol
#30 active-css-class-change-2031641-29.patch47.52 KBaburrows
#32 active-css-class-change-2031641-32.patch47.72 KBRainbowArray
#48 active-css-class-change-2031641-48.patch24.27 KBaburrows
#49 active-css-class-change-2031641-49.patch27.06 KBaburrows
#52 active-css-class-change-2031641-52.patch26.99 KBaburrows
#53 gray.PNG13.97 KBdarol100
#54 active-css-class-change-2031641-54.patch27.38 KBaburrows
#63 active-css-class-change-2031641-61.patch28.87 KBaburrows
#63 admin_reports_states.png33.63 KBaburrows
#63 views_tour.png188.22 KBaburrows
#63 toolbar_install_theme.png55.66 KBaburrows
#63 active-css-class-change-2031641-63.patch28.87 KBaburrows
#67 active-css-class-change-2031641-67.patch25.78 KBlauriii
#71 active-css-class-change-2031641-71.patch14.97 KBredsquid
#73 active-css-class-change-2031641-73.patch0 bytesredsquid
#74 active-css-class-change-2031641-74.patch49.36 KBredsquid
#76 active-css-class-change-2031641-76.patch25.9 KBtuutti
#78 Screenshot 2014-09-05 17.35.28.jpg401.79 KBLewisNyman
#78 Screenshot 2014-09-05 17.35.35.jpg447.52 KBLewisNyman
#78 Screenshot 2014-09-05 17.35.43.jpg597.43 KBLewisNyman
#78 Screenshot 2014-09-05 17.36.09.jpg510.35 KBLewisNyman
#81 active-css-class-change-2031641-81.patch26.59 KBLewisNyman
#82 active-css-class-change-2031641-82.patch26.07 KBtuutti
#88 active-css-class-change-2031641-88.patch26.31 KBaburrows
#90 Screen Shot 2014-10-03 at 14.53.38.png322.88 KBpaulhhowells
#91 Screen Shot 2014-10-03 at 15.39.33.png725.73 KBpaulhhowells
#92 Screen Shot 2014-10-03 at 15.48.02.png271.46 KBpaulhhowells
#98 active-css-class-change-2031641-97.patch26.31 KBlauriii
#101 active-css-class-change-2031641-101.patch29.72 KBlauriii
#103 active-css-class-change-2031641-103.patch29.88 KBlauriii
#103 noob.png32.33 KBlauriii
#105 active-css-class-change-2031641-105.patch26.48 KBlauriii
#105 interdiff-2031641-103-105.txt3.08 KBlauriii
#109 active-css-class-change-2031641-109.patch26.81 KBkallehauge
#109 interdiff.txt2.24 KBkallehauge
#116 interdiff.txt5.92 KBlauriii
#116 active-css-class-change-2031641-115.patch25.02 KBlauriii
#118 change-active-is-active-2031641-118.patch4.97 KBbrahmjeet789
#121 active-css-class-change-2031641-121.patch25.17 KBsaki007ster
#127 active-css-class-change-2031641-127.patch45.3 KBeldrupalista
#127 interdiff-2031641-121-127.txt19.71 KBeldrupalista
#130 active-css-class-change-2031641-130.patch46.35 KBlauriii
#130 interdiff.txt920 byteslauriii
#132 active-css-class-change-2031641-132.patch44.85 KBlauriii
#132 interdiff.txt2.26 KBlauriii
#135 2031641-is-active-class.patch56.29 KBRavindraSingh
#145 2031641-change-active-to-is-active-145.patch43.71 KBakalata
#145 interdiff-20316141-132-145.txt2.74 KBakalata
#147 2031641-interdiff.txt1.82 KBrpayanm
#147 2031641-147.patch45.57 KBrpayanm
#149 interdiff-2031641-147-149.txt6.42 KBnlisgo
#149 change_active_class_to-2031641-149.patch51.23 KBnlisgo
#150 interdiff-2031641-149-150.txt875 bytesnlisgo
#150 change_active_class_to-2031641-150.patch51.36 KBnlisgo
#152 change_active_class_to-2031641-152.patch47.31 KBnlisgo
#154 interdiff-2031641-152-154.txt1.66 KBnlisgo
#154 change_active_class_to-2031641-154.patch46.57 KBnlisgo
#157 interdiff-2031641-152-157.txt615.7 KBnlisgo
#157 change_active_class_to-2031641-157.patch46.57 KBnlisgo
#159 interdiff-2031641-157-159.txt672 bytesnlisgo
#159 change_active_class_to-2031641-159.patch45.92 KBnlisgo
#164 change_active_class_to-2031641-164.patch49.32 KB_nolocation
#164 interdiff.txt4.05 KB_nolocation
#167 162-164-3_views_summary_unformatted.png119.79 KBakalata
#167 162-164-2_views-summary.png107.7 KBakalata
#167 162-164-1_toolbar_resize_after-162.png29.19 KBakalata
#167 162-164-1_toolbar_resize_after-159.png31.96 KBakalata
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

aburrows’s picture

Assigned: Unassigned » aburrows
LewisNyman’s picture

Assigned: aburrows » Unassigned
Issue tags: +CSS, +d8mux-css-cleanup

taggin

aburrows’s picture

Assigned: Unassigned » aburrows
Status: Active » Needs review
FileSize
537 bytes

New 'is-active' class added

aburrows’s picture

Modified comment to suit new class name

Dragan Eror’s picture

@aburrows Thanks for this awesome fast replay and patch :)

This is a nice change to l() function, but this issue requires a little bit more changes in all CSS files in core which contains .active related styles and should be checked carefully which means Find & Replace can NOT be used.

This change requires change on function it self as well as in all CSS files where active link is styled.

Dragan Eror’s picture

@aburrows Thanks for this awesome fast replay and patch :)

This is a nice change to l() function, but this issue requires a little bit more changes in all CSS files in core which contains .active related styles and should be checked carefully which means Find & Replace can NOT be used.

This change requires change on function it self as well as in all CSS files where active link is styled.

aburrows’s picture

Just about to submit with all the css changes done

aburrows’s picture

Patch with all css class name changes.

Status: Needs review » Needs work

The last submitted patch, 2031641-changeactiveclass-7.patch, failed testing.

LewisNyman’s picture

When applying I get the error:

Checking patch core/themes/seven/style.css...
error: while searching for:
  margin: 0 2px;
}
ul.primary li a:link,
ul.primary li a.active,
ul.primary li a:active,
ul.primary li a:visited,
ul.primary li a:hover,
ul.primary li.active a {
  display: block;
  float: left; /* LTR */
  padding: 0.615em 18px;

error: patch failed: core/themes/seven/style.css:244
aburrows’s picture

Patch with additions of menu.inc added

LewisNyman’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 2031641-changeactiveclass-10.patch, failed testing.

aburrows’s picture

Status: Needs work » Needs review
FileSize
49.12 KB

This should now contain all the amended items

Status: Needs review » Needs work

The last submitted patch, 2031641-changeactiveclass-13.patch, failed testing.

aburrows’s picture

I need to get the latest HEAD and that should solve it.

aburrows’s picture

Status: Needs work » Needs review
FileSize
44.58 KB

Patch is now working with latest head, I though I had latest HEAD but didn't. Now I do and just tested locally and it applied fine.

Status: Needs review » Needs work

The last submitted patch, 2031641-changeactiveclass-16.patch, failed testing.

aburrows’s picture

Status: Needs work » Needs review
FileSize
46.38 KB

This hopefully should stop the failing tests, hopefully in time before core updates

Status: Needs review » Needs work

The last submitted patch, 2031641-changeactiveclass-17.patch, failed testing.

Pol’s picture

Status: Needs work » Needs review
FileSize
1 KB
46.99 KB

Status: Needs review » Needs work

The last submitted patch, 2031641-changeactiveclass-18.patch, failed testing.

Pol’s picture

Status: Needs work » Needs review
FileSize
1.13 KB
1.13 KB
Pol’s picture

Oops, my bad.

Status: Needs review » Needs work

The last submitted patch, 2031641-changeactiveclass-20.patch, failed testing.

Pol’s picture

Status: Needs work » Needs review
FileSize
47.08 KB

Sorry for the noise, I had a problem with my patches!

Dragan Eror’s picture

@aburrows, @Pol I am so happy to see such a contribution from you on this really new task. Already see results from this soon :)

Can you guys please follow the patch naming convention and help core maintainer to not become confused?

aburrows’s picture

@Dragan Eror sorry got into bad practice with that, can't remember where I saw it

Status: Needs review » Needs work

The last submitted patch, 2031641-changeactiveclass-21.patch, failed testing.

aburrows’s picture

Status: Needs work » Needs review
FileSize
47.52 KB

Status: Needs review » Needs work

The last submitted patch, active-css-class-change-2031641-29.patch, failed testing.

RainbowArray’s picture

Assigned: aburrows » Unassigned
Status: Needs work » Needs review
FileSize
47.72 KB

Here's a reroll of this patch.

15 files with merge conflicts! Not surprising after a few months.

What I haven't done is to go through all of Drupal core to see if there are any new files that might have the active class which needs to be changed to is-active. Hope this serves as an updated patch to move forward with that work.

Status: Needs review » Needs work

The last submitted patch, active-css-class-change-2031641-32.patch, failed testing.

LewisNyman’s picture

I've found a few more mentions of class 'active' in:

  • LinkGenerator.php
  • LinkGeneratorTest.php
  • LocalTaskDefault.php
  • UrlTest.php
  • toolbar.js
aburrows’s picture

@Lewis, is this issue still outstanding? If so assign to me and i'll sort it tonight.

LewisNyman’s picture

Issue summary: View changes
Issue tags: +frontend, +dreammarkup, +Needs reroll

@aburrows Looks like I missed this. I'm not sure if this should be moved to 9.x?

aburrows’s picture

Yeah I think 9.x would be best because its going to cause to many issues for 8.x

RainbowArray’s picture

We're just changing a name of a class. Yes, there are a lot of places where it's changed, but is it really that much of a change that we need to push this fix out for 3-5 years from now?

Anonymous’s picture

Version: 8.x-dev » 9.x-dev
Status: Needs work » Postponed

After chatting with Lewis, it seems like this could be a really far-reaching change, even though on the surface it's trivial. Tentatively postponing to 9.x...

LewisNyman’s picture

This change will affect every theme. It's a pretty big API change for themers. It feels like more trouble than it's worth.

LewisNyman’s picture

Status: Postponed » Needs work

Morten told me that Alex Pott told him that markup freeze is release candidate 1. So we still have time! We can do this!

LewisNyman’s picture

Version: 9.x-dev » 8.x-dev
aburrows’s picture

How much time do we have?

Dragan Eror’s picture

@aburrows, release candidate 1 means 4-6 months at least. Currently it's not even beta...

aburrows’s picture

ok cool ill get this done tomorrow when im mentoring, if i have issues ill speak to alexpott about what it will affect :P

aburrows’s picture

Assigned: Unassigned » aburrows
LewisNyman’s picture

Issue tags: +focus

Let's get this through in Austin

aburrows’s picture

Status: Needs work » Needs review
FileSize
24.27 KB

Heres the latest patch that changes .active to .is-active

aburrows’s picture

Fixed installation issue for is-active

aburrows’s picture

Assigned: aburrows » Unassigned
Issue tags: -Needs reroll
darol100’s picture

I test your patch on SimplyTest.me And the toolbar doesn't work.

aburrows’s picture

Assigned: Unassigned » aburrows
FileSize
26.99 KB

Patch fixed was a JS issue

darol100’s picture

FileSize
13.97 KB

The toolbar have a different problem now. When you click in the toolbar a tab you can't go back to a the first tab. You can tell by the color gray on the toolbar.
Toolbar error

aburrows’s picture

Cheers darol100 I've fixed the js issue now please test.

darol100’s picture

The toolbar is working fine. Also, I revise your patch seem to be working fine.

darol100’s picture

Status: Needs review » Reviewed & tested by the community
aburrows’s picture

Assigned: aburrows » Unassigned
alexpott’s picture

Issue tags: +Needs change record

Can we get a change record before commit

aburrows’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
this.$el.find('button').toggleClass('active', !this.model.get('isViewing'));

/Volumes/devdisk/dev/drupal/core/modules/contextual/js/toolbar/views/VisualView.js

        .toggleClass('active', isActive)

/Volumes/devdisk/dev/drupal/core/modules/tour/js/tour.js

      if (steps[i].className === 'active') {
        return i + 1;
      }

/Volumes/devdisk/dev/drupal/core/themes/seven/js/mobile.install.js

Also can we have some screenshots proving that nothing is borked

lauriii’s picture

Assigned: Unassigned » lauriii
lauriii’s picture

Assigned: lauriii » Unassigned
Status: Needs work » Needs review
FileSize
28.87 KB
1.5 KB
266.88 KB

Here's a patch for the points that Alex pointed. I have also attached some pictures from the places where active class has been changed to is-active.

aburrows’s picture

I noticed a few more places missing it, so heres a complete patch with the other places that were missing it

aburrows’s picture

Assigned: Unassigned » aburrows
aburrows’s picture

aburrows’s picture

Assigned: aburrows » Unassigned
lauriii’s picture

Rerolled the patch #63

aburrows’s picture

Is this going to have to be rerolled again? So we can keep up with latest release?

lauriii’s picture

Issue tags: +Needs reroll
redsquid’s picture

Assigned: Unassigned » redsquid
Issue tags: +FUDK
redsquid’s picture

Rerolled the patch #68. My first

Status: Needs review » Needs work

The last submitted patch, 71: active-css-class-change-2031641-71.patch, failed testing.

redsquid’s picture

Assigned: redsquid » Unassigned
Status: Needs work » Needs review
FileSize
0 bytes

Ups. Wrong branche.

redsquid’s picture

Not so fast. Third time... maybe

star-szr’s picture

Status: Needs review » Needs work

Thanks @redsquid! The patch almost doubled in size from #67 to #74 so I think this needs another look.

tuutti’s picture

Rerolled #67 patch

tuutti’s picture

Status: Needs work » Needs review
LewisNyman’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
401.79 KB
447.52 KB
597.43 KB
510.35 KB

I manually test this patch and couldn't find any visual regressions in Seven or Bartik. I've attached a few screenshots.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 76: active-css-class-change-2031641-76.patch, failed testing.

joelpittet’s picture

Issue tags: +Needs reroll
LewisNyman’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
26.59 KB
tuutti’s picture

patching file core/includes/menu.inc
Hunk #1 FAILED at 411.
1 out of 1 hunk FAILED -- saving rejects to file core/includes/menu.inc.rej
patching file core/themes/seven/seven.theme
Hunk #1 FAILED at 107.
1 out of 1 hunk FAILED -- saving rejects to file core/themes/seven/seven.theme.rej

Needed reroll again because of #1898478: menu.inc - Convert theme_ functions to Twig

aburrows’s picture

Whats the status of this, as ill re-roll and re-patch

joelpittet’s picture

@aburrows it certainly needs a re-roll, could you please?

Also if you do, it would be great to find someone on IRC or at drupalcon to partner up to review your re-roll so this can get RTBC'd

aburrows’s picture

It will be completed today

aburrows’s picture

Assigned: Unassigned » aburrows
aburrows’s picture

Status: Needs review » Needs work

working on this now

aburrows’s picture

aburrows’s picture

Status: Needs work » Needs review
paulhhowells’s picture

* Applied the patch.
* Selected the Admin menu, and
* observed that a class of .is-active was applied, and
* observed that a class of .active was no longer applied:

paulhhowells’s picture

* Applied the patch
* Navigated to /update.php
* Observed that a class of .is-active was applied to 'Overview' within the .task-list:

paulhhowells’s picture

* Applied the patch
* Navigated to /admin/config
* Observed that a class of .is-active was applied to 'Configuration':

paulhhowells’s picture

* Applied the patch
* Navigated to /admin/theme/install
* Observed that a class of .is-active was applied to the 'Update' tab:

Only local images are allowed.

paulhhowells’s picture

Assigned: aburrows » Unassigned
Status: Needs review » Reviewed & tested by the community
emma.maria’s picture

I was the mentor that helped @paulhhowells to review this patch.

- We observed no JS errors that previous reviews above found.
- None of the UI had visually broken parts.
- The console returned no errors.
- The .is-active class is now present in the markup, .active is gone.

I agree with RTBC decision.

Wim Leers’s picture

RTBC+1, patch looks great!


+++ b/core/includes/menu.inc
@@ -329,7 +329,7 @@ function template_preprocess_menu_local_task(&$variables) {
-    $variables['attributes']['class'] = array('active');
+    $variables['attributes']['class'] = array('is-active');

+++ b/core/includes/tablesort.inc
@@ -50,7 +50,7 @@ function tablesort_header(&$cell_content, array &$cell_attributes, array $header
-      $cell_attributes['class'][] = 'active';
+      $cell_attributes['class'][] = 'is-active';

+++ b/core/includes/theme.inc
@@ -1371,7 +1371,7 @@ function template_preprocess_table(&$variables) {
-              $cell_attributes['class'][] = 'active';
+              $cell_attributes['class'][] = 'is-active';

@@ -1562,7 +1562,7 @@ function template_preprocess_task_list(&$variables) {
-      $variables['tasks'][$k]['attributes']->addClass('active');
+      $variables['tasks'][$k]['attributes']->addClass('is-active');

I'd argue these should be converted to use data-drupal-link-system-path, so that the is-active class can be added by the JS.

But, that's of course very much out of scope here. Perhaps that'd be a nice next issue to work on for paulhhowells? :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 88: active-css-class-change-2031641-88.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
26.31 KB

Rerolled

chx’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 98: active-css-class-change-2031641-97.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
29.72 KB

Rerolled again

Status: Needs review » Needs work

The last submitted patch, 101: active-css-class-change-2031641-101.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
29.88 KB
32.33 KB

Fabianx’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Template/Attribute.php
@@ -161,6 +161,48 @@ public function addClass() {
+  public function setAttribute($attribute, $value) {
+    // If attribute key exists we can set attribute.
+    if ($attribute) {
...
+  public function removeAttribute() {
+    $args = func_get_args();
+    foreach ($args as $arg) {

Not that I am opposed to it, but I think those functions sneaked in lazily from another path. I very much assume they did hide themselves in a LazyBeanBagCollection so that you could not see them :-D.

lauriii’s picture

Status: Needs work » Needs review
FileSize
26.48 KB
3.08 KB

Removed that stuff.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Looks great to me!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

This conflicts with #2329767: Move table classes from preprocess to templates - the interesting thing is why with an issue titled "Change active class in l() function to is-active" are we changing the active class on the tablesort indicator? This is not the active link.

Wim Leers’s picture

Sorry, just one stupid nitpick:

+++ b/core/includes/theme.inc
--- a/core/misc/active-link.js
+++ b/core/misc/active-link.js

Please also update the docblock for Drupal.behaviors.activeLinks.


AFAICT this was manually tested in #90–#95, in which case RTBC+1.

kallehauge’s picture

  1. Changed the docblock for Drupal.behaviors.activeLinks
  2. Removed the table-related code commented by @alexpott at comment #107 regarding issue https://www.drupal.org/node/2329767
  3. Not all changes would be applied in "Seven" tabs.css after https://www.drupal.org/node/2360069 . I also redid this.

The code inside the preprocess_table function (#107) doesn't show up in the interdiff since it could not be applied with the previous patch, anymore (#105)

kallehauge’s picture

Status: Needs work » Needs review
lauriii’s picture

Status: Needs review » Reviewed & tested by the community

Patch fixes #107 and #108

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/tablesort.inc
@@ -50,7 +50,7 @@ function tablesort_header(&$cell_content, array &$cell_attributes, array $header
-      $cell_attributes['class'][] = 'active';
+      $cell_attributes['class'][] = 'is-active';

+++ b/core/modules/system/css/system.theme.css
@@ -13,10 +13,10 @@
 /**
  * Markup generated by theme_tablesort_indicator().
  */
-th.active img {
+th.is-active img {
   display: inline;
 }
-td.active {
+td.is-active {
   background-color: #ddd;
 }

+++ b/core/themes/seven/css/components/tables.css
@@ -66,38 +66,38 @@ th > a:after {
-th.active > a {
+th.is-active > a {
   color: #004875;
 }
-th.active img {
+th.is-active img {
   position: absolute;
   right: 0; /* LTR */
   top: 50%;
 }
-[dir="rtl"] th.active img {
+[dir="rtl"] th.is-active img {
   right: auto;
   left: 0;
 }
-th.active > a:after {
+th.is-active > a:after {
   border-bottom-color: #004875;
 }
 th > a:hover,
 th > a:focus,
-th.active > a:hover,
-th.active > a:focus {
+th.is-active > a:hover,
+th.is-active > a:focus {
   color: #008ee6;
   text-decoration: none;
 }
 th > a:hover:after,
 th > a:focus:after,
-th.active > a:hover:after,
-th.active > a:focus:after {
+th.is-active > a:hover:after,
+th.is-active > a:focus:after {
   border-bottom-color: #008ee6;
 }
 td .item-list ul {
   margin: 0;
 }
-td.active {
+td.is-active {

This is a table cell class not a link class. This issue is about link classes. And it's inconsistent with the current table.html.twig template.

        {% for cell in header %}
          {%
            set cell_classes = [
              cell.active_table_sort ? 'active',
            ]
          %}
          <{{ cell.tag }}{{ cell.attributes.addClass(cell_classes) }}>
            {{- cell.content -}}
          </{{ cell.tag }}>
        {% endfor %}

This why this issue has taken so long to get done - because it has not stuck to the scope outlined by the issue title Change active class in l() function to is-active. This has nothing to do with tablesorts.

LewisNyman’s picture

Title: Change active class in l() function to is-active » Change active class to is-active
Issue summary: View changes
Status: Needs work » Reviewed & tested by the community

Ah that's right, but we do need to change the active class in the whole of core, not just when added in the l() function. I think when this issue was written the assumption was that it was only appeared in the l() function, but this is not the case.

So we could either:

1. Move the code for the table sort component out of this issue into a follow up.
2. Change the scope and summary of the issue to reflect the current patch, which is in line with the standards.

If we go with two then that means I can set the most recent patch to RTBC, and it saves time and effort.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

This patch will break tablesort theming then - since table.twig.html still uses the active class.

alexpott’s picture

And the draft change record needs updating too then.

lauriii’s picture

Status: Needs work » Needs review
FileSize
5.92 KB
25.02 KB

Removed table related changes. Some other changes because of hunks have changed a bit.

LewisNyman’s picture

Status: Needs review » Needs work

We shouldn't remove the changes, this needs to be consistent. We should make the change the table.twig.html to fix this instead.

brahmjeet789’s picture

I have changed the class active to is-active from table.twig.html and also some more changes

lauriii’s picture

Status: Needs work » Needs review
Issue tags: -FUDK +dcdelhi

Lets move the issues status to Needs review so the testbot triggers. I also added the sprint tag for this issue

LewisNyman’s picture

Status: Needs review » Needs work

It looks like the most recent patch does not include the changes of the previous patch?

saki007ster’s picture

Merged the patches in #116 and #118.

saki007ster’s picture

Status: Needs work » Needs review
saki007ster’s picture

Also tested this the classes have been changed to 'is-active' from 'active'.

akalata’s picture

alexpott’s picture

Status: Needs review » Needs work
  1. .toolbar .toolbar-tray-vertical.active,
    

    Is in toolbar.module.css

  2.   .featured .region-primary-menu .menu li a.active {
    

    is in bartik's media.css

  3. .region-primary-menu .menu li a.active {
    

    is in bartik's primary-menu.css

  4. There are 4 usages of .active in bartik's tabs.css
  5. There are 9 usages of .active in seven's tables.css

This is just the result of a quick search - we need to ensure we've caught everything here...

alexpott’s picture

For example...

  1. $cell_attributes['class'][] = 'active';
    

    in tablesort.inc

  2. $options['attributes']['class'][] = 'active';
    

    in LocalTaskDefault.php

  3.       // Only if the the path, the language and the query match, we set the
          // "active" class.
          if ($is_active) {
            $class = $node->getAttribute('class');
            if (strlen($class) > 0) {
              $class .= ' ';
            }
            $class .= 'active';
    

    SystemController.php

  4. There are a whole heap of tests for the active class being added to links - they will need changing.
  5. $variables['rows'][$id]->attributes['class'][] = 'active';
    

    There a couple of instances of this in views.theme.inc

eldrupalista’s picture

Hi, added all the ones listed by @alexpott and a few others.

eldrupalista’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 127: active-css-class-change-2031641-127.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
46.35 KB
920 bytes

Lets see if the tests pass now

joelpittet’s picture

Status: Needs review » Needs work

Just a quick run through and found a strange one:

  1. +++ b/core/modules/language/src/Tests/LanguageSwitchingTest.php
    @@ -129,34 +129,34 @@ protected function doTestLanguageBlockAnonymous($block_label) {
    -      'active' => array(),
    +      'is-active' => array(),
    ...
    -      'active' => array(),
    +      'is-active' => array(),
    

    This doesn't look like classes, are you sure this is right?

  2. +++ b/core/modules/language/src/Tests/LanguageSwitchingTest.php
    @@ -129,34 +129,34 @@ protected function doTestLanguageBlockAnonymous($block_label) {
    -    $this->assertIdentical($links, array('active' => array('en'), 'inactive' => array('fr')), 'Only the current language list item is marked as active on the language switcher block.');
    -    $this->assertIdentical($anchors, array('active' => array('en'), 'inactive' => array('fr')), 'Only the current language anchor is marked as active on the language switcher block.');
    +    $this->assertIdentical($links, array('is-active' => array('en'), 'inactive' => array('fr')), 'Only the current language list item is marked as active on the language switcher block.');
    +    $this->assertIdentical($anchors, array('is-active' => array('en'), 'inactive' => array('fr')), 'Only the current language anchor is marked as active on the language switcher block.');
    

    Same here this doesn't look like classes.

  3. +++ b/core/modules/system/src/Controller/SystemController.php
    @@ -429,7 +429,7 @@ public static function setLinkActiveClass(array $element, array $context) {
    -        $class .= 'active';
    +        $class .= 'is-active';
    

    This should get a follow-up issue so that it's an array like the rest of core.

lauriii’s picture

Status: Needs work » Needs review
FileSize
44.85 KB
2.26 KB

Good catches, I fixed #131.1-2

joelpittet’s picture

joelpittet’s picture

Status: Needs review » Needs work
  1. +++ b/core/misc/active-link.js
    @@ -16,7 +16,7 @@
    -   * Does not discriminate based on element type, so allows you to set the active
    +   * Does not discriminate based on element type, so allows you to set the is-active
    

    Both of these are 80 char breakers, should be moved to new line.

  2. +++ b/core/modules/editor/js/editor.admin.js
    @@ -387,7 +387,7 @@
    -        if (!filterStatus.active) {
    +        if (!filterStatus.is-active) {
    

    This looks like it would break JS, and is likely an unrelated change.

  3. +++ b/core/modules/editor/js/editor.admin.js
    @@ -578,7 +578,7 @@
    -    this.active = false;
    +    this.is-active = false;
    

    This would also break JS, and shouldn't be changed.

  4. +++ b/core/modules/editor/js/editor.admin.js
    @@ -695,7 +695,7 @@
    -          Drupal.filterConfiguration.statuses[filterID].active = $('[name="filters[' + filterID + '][status]"]').is(':checked');
    +          Drupal.filterConfiguration.statuses[filterID].is-active = $('[name="filters[' + filterID + '][status]"]').is(':checked');
    

    Same, this is a breaky change.

  5. +++ b/core/modules/language/src/Tests/LanguageSwitchingTest.php
    @@ -140,14 +140,14 @@ protected function doTestLanguageBlockAnonymous($block_label) {
           $anchor_classes = explode(" ", (string) $link->a['class']);
    -      if (in_array('active', $anchor_classes)) {
    +      if (in_array('is-active', $anchor_classes)) {
    

    This is hilarious, it's converting an array to a string to then explode it back into an array. Could this be fixed here or should we open up a folloup for that too?

RavindraSingh’s picture

@joelpittet, agreed on your 1st point, but I don't think about the other stuff are breaky. as they all are defined and just a replacement of .active class.

RavindraSingh’s picture

Assigned: Unassigned » RavindraSingh
Status: Needs work » Needs review

I am retesting it.

Status: Needs review » Needs work

The last submitted patch, 135: 2031641-is-active-class.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 135: 2031641-is-active-class.patch, failed testing.

idebr’s picture

Issue tags: +Needs reroll
error: patch failed: core/modules/toolbar/css/toolbar.icons.css:84
error: core/modules/toolbar/css/toolbar.icons.css: patch does not apply
joelpittet’s picture

@RavindraSingh Read them carefully. '-' character in javascript is a minus sign;-) That's why it's breaky

vineeth@nair’s picture

vineeth@nair’s picture

Issue tags: +#DCM2015
star-szr’s picture

Assigned: vineeth@nair » Unassigned

#135 looks like it introduces a bunch of unrelated changes (forgot to rebase?), so I'd suggest a reroll should be started from #132 instead.

@vineeth@nair I'm unassigning since it's been a while, if you are in fact working on this still please comment back and feel free to reassign :)

Some of the unneeded changes:

+++ b/core/core.services.yml
@@ -197,7 +197,7 @@ services:
-    arguments: ['@request_stack', '@url_generator']
+    arguments: ['@request_stack', '@url_generator', '@current_route_match']

+++ b/core/lib/Drupal/Core/Form/FormSubmitter.php
@@ -12,6 +12,7 @@
+use Drupal\Core\Routing\RouteMatchInterface;

@@ -33,15 +34,25 @@ class FormSubmitter implements FormSubmitterInterface {
+   * The current route match.
+   *
+   * @var \Drupal\Core\Routing\RouteMatchInterface
+   */
+  protected $currentRouteMatch;
+
+  /**
    * Constructs a new FormValidator.
    *
    * @param \Symfony\Component\HttpFoundation\RequestStack $request_stack
    *   The request stack.
    * @param \Drupal\Core\Routing\UrlGeneratorInterface $url_generator
+   * @param \Drupal\Core\Routing\RouteMatchInterface $current_route_match
    */
-  public function __construct(RequestStack $request_stack, UrlGeneratorInterface $url_generator) {
+  public function __construct(RequestStack $request_stack, UrlGeneratorInterface
+      $url_generator, RouteMatchInterface $current_route_match) {
     $this->requestStack = $request_stack;
     $this->urlGenerator = $url_generator;
+    $this->currentRouteMatch = $current_route_match;
akalata’s picture

Status: Needs work » Needs review
FileSize
43.71 KB
2.74 KB

Updating #132 with the feedback in #134.

Status: Needs review » Needs work

The last submitted patch, 145: 2031641-change-active-to-is-active-145.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 147: 2031641-147.patch, failed testing.

nlisgo’s picture

Status: Needs work » Needs review
FileSize
6.42 KB
51.23 KB
nlisgo’s picture

LewisNyman’s picture

Status: Needs review » Needs work

Looks like the last patch included Bartiks media.css file by mistake, which is now removed in HEAD.

nlisgo’s picture

Status: Needs work » Needs review
FileSize
47.31 KB

Good spot @LewisNyman. Not sure how that found it's way in there.

joelpittet’s picture

Status: Needs review » Needs work

Nice work everybody!

Here is a few things as I went line by line through the patch:

  1. +++ b/core/includes/menu.inc
    @@ -35,7 +35,7 @@ function template_preprocess_menu_local_task(&$variables) {
    -    $variables['attributes']['class'] = array('active');
    +    $variables['attributes']['class'] = array('is-active');
    

    Not something to fix in this issue, but that looks like a clobber of other classes on that variable. Needs a follow-up issue to fix that. Should be $variables['attributes']['class'][] = 'is-active'; but we need to check how that effects things in that preprocess.

  2. +++ b/core/modules/system/templates/table.html.twig
    @@ -62,7 +62,12 @@
    -          <{{ cell.tag }}{{ cell.attributes }}>
    +          {%
    +            set cell_classes = [
    +              cell.active_table_sort ? 'is-active',
    +            ]
    +          %}
    +          <{{ cell.tag }}{{ cell.attributes.addClass(cell_classes) }}>
    

    This looks like a bug fix. Can we move this to another issue or remove it if it's not needed anymore?

  3. +++ b/core/modules/toolbar/js/toolbar.menu.js
    @@ -148,7 +148,7 @@
    -        var $activeItem = $menu.find('a[href="' + activeItem + '"]').addClass('menu-item--active');
    +        var $activeItem = $menu.find('a[href="' + activeItem + '"]').addClass('menu-item--is-active');
             var $activeTrail = $activeItem.parentsUntil('.root', 'li').addClass('menu-item--active-trail');
    

    This may be scope creep, but I'm ok with it, though we may want to punt it to another issue to keep this from growing, because we may may want also menu-item--active-trail be menu-item--is-active-trail, for consistency.

  4. +++ b/core/themes/bartik/css/colors.css
    @@ -6,14 +6,14 @@ body {
    -.region-primary-menu .menu-item a.active,
    -.region-primary-menu .menu-item .menu-item--active-trail a {
    +.region-primary-menu .menu-item a.is-active,
    +.region-primary-menu .menu-item .menu-item--is-active-trail a {
    

    which was actually done here, lol. Well if there aren't many maybe we can add it to this issue... you be the judge, just keep in mind the bigger the patch, the harder to get reviews, so keep it from creeping too much...

nlisgo’s picture

Status: Needs work » Needs review
FileSize
1.66 KB
46.57 KB

Addressing feedback in #153.

Code in point #2 was removed here #2407745: Remove classes from system templates t*.html.twig, so removing from this patch as requested.

Regarding points #3 and #4, it is clear that at some stage in this issue someone introduced is-active-trail but it was cleared up later on and the css identified in #4 was the only remnant of this work. So, I'm removing and recommend also that this be addressed in a followup issue.

I also removed a blank link which had been introduced to core/themes/bartik/css/colors.css

Status: Needs review » Needs work

The last submitted patch, 154: change_active_class_to-2031641-154.patch, failed testing.

joelpittet’s picture

@nlisgo the interdiff and rational looks great, the patch has some extra stuff in it, maybe needs a rebase to catch up to head on that branch you made the diff from? Just a guess.

Thank you for addressing my review points!

nlisgo’s picture

Status: Needs work » Needs review
FileSize
615.7 KB
46.57 KB

Rebase done. :)

joelpittet’s picture

Status: Needs review » Needs work

Thanks again, trust me I've done that quite a few times too;)

+++ b/core/modules/toolbar/js/toolbar.menu.js
@@ -148,7 +148,7 @@
-        var $activeItem = $menu.find('a[href="' + activeItem + '"]').addClass('menu-item--active');
+        var $activeItem = $menu.find('a[href="' + activeItem + '"]').addClass('menu-item--is-active');

I think this one got missed from being un-done and moved to follow-up?

nlisgo’s picture

Status: Needs work » Needs review
FileSize
672 bytes
45.92 KB
jp.stacey’s picture

Reviewed as follows:

* The patch applies cleanly to 300f14e (D8 head as of this morning).
* Testing with a couple of hierarchical links set up in a menu, by clicking between them, .is-active is present on the relevant link, and the link picks up styles accordingly.

LewisNyman’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Thanks, let's get this in before it needs a reroll. I've added the beta evaluation

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

We still have things using the active class.

  1. .toolbar-tray-vertical.active
    

    in core/modules/toolbar/css/toolbar.menu.css

  2.   <a href="{{ row.url }}"{{ row.attributes.addClass(row.active ? 'active') }}>{{ row.link }}</a>
    

    in core/modules/views/templates/views-view-summary-unformatted.html.twig

  3.     <li><a href="{{ row.url }}"{{ row.attributes.addClass(row.active ? 'active') }}>{{ row.link }}</a>
    

    in core/modules/views/templates/views-view-summary.html.twig

  4.   .featured .region-primary-menu .menu-item a.active {
    

    in core/themes/bartik/css/components/primary-menu.css

  5. .site-footer .content a.active {
    

    in core/themes/bartik/css/components/site-footer.css

  6.               cell.active_table_sort ? 'active',
    

    in core/themes/classy/templates/dataset/table.html.twig

  7.  *   jQuery('li:has("a.active")')
    

    in core/includes/theme.inc

LewisNyman’s picture

Issue tags: +Novice

Tagging with novice to make these changes

_nolocation’s picture

Status: Needs work » Needs review
FileSize
49.32 KB
4.05 KB

I made the changes from comment #162

rteijeiro’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC!

akalata’s picture

Assigned: Unassigned » akalata
Status: Reviewed & tested by the community » Needs work

Needs manual testing and screenshots per alexpott in IRC. Working on those now.

akalata’s picture

Verifying that #164 addresses #162:

  1. Toolbar resizes appropriately at less than 320px. Screenshots attached.
  2. Confirmed class changes and visual styling does not change (Bartik). Screenshot attached.
  3. Confirmed class changes and visual styling does not change (Bartik). Screenshot attached.
  4. Can't find this to test. The 'featured' class is only in Bartik's CSS in that one declaration, and is likely left over from [#2393897].
  5. Also not testable. Menu blocks do not have a 'content' wrapper, so the CSS is not applying in either case. This should be addressed in its own Bartik issue.
  6. I can't figure out how to test this one. I've been able to find where the template is being used (set Classy to be the admin theme and visit admin/reports/access-denied), but event before the patch in #162, the active th has 'is-active'. My guess is that this template includes the addition of the class as a last-ditch fallback, but I don't know how to prevent the is-active from being added earlier.
  7. The [code]jQuery('li:has("a.is-active")')[/code] has been updated, but there are a few instances of "active" in the comment block before it that should probably be updated for consistency.

New item found:
8. Default template_preprocess_views_view_table(), class depicting active table sort: core/modules/views/views.theme.inc

alexpott’s picture

akalata’s picture

nod_’s picture

Issue tags: +JavaScript
ti2m’s picture

FileSize
69.97 KB

I ran the patch through siteeffect and the only thing I found is an issue with table sorting in views on e.g. /admin/content (see attached gif) or /admin/people. This is probably covered through point 8 in #167.

nlisgo’s picture

Assigned: Unassigned » nlisgo

I'm going to take this issue on. First action is to address point 8 in #167 for which a screenshot is provided in #171.

nlisgo’s picture

Assigned: nlisgo » Unassigned
Status: Needs work » Needs review
FileSize
564 bytes
49.87 KB
LewisNyman’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice, -Needs visual regression testing +drupaldevdays
FileSize
654.78 KB

Thanks for providing the interdiff. I manually tested the patch and I can confirm that it fixes the problem reported by the visual regression testing.

It would be amazing to get this patch committed at DevDays, because it's been open since DevDays in Ireland :D

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Let's do this! Committed b676df9 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed b676df9 on 8.0.x
    Issue #2031641 by aburrows, lauriii, nlisgo, Pol, redsquid, tuutti,...
aburrows’s picture

Great work team!!!!!!!!!!!!

joelpittet’s picture

<3 state classes!

Status: Fixed » Closed (fixed)

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

brahmjeet789’s picture

LewisNyman’s picture