Update normalize.css to the most recent version

Drupal still support IEs for awhile.

I think it needs a better comparison to see what style actual changed.

Just list the actual changes (what's added & removed between v3 ~ v5)?

+++ b/core/assets/vendor/normalize-css/normalize.css
@@ -1,154 +1,200 @@
-  font-family: sans-serif; /* 1 */
-  -ms-text-size-adjust: 100%; /* 2 */
-  -webkit-text-size-adjust: 100%; /* 2 */
+  font-family: sans-serif;
...
+  -ms-text-size-adjust: 100%;
...
+  -webkit-text-size-adjust: 100%;

For example these few lines, there're no real changes. And many styles just move up and down.

@chrisrockwell,

It doesn't point out every change, all reviewers still have to scan full patch once.

I noticed one thing, do you modify the code style?
https://github.com/necolas/normalize.css/blob/5.0.0/normalize.css

Here's a better comparison. Manual aligned. Never want to do it again.

It seems not so possible to add to CORE though. Apparently, some changes will break the designs.

Thanks @droplet. There is a declaration missing: [hidden] { display: none } but I didn't review it all.

this rule is merged, and no changes between v3 & 5.

(** this is not a final patch but for reviewing only)

On other points, I understand your concerns :)

1. @@ -273,34 +286,41 @@
   -table {
   -  border-collapse: collapse;
   -  border-spacing: 0;
   -}
   ...
   -td,
   -th {
   -  padding: 0;
   -}
   

   Based on my personal experience,
   I'd say take the newer version. and then add these back to CORE

2. @@ -1,11 +1,9 @@
   +  line-height: 1.15; ¶
   
   @@ -184,20 +193,22 @@
   +  font-family: sans-serif; ¶
   ...
   +  line-height: 1.15; ¶
   

line-height changes needed discussion

and all others change I think that's acceptable and will not make a big visual break

I re-created the patch locally by just getting https://raw.githubusercontent.com/necolas/normalize.css/5.0.0/normalize.css...

I looks like the patch in #12 adds a new line, incorrectly. Patch fixes that.

I think it is important we keep this up-to-date.

Thanks everyone. Extra shout out to @droplet for the work on creating that diff, very helpful!

Can we please get a follow-up issue filed (and related to this issue) for adding the border-collapse and any other needed CSS to Stable and/or Classy? I think Stable would make sense in this case. I want to make sure that doesn't slip between the cracks.

Follow-up created, hopefully got the intention of it correct in the issue summary.

#2845106: Add needed diff between normalize.css 3.0.3 and 5.0.0

Back to RTBC

Hmmm... surely the new CSS to ensure things don't change has to be added in this issue?

I'm ok with that being added in this issue too and was thinking that would be the case. Thought @Cottser had a commit plan in mind, so went with it... like improv;)

Yeah, sorry, what @alexpott said in #25 does make more sense because then we don't get the regression at all.

Thanks @Cottser, move the IS changes into this issue, let's do it here.

Looks like the test failure was the known intermittent failure of OutsideInBlockFormTest ; the patch is passing again now. Setting back to NR.

Not in 8.3.x yet, although I think we could still consider backporting this if it does land, especially before RC.

For now. We debated whether this could go in during RC/patch release, but I am leaning away from that since it's technically a major version change and includes frontend API changes. So by marking it with the deadline now, we can be sure to at least revisit it.

Forgot to adjust branch. If we don't get this in for RC it can be moved back to 8.4.x.

+++ b/core/assets/vendor/normalize-css/normalize.css
--- a/core/themes/stable/css/system/system.admin.css
+++ b/core/themes/stable/css/system/system.admin.css

I'm not sure that this is the right place to load add this css. Normalise is loaded on every page - this is loaded with the admin library. Ideally this would go in a component that is part of base library defined in system.libraries.yml

Re #31 I think you are correct - we should add this in as part of the system's component css. Maybe stable should implement a hook_library_info_alter() to do this.

Since 8.3.x is now in commit freeze for its release candidate phase, this major version dependency update should now be targeted for 8.4.x. Thanks!

Still possible get into 8.4?

Usability is preferred over UX, D7UX, etc. See https://www.drupal.org/issue-tags/topic

Everything is over-engineering in Drupal. Seriously, it took 11 months for a simple task. (I do not blame any contributors in this issues thread but we have a real contribution workflow problem.) normalize.css has been released 2 more major version.

7.0.0 is released https://github.com/necolas/normalize.css/releases/tag/7.0.0

One thing to consider is that normalize has taken a large departure from it's previous versions... bootstrap forked the project because of this change.
https://blog.getbootstrap.com/2017/08/10/bootstrap-4-beta/

@joelpittet BS4 merged normalize with their standard style. It might not possible to do in Drupal CORE. (Not sure, But a huge workload to maintain the updates)

I haven't been following this issue and haven't read the 50 comments here... but I'd like to point out that on #2886904-29: display: block for details/summary hides drop arrows in Firefox (normalize.css update), the patch there from #6 (which updated normalize.css to version 7) was tested on IE and Edge, and it was broken as far as the drop-down arrows that are referenced on that issue.

Since the change for the drop-downs was added to Normalize before 5.0, I think it would be a problem there. I filed an issue in the Normalize project:
https://github.com/necolas/normalize.css/issues/740

Couldn't we make use of libraries API declaration at the theme level? Say, copy normalize-v3 to the Stable theme, and use libraries-override to establish it there. Then it could be updated more frequently in core/assets, and other themes (core, contrib, or custom) could choose to opt-in to the lastest version in core/assets, keep going with v3 from Stable, or bundle their own copy of normalize?

Would issues like #2886904: display: block for details/summary hides drop arrows in Firefox (normalize.css update) would be easier to address this way?

Probably we could, but what I was trying to say in #52 is that newer versions of normalize fix Firefox while breaking IE and Edge for the problem we're trying to solve in #2886904: display: block for details/summary hides drop arrows in Firefox (normalize.css update)... If we were to adopt a newer version of Normalize, we'd definitely need some extensive manual testing to see what else might have been changed/broken in various browsers, and then we'd need to introduce fixes so we don't break the core Seven and Bartik themes.

I don't think we can safely update normalize.css without providing BC layer in stable. This does force us to keep two versions of the library at once. Luckily normalize.css only consist of CSS, which means it will result in only a little bit of overhead. We should try to update this before Drupal 9 so that this gets shipped as part of Drupal 9 stable.

We're working on a new browser support policy in #2390621: [policy, no patch] Update Drupal's browser support policy. As part of the issue, we are dropping support to lots of old browser versions. This could allow us to update to the most recent version of normalize.css.

Switching this to a 9.0.x patch.
I did some issue summary updating, but also adding the "Needs issue summary update" tag as it will require more before this is ready for review.

No interdiff since this is applying to 9.0.x and with a much newer version of normalize.css

Not sure if there's a good way to manually test since stable is still using the prior version of normalize.css. If there are issues they will surface as themes decouple from Classy and Stable, but if there are particularly at-risk areas they could be tested with Stark.

I didn't know who to test this so I did a small experiment where I ran a pre-existing test and output screenshots

1. I used \Drupal\Tests\layout_builder\FunctionalJavascript\InlineBlockTest::testInlineBlocks() as test case
2. I made some changes so that with every drupalGet() it output a screenshot
3. I am some other changes to not output dates and times since this would change
4. I ran the test multiple 2x with the patch from #61 outputting the screenshots to a different folder each time.
5. I then diffed the folders via diff -rq folder1 folder2 to see if the exact same screen shots were produced each time.
6. There screenshot were exactly the same except for 1 which random text in a body field
7. This proves that above hack can produce the exact same screenshots, there were 20(19 exact matches)
8. I think switched to 9.0.x and di the same test. Outputting 2 sets of screenshots. The 2 9.0.x folders were the same exact for the 1 with random text
9. I then compared screenshots produced with patch with the screenshot produced with 9.0.x
10. There was 1 screen that was different between 9.0.x and the patch version. It was when the user was logged out and there was extra whitespace around the page in 9.0.x. Personally the patched version looks more "correct" to me but of course this is change

I will attach the non matching screenshots and the patch I used to make the screen shots.

I am not sure if such a small change is important. Maybe we should run the same process on some other tests?

This will require some browser-specific testing of elements that are impacted by normalize.css, specifically the rules that actually change something from the browser default stylesheet. I'll get some info and screenshots on that so it's not a beast to review.

Went through every change between 3.0.3 and 8.0.1 and narrowed it down to ones that could potentially impact Drupal

1. Table/table cell resets removed - deemed opinionated.
   https://github.com/necolas/normalize.css/issues/506
   These rule is no longer present.

   table {
       border-collapse: collapse;
       border-spacing: 0;
   }
   td,
   th {
     padding: 0;
   }

2. An SVG reset was removed that is still applicable to IE11 (its default is overflow: visible;)
   The commit message states that it's part of removing no longer supporter browers (incl IE9 and below)
   Unsure why this was removed as it impacts 10/11 still.
   https://github.com/necolas/normalize.css/commit/004d58b2f2e0ac3d03d075f8...
   NO LONGER PRESENT

   svg:not(:root) {
     overflow: hidden;
   }

   Note that version 4.4.1, this rule was changed for less specificity
   https://github.com/twbs/bootstrap/commit/900775483ff0d5aa79e497fbfee8985...
   to Just

   svg {
     overflow: hidden;
   }

3. A rule removing hover and active outlines is no longer present, it was deemed opinionated.
   https://github.com/necolas/normalize.css/commit/b5f0e9d79a997952f0d68f55...
   NO LONGER PRESENT

   a:active,
   a:hover {
     outline: 0;
   }

4. The default input[type="search"] looks slightly different in Safari
   Before

   input[type="search"] {
     -webkit-appearance: textfield; /* 1 */
     box-sizing: content-box; /* 2 */
   }

   After

   [type="search"] {
     -webkit-appearance: textfield; /* 1 */
     outline-offset: -2px; /* 2 */
   }

5. Some fieldset styling was removed as it was deemed opinionated
   https://github.com/necolas/normalize.css/commit/b5f0e9d79a997952f0d68f55...
   NO LONGER PRESENT
   fieldset {
   border: 1px solid #c0c0c0;
   margin: 0 2px;
   }
   fieldset padding also changed from
   padding: 0.35em 0.625em 0.75em; to padding: 0.35em 0.75em 0.625em;
6. Rules regarding cursor styles for some inputs were removed after they were deemed opinionated
   NO LONGER PRESENT

   [type="button"],
   [type="reset"],
   [type="submit"] {
   removed cursor: pointer;
   }

7. BEFORE

   abbr[title] {
     border-bottom: 1px dotted;
   }
   

   AFTER

   abbr[title] {
     border-bottom: none; /* 1 */
     text-decoration: underline; /* 2 */
     text-decoration: underline dotted; /* 2 */
   }
   

8. Removed - considered opinionated
   NO LONGER PRESENT

   pre {
     overflow: auto;
   }

9. line-height: 1.15; added to <html> This was to add consistency to most browser defaults being line-height:normal. There are several conversations regarding this change, this one seems to have the most info + links to others. https://github.com/necolas/normalize.css/issues/612

I went through each potentially-disruptive change in #64 to see if they were in fact disruptive. Added core examples when I could find them (refer to that comment for details about the rules).

1. Disruptive
   <table>,<td>,<th>
   Modernizr 3.0.3
   
   Modernizr 8.0.1
   
2. Disruptive
   This is still an issue in IE11, should not have been removed as a rule outside of their supported browsers.
   This should be a 1/4 circle, but is not with Modernizr 8.0.1
   Modernizr 3.0.3
   
   Modernizr 8.0.1
   
3. Probably fine
   reviewed every desktop browser's default stylesheet and none provide outlines on a:active,a:hover
4. Disruptive
   Search input styling changes on Safari and Chrome
   Safari
   
   Chrome
   
5. Disruptive
   This is overridden in every core and popular-contrib instance I could find, but it still changes things from the default, and contrib or custom could be expecting this styling.
   Screenshot from Chrome with only normalize.css
   
6. Probably fine
   Tried this on every supported desktop browser - mobile isn't needed since it's cursor styling (i think...)
   The default stylesheet for these has the same rule.
7. Different, but maybe fine
   It's an opinion, granted, but the <abbr> change may be welcomed?
   Safari
   
   Chrome/Firefox
   
8. Disruptive
   This could result in <pre> elements that had scrolling to instead flow outside of their container.
9. Probably!
   I couldn't find a scenario with core themes where this would have an impact, but a line-height declaration on <html> is quite capable of altering the appearance of something contrib/custom. This is a good rule to have, but may not be ideal to introduce to an existing site, particularly since it's not something that can provide deprecation warnings to maintainers. This change has received some complaints https://github.com/necolas/normalize.css/issues/612

Some of these changes may be acceptable since Stable (8) will still use normalize 3.0.3, and perhaps some changes in 9 can be accepted even if they can't receive deprecation coverage. Some, like table and search, don't seem acceptable and a custom solution may be necessary.

Thank you for the thorough research @bnjmnm! 👏I think it's fine for us to proceed with updating the core version of normalize.css. I checked whether this affected the Bootstrap theme. It seems like they ship with their own version of normalize.css, meaning that this change won't affect them. I'm wondering if we should ship with normalize.css as part of core (rather than Classy) at all since it's only being loaded there. Maybe we could open a follow-up to determine that?

I think we should open upstream issue for #64.2. We should also consider fixing this in core while we wait for this to be fixed in the upstream package.

#64.1 and #64.4 we should solve this in the affected themes. This becomes relevant once we move forward with #3050389: [META] Remove dependency to Classy from core themes.

#64.2 Opened a PR for the SVG issue https://github.com/necolas/normalize.css/pull/806. The normalize.css repo has been inactive for ~14 months so I'm not sure if it will get any traction, but it's also possible that the current PRs/issues are things that are less straightforward.

Will provide a patch with fixes for items 1,2,4 in #64 next.

This provides additional CSS to address items 1,2 & 4 in #64 Discovered some of the issues were due to a removed line-height rule that normalize.css added for FF4, but is now baked into some assumptions in core.

Debated commenting all the new rules, but in most cases it seemed like it could cause more confusion than benefit as they're not particularly different from nearby uncommented rules. We'll let this get figured out via review.

Thank you for opening the upstream PR! 👏

What are the steps for testing the changes in #68?

+++ b/core/core.libraries.yml
@@ -580,7 +580,7 @@ modernizr:
-  version: "3.0.3"
+  version: "8.0.1"

+++ b/core/themes/stable/stable.info.yml
@@ -89,6 +89,13 @@ libraries-override:
+    # Load version 3.0.3 of normalize.css for backwards compatibility.

Should we allow overriding the library version and other values? If we override the library with a different version of the library, value of this property is not correct anymore.

Should we allow overriding the library version and other values? If we override the library with a different version of the library, value of this property is not correct anymore.

I got hung up on the override not allowing version: then realized I could just replace the whole library, so that's all set now.

The patch also changes Claro's pcss file instead of the css, because we all need that to happen to us at least once.

What are the steps for testing the changes in #68?

It's not a straightforward process because the patch includes stable using normalize.css 3.0.3. To make this testable I added a patch specifically for testing. This removes the override, so themes are using normalize.css 8.0.1.
The patch also includes a comment with every rule that was added to fix issues caused by the normalize.css update, you can find these comments by searching for the string TEST-2821525. To see the difference with/without the fix, follow the steps in each comment and comment the CSS rule to see the difference.

Here are before/after screenshots of each added rule.
Input line height - chrome - bartik - not fixed

Input line height - chrome - bartik - fixed

☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️

Input line height - chrome - seven - not fixed

Input line height - chrome - seven - fixed

☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️

Search input cancel button - safari - bartik - not fixed

Search input cancel button - safari - bartik - fixed

☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️

Search input cancel button - safari - seven - not fixed

Search input cancel button - safari - seven - fixed

☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️

Table styling - all browsers - claro - not fixed

Table styling - all browsers - claro - fixed

☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️ ☠️

Table styling - all browsers - seven - not fixed

Table styling - all browsers - seven - fixed

Tagging as a requirement for Drupal 9.0-beta1.

I've run visual regression testing on the patch using Wraith, and spidering through all of the paths in a Standard Drupal core install. This tested around 1500 paths at 5 different resolutions. The only differences that were found were due to content differences between the test sites (e.g. timestamps), so I think that this is ready to go from the standpoint of matching experiences and the CSS changes make sense to me from an overall architecture viewpoint.

@DyanneNova Thank you for doing manual testing on this! It would be useful to get a bit more information on what was being tested. Which patch did you use for testing? Also, which themes did your test cover?

According to @DyanneNova, the testing patch was used, and Seven and Bartik were tested. She's going to look into testing Claro and Umami (which will take a long time because of all the content paths it has).

Made the issue summary current.

I've tested the testing patch with Claro and Umami now and found no differences besides content differences, so I think that this is looking good to go.

I did a bit of manual testing and I found the following regressions that should be still addressed:

Theme / Component	Before	After
Bartik / Table
Seven / Select
Seven / Fieldset
Umami / Fieldset
Quickedit
Bartik / Fieldset

Very good catches in #78! The majority of these are issues that were already solved in some themes, but obviously not all of them. Eventually it became clear that the regressions were all best addressed within the library instead of theme-specific changes. Fewer overall changes and easier to document the ones that were added.

Will follow this up with manual review and screenshots.

I'm wondering if these are actual regressions or just changes of behavior? If we ship these as part of the normalize-fixes, it means that these will be shipped to all themes, not just core themes. Is that what we actually want?

Re #80

I'm wondering if these are actual regressions or just changes of behavior? If we ship these as part of the normalize-fixes, it means that these will be shipped to all themes, not just core themes. Is that what we actually want?

\

My earlier patch leaned more in the direction of theme-specific, but based on the feedback in #78, I believe this is what is want (with one possible exception).

I'll go through each of the items in normalize-fixes:

1. +++ b/core/misc/normalize-fixes.css
   @@ -1,13 +1,56 @@
   +}
   

   This is to address a bug in Normalize.css, which I've submitted a PR for.

2. +++ b/core/misc/normalize-fixes.css
   @@ -1,13 +1,56 @@
   +fieldset {
   +  border: 1px solid #c0c0c0;
   +  margin: 0 2px;
   +  padding: 0.35em 0.625em 0.75em;
   +}
   

   This was one I initially categorized as a change in behavior and did not provide any changes. Since it was pointed out in #78 as something that should be addressed if possible, it was added. It was placed in normalize-fixes as it was reported as occuring in Bartik + Umami, and Claro + Seven have no rules specifically targeting this element.

3. +++ b/core/misc/normalize-fixes.css
   @@ -1,13 +1,56 @@
   +button,
   +input,
   +optgroup,
   +select,
   +textarea {
   +  line-height: inherit;
   +  font-size: inherit;
   +}
   

   This was already added to Bartik and Seven for <input>. #78 revealed this is also occurring with select, and I found this to be the case with <select> and <input> with Umami as well. These examples led me to believe that there are likely more unwanted surprises to come that result from normalize.css 8.0.1 introducing font-size and line-height to these elements with this rule:

   button,
   input,
   optgroup,
   select,
   textarea {
     font-family: inherit; /* 1 */
     font-size: 100%; /* 1 */
     line-height: 1.15; /* 1 */
     margin: 0; /* 2 */
   }

4. +++ b/core/misc/normalize-fixes.css
   @@ -1,13 +1,56 @@
   +table {
   +  border-collapse: collapse;
   +  border-spacing: 0;
   +}
   +td,
   +th {
   +  padding: 0;
   +}
   

   These were already necessary additions to Claro and Seven. I moved it to normalize-fixes after #78 pointed out the table difference in Bartik. I suspect this got past manual testing as tables on the FE theme are not as common, and should have explicitly tested for this. This will likely impact Umami in the same way as it has no styles specific to table/tr/td elements.

5. +++ b/core/misc/normalize-fixes.css
   @@ -1,13 +1,56 @@
   +/**
   + * Prevent regression due to normalize.css no longer hiding the cancel search
   + * button in 8.0.0. The cancel search button appears in Safari.
   + */
   +input[type="search"]::-webkit-search-cancel-button {
   +  -webkit-appearance: none;
    }
   

   This was initially added to just Bartik and Seven, and I opted to move it to normalize-fixes for consistency, but upon review I'm leaning towards moving this back to just Bartik and Seven and creating a followup to address the styling issues that result from the webkit search cancel button being visible, so this functionality can be present in all themes.

   Normalize 3.0.3 set this element to display:none. This rule was removed because it's effectively removing functionality inherent to input[type="search"], so it would be nice to fully remove it from core, too.

Discussing this with @lauriii exposed some core-developer myopia on my part as I'd forgotten about how this impacts custom themes. While most of the rules I added to normalize-fixes should be present in all core themes, it should not be added in a way that forces those rules on custom themes. This also emphasizes the need for a detailed change record. While these rules should not be forced upon themes, it may result in some changes to existing ones and it would be good to detail those.

As part of moving the rules to individual themes, I went through each selector and rule to confirm it was needed and found that many were not. Previously, I'd been copying the entire block of CSS from normalize.css, but quite often only a portion of it was necessary.

I'll go through change-by-change again.

1. /**
    * Add SVG styling for IE that was mistakenly removed from normalize.css in
    * 8.0.0.
    */
   svg:not(:root) {
     overflow: hidden;
   }

   This stays in normalize-fixes. It's fixing an actual bug.

2. +fieldset {
   +  border: 1px solid #c0c0c0;
   +  margin: 0 2px;
   +  padding: 0.35em 0.625em 0.75em;
   +}

   This was necessary in every core theme other than Claro, which always overrides these styles by adding a class in claro_preprocess_fieldset(), and I did not find instances of a hard coded fieldset (if they exist, then we should see if they are impacted by the regression)

3. +button,
   +input,
   +optgroup,
   +select,
   +textarea {
   +  line-height: inherit;
   +  font-size: inherit;
   +}

   This was the trickiest one as the way this was applied in normalize.css 3 was a bug - inherit should not be used. For each theme, I went through each selector to see if a change occurred these rules were disabled. If there was a change, I provided a hard coded line-height/font-size to match what resulted from using inherit. In some cases, particularly Claro, the normalize css rules were always overridden so those were skippable as well. Each theme only required a small subset of this rule.

4. +table {
   +  border-collapse: collapse;
   +  border-spacing: 0;
   +}
   +td,
   +th {
   +  padding: 0;
   +}

   Umami looks slightly better without these rules added, so I skipped them
   With the normalize.css 3.0.3 table CSS
   
   Without it
   
   For the remaining themes, only border-collapse: collapse; is necessary. The other table/td/tr rules were not added to those themes.

5. +/**
   + * Prevent regression due to normalize.css no longer hiding the cancel search
   + * button in 8.0.0. The cancel search button appears in Safari.
   + */
   +input[type="search"]::-webkit-search-cancel-button {
   +  -webkit-appearance: none;
    }

   Keeping this in normalizes-fixes, with plans to remove in #3114878: The webkit search cancel button should not be hidden . This rule hides search cancel buttons from appearing in the webkit browsers that support them, and this is how core currently functions. However, it is preferred to adopt the perspective of normalize.css and make this element visible, but if this rule is removed, it impacts the search input styling for all themes. In some cases inarguably breaks it. In the followup we can fix those issues in core themes and determine any risks to contrib/custom themes by making this element visible. In the scope of this issue, however, I'd be concerned about exposing an element that has otherwise been hidden for Drupal 8's lifespan.

Here are the patches I meant to include with #82
(and this one submitted without my consent so the patches aren't here either.... we will try again)

I did pretty extensive testing on the patch. I could only find this one regression which affects all themes.

Before:

After:

#86 is due to a rule introduced in normalize.css 4.1.0. Added regression prevention to each theme.

1. +++ b/core/core.libraries.yml
   --- a/core/lib/Drupal/Core/Render/ElementInfoManagerInterface.php
   +++ b/core/lib/Drupal/Core/Render/ElementInfoManagerInterface.php
   
   +++ b/core/lib/Drupal/Core/Render/ElementInfoManagerInterface.php
   @@ -57,7 +57,7 @@ public function getInfo($type);
   -   * @return mixed
   +   * @return string
   

   Unrelated change 🔬👁

2. 
   There's still a minor difference on this field. The field is slightly higher which makes the "No file chosen" text unaligned in comparison to the button. This seems to only happen in Seven.

The should-have-rerolled issue is addressed, but the interdiff doesn't include that.

I found minor differences with the file input on Umami and Bartik as well. I opted to address these as well since the solution is applying less styling overall. Note that In Seven there is a barely (if at all) perceptible difference in line-height by going with this less-styling approach, which I preferred to adding a rule specific to file inputs.

This addressed #88 👍

I found one more regression on IE 11; for some reason there's a bullet next to details elements. This happens in all themes except Claro.

Bumping to critical since this blocks #3094468: [plan] Update core JavaScript (and CSS) dependencies prior to 9.0.0-beta1 which in turn blocks the beta/release.

The issue in #90 is due to normalize.css setting all summary elements to display: list-item. I'd assumed was only a good thing as this was a problem in some Firefox versions. However, for browsers that don't support details/summary, this results in a list item bullet being attached to the element.

Note that for this fix several of my attempts to target Edge did not work, including ones that are in use on Claro. I tried:

_:-ms-lang(x), _:-webkit-full-screen, .selector {}
_:-ms-input-placeholder, :root .selector {}
_:-ms-fullscreen, :root .selector {}
_:-ms-input-placeholder, .selector {}
_:-ms-fullscreen, .selector {}

Until I was finally able to get it to work in my Virtualbox version of edge with
@supports (-ms-ime-align:auto){}

Also confirmed that tab navigation and focus still work properly in IE/Edge after this change.

Confirmed that this fixes #90 in both, IE Edge and IE 11.

I've now tested Seven, Bartik, Claro and Umami with IE 11, Edge, Firefox, Chrome (MacOS and Android) and Safari (MacOS and iOS) using the Style guide module which should cover most standard HTML element styles. I've also clicked through lots of features in the UI, including node edit form, Umami front-end pages, Bartik front-end pages, status report page, Views UI, layout builder, media library, Field UI and a couple of the configuration forms. I couldn't find any additional regressions.

We've also done some automated regressions testing (#77) which suggested that there aren't any big regressions.

I also confirmed that correct files are loaded from Stable using the non-testing patch.

I'm removing the needs subsystem maintainer review since I believe that it isn't necessary anymore at this point anymore.

I created a draft CR for this since I think it would be good to notify themes not extending Stable or Classy that this might cause regressions in their themes.

Good call on the CR. We should also actually add a release note summarizing the potential disruption. (We'd already be mentioning the dependency major version update anyway, so we can just add a sentence or two to that.)

Added a release note based on the CR.

I ran a quick wraith test on the manual testing patch in #92. I tested Claro and Umami, with a few sample content items at 360px, 768px, and 1280px widths. I've attached the spider yaml file with the tested urls.

The test found some minor changes on these urls:

/filter/tips
/contact/feedback
/contact
/user/login
/user
/user/password
/search
/search/node

I've uploaded the gallery as a zip file with the screenshots and diffs, plus the gallery website with the analysis.

It looks like the relevant rule causing the change for the forms is in the umami base.css file:

/**
* Prevent regression due to explicit line-height applied to these elements in
* normalize.css 7.0.0.
*/
button,
input:not([type="file"]),
textarea {
line-height: 1.5rem;
}

For filter/tips, the rules that used to apply from normalize.css and are now missing are:

table {
border-collapse: collapse;
border-spacing: 0;
}

and

td, th {
padding: 0;
}

Thank you @DyanneNova! It seems like the differences on tables and form line heights are small enough for them to be ignored, but it's good that they have been now listed here. Does the gallery only list URLs that have differences?

Would you be able to run the test also on Bartik and Seven?

Question about the CR. It says:

The Drupal core normalize.css has been updated from 3.0.3 to 8.0.1. There are lots of changes on the update so manual testing is strongly recommended to themes that are not extending Stable or Classy. Stable theme will continue using normalize.css 3.0.3 for backwards compatibility.

However, presumably, Stable 9 should use the updated version. Have we explored that yet? The CR should probably clarify that point as well.

@bnjmnm is going to run #97.

This is happening in Seven ☹️

I wonder if #100 is the same line height difference mentioned in #96?

I rescind my "needs work". Manual testing revealed that my finding in #100 occurs with both versions of normalize. The situation that showed up in the second screenshot, where the "or" is to the right of the select field, occurs (in Chrome at least) in a very specific pixel range 356px-361px. I suspect the 360px specified in wraith may have been interpreted as slightly wider/narrower in the test.

I ran regression tests on Bartik and Seven, using the same paths as the tests in #96. The only differences are related to form input line-heights, which I suspect are acceptable based on #97. It's also not surprising that this would happen, as normalize.css had previously used line-height: inherit, which means input line heights were not a fixed value throughout the theme. This results in slightly higher line-heights on some admin forms (the values I added were based on keeping entity forms consistent). This difference happens to be closer to what Claro is doing.

I attached two comparisons from Bartik. This is exactly what is happening in Seven as well so referencing those is applicable to both themes.

#102 looks like a pass on Seven and Bartik, so I think we can restore the RTBC here.

So i think the issue summary need to make it clear that themes extending from stable & classy are still going to using the old version. I also think the change record needs to detail this and it should detail how (if you not extending stable) you can do the same thing as stable if you are using the normalise library provided by core.

I think the approach here to update the library but continue to use the old one in core themes is a good one and we can deal with more fallout when we remove stable as a base theme for claro, umami, and seven.

I ran the same Wraith tests on Bartik and Seven and also found a small font size difference in form select fields, introduced by this rule:

select {
font-size: 0.8125rem;
}

Current

New

Difference

The rest of the diffs in the test all appear to be line height related.

Setting back to Needs work.

a small font size difference in form select fields

Nice find! I had a hard time spotting that from the full screenshots in #109, so here's just the select field cropped out from each of them. The first/current one is slightly larger than the second/new one.

I updated the CR with the info that was requested on #105.

I'll have the font-size issue addressed shortly.

This addresses #109 and applies similar fixes to Bartik

The location of Stable's normalize-fixes.css was also changed, since Stable's css/core directory already represents /misc. The css/core/misc directory is unnecessary.

I think both, #105 and #109 has been addressed. Since this is blocking multiple issues, we should fix rest of the regressions to core themes on #3110855: Plan for removing dependency to Stable in Bartik/Seven/Claro/Umami since they are not visible to users before that issue is committed.

Just noticed that the CSS changes cause CSSLint errors:

$ yarn run lint:css
yarn run v1.21.1
$ stylelint "**/*.css"

themes/bartik/css/components/form.css
 37:3  ✖  Expected "padding" to come before "border"   order/properties-order

themes/bartik/css/components/table.css
 13:3  ✖  Expected "border-collapse" to come before "font-size"   order/properties-order

themes/claro/css/components/form.pcss.css
 209:6  ✖  Expected newline after ","   selector-list-comma-newline-after

themes/seven/css/components/form.css
 25:3  ✖  Expected "font-size" to come before "line-height"   order/properties-order
 31:1  ✖  Expected no more than 1 empty line                  max-empty-lines
 38:3  ✖  Expected "margin" to come before "border"           order/properties-order

profiles/demo_umami/themes/umami/css/base.css
 269:3  ✖  Expected "margin" to come before "border"   order/properties-order

themes/stable/css/core/assets/vendor/normalize-css/normalize.css
 138:3   ✖  Expected "margin" to come before "font-size"                 order/properties-order
 147:3   ✖  Expected "color" to come before "background"                 order/properties-order
 166:3   ✖  Expected "position" to come before "line-height"             order/properties-order
 233:27  ✖  Unexpected duplicate name monospace                          font-family-no-duplicate-names
 259:3   ✖  Expected "margin" to come before "font"                      order/properties-order
 314:3   ✖  Expected "padding" to come before "border"                   order/properties-order
 378:3   ✖  Expected "margin" to come before "border"                    order/properties-order
 389:3   ✖  Expected "padding" to come before "border"                   order/properties-order
 418:3   ✖  Expected "border-spacing" to come before "border-collapse"   order/properties-order

That's among the more disappointing ways to realize that PHPStorm lint checking was only set up for my 8.x install.... Fix on the way.

Fixed

+++ b/core/themes/seven/css/components/form.css
--- a/core/themes/stable/css/core/assets/vendor/normalize-css/normalize.css
+++ b/core/themes/stable/css/core/assets/vendor/normalize-css/normalize.css

Let's actually revert these changes and add core/themes/stable/css/core/assets/vendor as ignored path in .stylelintrc.json.

Addresses #119 and an extra line in form.css not caught by stylelint autofix

Thank you! This addresses #116 ✨

Couple of questions...

1. +++ b/core/profiles/demo_umami/themes/umami/css/base.css
   @@ -260,6 +260,37 @@ label {
   +/**
   + * Prevent regression due to -webkit-appearance being set to button in
   + * normalize.css 4.1.0.
   + */
   +::-webkit-file-upload-button {
   +  -webkit-appearance: push-button;
   +}
   
   +++ b/core/themes/bartik/css/components/form.css
   @@ -87,6 +106,14 @@ textarea {
   +/**
   + * Prevent regression due to -webkit-appearance being set to button in
   + * normalize.css 4.1.0.
   + */
   +::-webkit-file-upload-button {
   +  -webkit-appearance: push-button;
   +}
   
   +++ b/core/themes/claro/css/components/form.css
   @@ -280,3 +280,26 @@ tr .form-item,
   +/**
   + * Prevent regression due to -webkit-appearance being set to button in
   + * normalize.css 4.1.0.
   + */
   +
   +::-webkit-file-upload-button {
   +  -webkit-appearance: push-button;
   +}
   
   +++ b/core/themes/claro/css/components/form.pcss.css
   @@ -198,3 +198,23 @@ tr .form-item,
   +/**
   + * Prevent regression due to -webkit-appearance being set to button in
   + * normalize.css 4.1.0.
   + */
   +::-webkit-file-upload-button {
   +  -webkit-appearance: push-button;
   +}
   
   +++ b/core/themes/seven/css/components/form.css
   @@ -353,3 +385,11 @@ select {
   +/**
   + * Prevent regression due to -webkit-appearance being set to button in
   + * normalize.css 4.1.0.
   + */
   +::-webkit-file-upload-button {
   +  -webkit-appearance: push-button;
   +}
   

   As this is going everywhere should it be in the root fixes file?

2. +++ b/core/themes/bartik/css/components/form.css
   @@ -13,9 +13,28 @@ form {
   +button {
   +  line-height: 1.21875rem;
   +}
   +input:not([type="file"]) {
   +  line-height: normal;
   +}
   +select {
   +  line-height: 1.5;
   +}
   
   +++ b/core/themes/claro/css/components/form.css
   @@ -280,3 +280,26 @@ tr .form-item,
   +button {
   +  line-height: 1.125rem;
   +}
   ...
   +input,
   +optgroup {
   +  line-height: 1.5rem;
   +}
   
   +++ b/core/themes/claro/css/components/form.pcss.css
   @@ -198,3 +198,23 @@ tr .form-item,
   +/**
   + * Prevent regression due to explicit line-heights applied to these elements in
   + * normalize.css 7.0.0.
   + */
   +button {
   +  line-height: 1.125rem;
   +}
   +input,
   +optgroup {
   +  line-height: 1.5rem;
   +}
   

   Should the line-height have units? There's one without - but the other have them. I'm not sure.

#122.1 Regarding the addition of -webkit-appearance: push-button; to each core theme. We've reserved the root fixes file for actual bugs in normalize.css, The SVG styling in the current normalize-fixes.css addresses a bug -- it reintroduces a rule that shouldn't have removed until IE11 was no longer supported. In the case of -webkit-appearance: it is a worthwhile addition to the library to have available to new themes, despite it needing addressing in each core theme.

#122.2 Right, that could look pretty odd without context. The issue being addressed here is that normalize.css 3.0.3 styled these input elements with line-height: inherit, so the line-height was often determined by the nearest ancestor that specified a value. Different ancestor elements used different (or no) units for line-height.

Credited reviewers - @andrewmacpherson got credit for the first suggestion of using libraries overrides to provide BC, @Cottser and @joelpittet for issue management as theme sub system maintainers back in the day.

@bnjmnm thanks for answering my questions in #123. I think we're okay to go forward here. Maybe some more things will be found when we decouple core themes from stable but I think we've gone as far as possible to limit the fallout.

Committed 952e939 and pushed to 9.0.x. Thanks!

Adding a link to the CR to the release note (always a best practice).

publish change record