Untangle gin.css

I'll be honest 92 files +8571 −5663 is a massive MR to review.

As someone who has created massive MRs previously I've always provided steps to reproduce which are not in the issue summary.

I see mentions of "With Claude Code's help," but no real description of which part it did vs you, how would I go about verifying and reproducing your output? I've seen people post the chat log in other issues but I'm not sure if that helps if I'm being honest.

It's kind of hard to see which code is just reorganized, which is modified and what is changed.

What is the visual diff tool and how do I run it?

Why are all of the files called gin, I thought it was supposed to be default admin?

If I run the instructions here https://www.drupal.org/node/3084859 is it expected I would have no diff?

The visual diff too is mentioned in the parent, comment #5. It is a DDEV add-on https://github.com/mherchel/ddev-drupal-admin-vrt

Ok, thank you, let me start with running the visual diff then I can look at the plans copied in.

Having the plans in each commit is nice for reviewing the actual commit, but is pretty painful for replicating since I have to copy the plan from each step.

The visual regression test does not seem to be working.

I followed these steps:

• Clone drupal fresh
• Checkout this issue fork
• Switch back to main
• ddev composer require drush/drush
• ddev drush si
• ddev drush theme:install default_admin -y
• ddev drush config:set system.theme admin default_admin -y
• ddev add-on install https://github.com/mherchel/ddev-drupal-admin-vrt/tarball/main
• ddev restart
• ddev exec -d /var/www/html/.ddev/drupal-admin-vrt npm install
• ddev exec -d /var/www/html/.ddev/drupal-admin-vrt npx playwright install --with-deps chromium
• ddev vrt-update
• git stash // There are conflicts with composer
• Switch to this issue fork
• ddev composer require drush/drush
• ddev vrt

I received this error:

✘    1 [auth-setup] › fixtures/auth.setup.ts:7:6 › authenticate as admin (10.6s)


  1) [auth-setup] › fixtures/auth.setup.ts:7:6 › authenticate as admin ─────────────────────────────

    Error: expect(locator).toBeVisible() failed

    Locator: locator('body.user-logged-in')
    Expected: visible
    Timeout: 10000ms
    Error: element(s) not found

    Call log:
      - Expect "toBeVisible" with timeout 10000ms
      - waiting for locator('body.user-logged-in')


      30 |   }
      31 |
    > 32 |   await expect(page.locator('body.user-logged-in')).toBeVisible({ timeout: 10000 });
         |                                                     ^
      33 |   await page.context().storageState({ path: authFile });
      34 | });
      35 |
        at /var/www/html/.ddev/drupal-admin-vrt/fixtures/auth.setup.ts:32:53

    Error Context: ../../test-results/auth.setup.ts-authenticate-as-admin-auth-setup/error-context.md

    attachment #2: trace (application/zip) ─────────────────────────────────────────────────────────
    ../../test-results/auth.setup.ts-authenticate-as-admin-auth-setup/trace.zip
    Usage:

        npx playwright show-trace ../../test-results/auth.setup.ts-authenticate-as-admin-auth-setup/trace.zip

    ────────────────────────────────────────────────────────────────────────────────────────────────

  1 failed
    [auth-setup] › fixtures/auth.setup.ts:7:6 › authenticate as admin ──────────────────────────────
  126 did not run

To open last HTML report run:

Here are all of the plans:

I got the VRT command working. I had missed clearing cache after switching branches.

After clearing cache on this branch I ran:

• ddev vrt
• ddev vrt-report

I confirmed the regressions in #3582826-16: Untangle gin.css
I then got yarn installed and updated and removed the display: flex; in core/themes/default_admin/migration/css/base/gin-regions.css since that was identified as the cause.

I then used watch:css and confirmed display: flex; no longer was in the compiled css for .block-page-title-block
I cleared cache and ran the following commands again:

• ddev vrt
• ddev vrt-report

I then reviewed the report and the regression is still there. Attached a screenshot.

It's been a long time since I compiled pcss and I've never used the vrt tool before so I may be missing something.

I just had a long conversation with @mherchel on zoom.

There are two outstanding code related issues in the MR:
&.--is-processed thead seems to be invalid css
There is a regression with radio button toggles (see the attached screenshot)

We figured out how to run the VRT in a way that produced more sane results.

We checked out main and added the missing display flex, generated the baseline images, then checked out this branch and ran the report.

That resulted in 37 failures we could manually review.
The failures fell into a few categories.
1. The aforementioned toggle button issue
2. A specificity issue where spacing from help text p tags disappears
3. Something with table spacing causing different wrapping
4. Drop button widths
5. Width of autocomplete boxes
6. Spacing around select boxes

2-6 do not seem like they need to be fixed here, but the toggle buttons need to be fixed as well as the invalid CSS.
We do probably need follow ups for it.
We also need follow ups to review some items in Gin that were not introduced here:
E.g. the use of !important in many places is probably no longer necessary
There are some sections of the copied claro code that cause empty rulesets:
E.g. core/themes/default_admin/css/components/tabs.css

& .tabs--primary {
      & .tabs__link {
        &:focus {
          box-shadow:
            0 0 0 2px var(--color-gray-050),
            0 0 0 calc(var(--tabs--focus-height) + 2px) var(--color-focus);
        }
      }
    }

We also noticed that in general buttons are broken, but they are broken on HEAD as well

In general I think some of this should have been resolved before it was merged into core, but it is experimental so larger changes are probably acceptable, @mherchel pointed out that there needs to be at least two more large MRsbeyond this MR to get it up to speed.

I think the above solves my technical concerns.

I do have a couple of other reservations which I am not sure are blocking, but wanted to voice them here.
1. It was fairly involved to find and sift through real regressions, noise and minor regressions that can be follow ups. The initial review had over 100 regressions and it was almost by accident I found the broken toggle buttons. Thankfully we realized if we backported one fix it would solve most of the noise.
2. Even though we have the plans written and the commit history, the output is not reproducible in the same way which means if we uncover something that requires regenerating this it forces retesting the full output and VRT again, we can't just verify the individual piece that needed updating.
3. How does this work with licensing, I'm not sure I've seen a discussion around that.

1 is mitigated by how we ended up testing
2 is mitigated by reviewing the commit history, most of them are fairly small, I reviewed over half, but did not review every commit.
3. I'm not sure this is the issue to answer that, but if someone has a link to a policy on it I'd appreciate it.

I won't be able to test that fix tonight, but I wanted to comment that you are right the .- is valid https://www.w3.org/TR/CSS2/grammar.html

I think commenting out those changes to get a closer diff makes sense too.

Hi, I followed the test instructions and can report that:

1. I checked the original SCSS files from the base directory and confirmed a new CSS file exists in the new MR
2. I can confirm the gin.css file was deleted in this MR
3. I can confirm all the files added to the libraries.yml exist in the migration/css/base folder, and that all files added to migration/css/base folder are added to the libraries.yml too
4. I installed the latest Drupal version and enabled the default_admin theme, made it the default admin theme and after some time checking the site, things look pretty much in order, haven't found any obvious regression so far

RTBC+1 from me on this. I used some AI tooling for consistency checks and verified that we haven't lost anything. LGTM.

So after getting both set up locally and using the DDEV addon.

• I poked at most pages on the core navigation side panel on a desktop breakpoint and found no glaring issues (outside of those It already had on the original gin one).
• The ddev plugin helped with confirming that, though I got a lot of timeouts on mobile breakpoints.
• ddev vrt-report had a few false positives, a few pages such as /admin/structure/menu, but that was likely due to the snapshot happening too fast. Since manually i can see them working well.
• I looked over the Claude MD files, and they create a clear outline of what the transition was for some of the SCSS plugins and syntax. Especially those surrounding icons and media queries.
• I switched the admin theme to dark mode to check if something would become invisible, but so far it looks like everything is there.

There is one actual regression here compared with main, the hover on secondary buttons is broken:

I have a fix but not sure if I should bump this back now that it is RTBC :) @mherchel how would you prefer to handle this?

That should be fixed I think.

It would be good to see if the vrt can handle hover states eventually.

Pushed my changes. The change to the height fixes an existing bug, but seemed like it was worth improving here.

Before:

After (default):

After (hover):

I think the button height was explicitly kept since it was not a regression and this issue is already massive so we could keep the vrt closer and fix the buttons in a follow up.

I just tested this locally and found the issue of the disappearing text in other pages, after applying the latest changes I can see it back to normal. I think this is working fine, the buttons are all showing up again. Thanks @pameeela! :)

No problem @nicxvan, removed the height fix. Thanks @dharizza!

Actually, I just noticed I tested right before the removal of the height fix, when I test with the last commit the hover state is wrong (but it is not wrong in the Block Layout page). Should that be fixed as part of this one? It can be seen in the View and Form mode pages.

It is indeed a regression as I'm not able to reproduce it in the main branch.

That's weird, it's working for me on those pages?

It is very strange, when I clear the cache I see it working correctly but if I reload then it looks as in the screenshot I sent.

That is so strange, I just tested in a few other browsers (using Chrome, but checked FF and Safari which I never use) and it's looking OK in all of them.

That makes sense, yes, after doing that it works like a charm! Thank you!

Thanks for all the manual testing!

Did quick sanity check over the files, ran a check with LLM for consistency, and did some manual testing to check high risk areas in the UI for regressions. I did find plenty of small bugs but none of them were caused by this change ✨