🇺🇸United States @caesius

It sounds like this was probably fixed in the latest Drupal core update, although the vulnerability is only detailed for email addresses, not usernames. https://www.drupal.org/sa-core-2024-004 →

Hi, just going to quickly note that this project is considered "Feature Complete," as such I won't be very active in participating in feature requests. However, if this work (or any other Feature Request for that matter) turns out to be really solid and bug-/consequence-free, then I'll happily merge it -- though probably only into a new 2.0-alpha branch.

Otherwise, I'll be pretty hands-off on this and won't be reviewing work until it's marked RTBC.

See ✨ Allow other entities to use this feature as well Active -- there's active work in progress on this feature.

Duplicate asterisks are a known issue, as is the fact that this bug seems to mainly if not exclusively manifest when using the Field Group module and we need a way to replicate the issue in core. These are noted in the issue summary in the Remaining Tasks section.

However, I believe this issue was originally created in and remained a core issue because there is at least some degree to which Claro's code does not make sense and makes it more difficult than necessary for Field Group to do its thing. The fact that this issue is four years old and has not been fixed within the Field Group module with its hundreds of thousands of installs says as much.* As does the involvement of frequent Drupal core contributors in this issue, none of whom seemed to question whether this should be fixed in Field Group -- including xjm in #56, who funnily enough moved the linked issue to the Field Group queue. (also funny how these issues keep getting tagged as "Novice" issues that'll be fixed at various Drupalcons...)

So what someone needs to do is actually look at what Field Group does against what Claro is doing and see which one is behaving incorrectly. If Claro is spitting out code that is needlessly hard for modules to work with, then it should be fixed in core. Otherwise the issue should be tossed over to Field Group and kept there.

*alternatively, everyone just forgot Field Group isn't actually a core module...

The above patch is almost identical to the MR except it removes the newly-added src/ThemeResolverInterface.php and src/ThemeResolver.php files.

I had no issues applying the MR to either the current 8.x-2.x-dev or to 8.x-2.15, so I don't see any reason for #50 to have been created. Please be more careful when creating patches.

To add: The issue with the tabindex=1 is that it modifies keyboard navigation such that the first item you end up tabbing to is the "Add to Calendar" button. This breaks accessibility standards for pages that have it.

I came across this and investigated it. It seems the Addtocalendar Javascript updates the tabindex:

https://addtocalendar.com/atc/1.5/atc.min.js

k.tabIndex=1;

I don't know why it does this.

I was able to fix this by renaming a template like views-view-unformatted--block-1.html.twig to views-view--block-1.html.twig

Doing stuff like changing {{- rows -}} to iterate over and display {{- row.content -}} instead only resulted in empty views.

I have a migration that breaks after updating to the latest version of Migrate Source UI which I traced to this change. Does this update require updating configuration for old migrations?

What happens is that existing content gets overwritten exactly as intended by the source ID (here called MatchKey), however items new to an import do not get imported due to this error:

Migration failed with source plugin exception: 'MatchKey' is defined as a source ID but has no value.

I believe removing the Migrate Source UI version of MigrateBatchExecutable causes the migrations to use the Migrate Tools version of the same class, which for whatever reason does not work with the code or configuration used for our migrations. We're using the latest version of Migrate Tools.

I looked through the release history and issue queue for Migrate Tools but couldn't find any indication of what updates need to be done.

I'll try to debug this more and may split off a new issue to the effect of "Document this breaking change" but wanted to flag this to the people who worked on this update.

Ah, I didn't realize there was a new release with this since there wasn't a security advisory. Thanks!

Since the security vulnerability is now public, will Drupal core get a security release on-cycle next week on the 18th or might it be released sooner?

I'm seeing duplicate red asterisks on tabs after applying the above patch. There's a workaround I'll describe below.

This issue was definitely left in a sort of limbo, probably due to the need to find a way to test this solely in Drupal core and not rely on the Field Group module, but I think #74 was the previous "best fit" patch for this issue, rather than #79.

To recap, in #74 I removed a block of code that I found unnecessary for reasons I described in more detail in #80. #79 added the block of code back in due to a misunderstanding of where a rogue asterisk was coming from (it was Field Group's fault, not Core's), and I described a workaround for it in #82:

Also, using patch #74, I can avoid getting a "rogue" asterisk on a fieldset simply by unchecking "Mark as required" for it, which still allows the "required-ness" of fields it contains to percolate up to parent field groups.

So, attaching a 10.3.x reroll of #74. If you see duplicate red asterisks on tabs then you probably need to apply this workaround regardless of which patch you use. The previous patch does seem to work for specifically preventing "fieldsets" from duplicating asterisks, but I'm adamant that adding .required-fields to all field groups only to selectively remove them right after is the wrong approach.

This update should be reverted per the above comment and issue 💬 Revert requirement that IDs not include spaces Needs work

I'll also note that per the issue title and description, the resolution should have been to "Document that column names have to be alphanumeric characters or underscores" and yet the actual implementation prevents the use of spaces in column names entirely. This breaks backward compatibility with migrations that were created before this change was implemented and does not document an upgrade path.

I tested this on Paragraphs dev and it looks fixed. Thanks!

So you're suggesting that they release a 3.7 version that can't be installed on Drupal 10.2, and then potentially later release a 3.8 with a security fix and rollback of this code that can be installed on 10.2, and then release 3.9 with the code re-removed, composer constraint re-added, and the security fix in place?

I think you're missing the point of semantic versioning. Field Group 8.x-3.x has 200,000 installs and is so frequently used that issues for it are sometimes filed to core 🐛 Field Groups marked as required are missing red asterisk Needs work . As such, the community needs to be confident that it is stable, and it needs to be available for all supported versions of Drupal, potentially even in-development versions such as Drupal 12.

If this code is absolutely required for automated tests to pass then it should be added to the project, but it can't be done in a way that introduces uncertainty about which version of the module is actually compatible with which version of Drupal. I don't understand the reluctance and obstinance of module maintainers to actually release a new major version of a module when making backward incompatible updates or potentially breaking changes. If any module needs to set an example for how to properly maintain versions and apply semantic versioning, it's this one. "It's a hassle" is not an excuse to do otherwise.

This change cannot be applied to Field Group 3.x. This module is covered by the security advisory policy, which I haven't thoroughly read, but I assume it implies that any security updates to a module must be applicable to all supported versions of Drupal. Preventing Drupal <10.3 from receiving new updates to the module would inherently prevent any potential security releases from being applied, which would be a huge problem. Drupal 10.2 is supported until the release of 10.4, so probably until December.

As mentioned, a new full release of this module is required for this code update. BC incompatible changes usually imply a new major version, though some like Linkit → opt for maintaining separate minor versions, i.e. 6.0.x and 6.1.x. However, Field Group is still using 8.x-3.x with no capability for distinguishing minor and patch versions, so a 4.0.0 release is the only option.

^ Not recommended on 3.x; see comment on other issue.

Simply updating the version constraint would not resolve the issue or change the workaround, which currently is to explicitly composer require drupal/field_group:3.4.0

If requiring field_group:^3 then Drupal 10.2 sites would still try to update to the bad 3.5 version as it's the latest 3.x that is listed as compatible with 10.2. There is no way to go back and apply a constraint to the current 3.5 version.

Fixing this absolutely requires that 🐛 Fix deprecated extending of render element Fixed be either reverted or made backward compatible on 3.x. If it can't be made backward compatible then 3.5 can be rereleased as 4.0.0 with version constraints so they have something to run automated tests on without them failing.

This is also an opportunity to update to proper semantic semantic versioning as this module still uses the old 8.x-X.Y format instead of the modern X.Y.Z

This is actually broken through Drupal 10.2 and not specific to Drupal 9 support.

This seems to WSOD on Drupal 10.2.

If you are on Drupal 10.2, downgrading to Field Group v3.4 should help.

Since you filed this issue a day before v3.5 released, you should check if you are requiring the dev version of the module without pinning to a commit hash.

caesius → created an issue.

Tested in both Gin and non-Gin admin themes, as well as with Gin as the default theme and "Use the administration theme when editing or creating content" unchecked.

Also tested that update hook works as expected.

This looks good to go.

Another possibility if the intent is to simply save work in progress is Autosave Form → .

I don't think the Save & Edit module should itself support bypassing required fields, since even though the intent of this module is to facilitate drafting content, there's currently no mechanism to prevent Save & Edit from appearing on published content.

However, I would support adding this functionality providing the following conditions are met.

• Add a configuration option to enable saving content without filling in required fields (except the title/label).
• Add a completely independent configuration option to not allow Save & Edit for Published to Published workflows generally.
• Add a configuration option that is dependent upon the above two options whereby if the former is enabled and the latter is not, disable the ability to Save & Edit from Published to Published if any required fields are not filled in.

Obviously this is fairly complicated so it's probably more efficient to just rely on a secondary module to get "close enough."

caesius → made their first commit to this issue’s fork.

Thanks! Confirmed this works and merged into dev.

Leaving a quick comment here as this is one of only two Google search results for "undefined field index_id" although my issue did not appear to be related to this (fixed) bug.

Per comment 12 on 🐛 Exception: "400" Status: undefined_field_index_id: undefined_field_index_id Fixed , uninstalling and reinstalling search_api and search_api_solr resolves "undefined field index_id" error for me on Drupal 10.

For the record (as this is only one of two results in Google for this issue)

Per #12, uninstalling and reinstalling search_api and search_api_solr resolves the issue for me on Drupal 10.

This was solved for case sensitivity by adding the function caseInsensitiveArrayIntersect(). Maybe that should be replaced/updated with a function that instead uses something like iconv() as suggested here https://stackoverflow.com/questions/471021/comparing-strings-in-php-the-...

The latest posted patch fails to apply on Drupal 10.2.x. It seems to just be a merge conflict on the tests though, so the underlying code should still be good.

Posting reroll for 10.2. I did not take the above suggestion re: typehinting into account as I was focused on just making a working patch.

However, I'm having trouble verifying that this still does what I thought it was supposed to do. I'm certain I originally applied this patch so that custom validators I implemented on taxonomy terms run when migrating content via CSV. This would ideally have two effects:

1. Nodes that try to create taxonomy terms with invalid names fail to import.
2. Taxonomy terms with invalid names are never created in the first place.

The first point always happens with or without the patch on both 10.1 and 10.2. The second point, however, does not -- the invalid taxonomy terms always get created and pollute the term list without getting referenced. Surely when I applied this patch it resolved one point or the other?... unfortunately I can't really revert the site back to 9.x to check.

Seems like you need to use the machine name of the widget as found at /admin/structure/rate_widgets i.e. rate_machine_name

e.g. if the machine name is community_likes you would use content.rate_community_likes

Thanks!

And just to clarify the underlying issue: It does seem that updating a site from <2.0.3 to 2.0.3/4 may automatically evaluate consent mode to be enabled for existing containers even if the form is never touched, at least from looking at the updated code.

So for non-EEA clients where the whole "consent mode" thing was likely never an action item, this bug seems bound to affect anyone who updates the module if they don't manually go in and uncheck the checkbox and then export configuration. In other words, vendors simply doing their duty to update modules to the latest patch version will inadvertently trigger analytics data loss when they push to production.

There's been a lot of comments already about data loss and requests for a new release, but just pointing out that the affected audience is probably significant.

If anyone needs a composer require command that'll grab the dev version until a stable version is tagged, this should do it.

composer require "drupal/google_tag:^2.0.5 || ^2.0@dev >2.0.4"

Also, this module is supposedly maintained by Acquia and yet they don't seem to be aware of this issue that's affecting non-EEA clients. If anyone has a line to Acquia to let them know that a module they supposedly maintain has a critical data loss issue in the latest stable version, maybe that'll prompt a stable release to be tagged.

What's the MR for? I already committed the .info.yml change to include || ^11. I don't see any changes in the opened MR, and I still wanted the D11 update bot to continue to apply future fixes.

If this is an attempt to snipe issue credit without actually doing anything, then please don't waste my time.

This behaves as expected. However, it needs an update hook to add the new enable_node_types_automatically setting to save_edit.settings config on existing installs. This should of course default to FALSE.

I assume the setting you're referring to is the "Hide the Publish button" setting. I have never used it myself and am not entirely sure what it does, but I suspect it became obsolete in Drupal 8.4.0. I took over maintainership of this module mostly just to handle the Drupal 10 fixes so it still has functionality I'm unfamiliar with.

See:
Remove Dropbutton support to resolve theming issues 📌 Remove Dropbutton support to resolve theming issues Fixed
"Save and Publish" button on entity forms changed to checkbox →
Change "Save and keep un-/published" buttons to a "Published" checkbox and an included "Save" button →

If someone can confirm that this setting does absolutely nothing on Drupal 10, then feel free to open a merge request similar to the one in the first link.

I agree that getting this into a 2.0.5 release asap is critical. Also, information regarding consent mode (and perhaps a mention of this bug) should probably be included on the project page. Since 8.x-1.6 has been out for about 16 months, maybe that "critical notes" section can be replaced? (keeping project pages short is important IMO)

^ That is a known bug with the latest patch as noted in the IS "Remaining Tasks" section.

It doesn't appear that the module maintainers have taken notice of this critical issue yet. I'll reach out to them.

I believe the best patch for anyone experiencing this issue is #9. It's a one-liner and very straightforwardly allows you to just go in and uncheck the checkbox in question without it resetting the next time you open the page. The setting itself will work as expected.

I would not recommend using #25 (or the merge request it's derived from) in a production environment since it includes a database update hook. The module maintainers could very easy roll out another stable release with a different update hook that's named identically.

Wouldn't this code automatically update the consent mode checkbox to be unchecked even in cases where uses wanted it to be checked?

/**
 * Update google_tag containers and set the consent_mode to FALSE.
 */
function google_tag_update_8202(&$sandbox) {
  $entities = \Drupal::entityTypeManager()->getStorage('google_tag_container')->loadMultiple();
  /** @var \Drupal\google_tag\Entity\TagContainer $container */
  foreach ($entities as $container) {
    $advanced_settings = $container->get('advanced_settings');
    $advanced_settings['consent_mode'] = FALSE;
    $container->set('advanced_settings', $advanced_settings);
    $container->save();
  }
}

I think we should only explicitly set consent mode to FALSE here if the config is empty. If it's TRUE leave it.

This checkbox doesn't exist to just to break Google Analytics on sites. Per the 2.0.3 release notes → sites that need consent mode enabled will have GA break without it.

No matter what we do here there will be users who update the module and end up with broken GA, but we need to do our best to ensure the following:

1. Users who are updating from <=2.0.2 have the checkbox unchecked by default.
2. Users who are updating from >=2.0.3 and have not touched the consent mode config from what is already provided (i.e. "enabled") have the checkbox checked by default.
3. Users who are updating from >=2.0.3 and appear to have explicitly set the config will have that setting persist.

If it's not possible to determine whether the checkbox was deliberately checked or unchecked then 2 and 3 would be impossible to implement together, in which case we'd need to pick one. We'd then need to explain the issue in the release notes and possibly the project page to maximize visibility.

^^ Be aware that the last two releases → (2.0.3 and 2.0.4) include updates to add consent mode, and crucially in 🐛 Config schema missing for consent mode Fixed to enable it by default when updating the Google Tag module, which seems deliberate (even if probably every person coming to this issue would disagree with that decision).

Updated title to quote checkbox name since it could have been read as a request to enforce that the checkbox is checked.

Anyway, I've just tested the other patch and it also appears to work. Not sure what's up with the different paths between the two files but as long as they commit to the actual project identically then it's a non-issue.

I have tested this on Drupal 10.1 on Group 2.x with an actual project (and been using the patch on production for months) and it checked out fine there.

However on Drupal 10.2/Group 3.2.2 with a sandbox project I should note that applying the patch on an existing installation results in a WSOD:

In DiscoveryTrait.php line 53:

  The "group_permission" plugin does not exist. Valid plugin IDs for Drupal\views\Plugin\ViewsPluginManager are: perm, role, none

I have no idea if it's a "just me" issue or not, but this definitely needs more testing on latest Drupal+Group 3.2.2 to confirm that committing this won't hose existing projects. I've reinstalled several times and I get the error whether I apply or un-apply the patch after installing and setting up Groups.

I didn't write the patch though, so make sure to credit RichardDavies -- he got to this way before anyone else :P

seanB's patch appears identical to the one I initially attached to this issue; I took it from RichardDavies on the parent ticket: https://www.drupal.org/project/group/issues/3029908#comment-15205403 ✨ Add revisions tab on groups Fixed

Since this checkbox can cause google analytics to outright not work on sites that need this option disabled, and given that someone can very easily visit this configuration page while GA is working, notice the box is checked, and assume nothing needs to be done with it before saving, I personally consider this bug to be Major.

Works for me also.

caesius → created an issue.

I was actually asking about any theoretical "similarities" that can occur in the data comparisons besides case sensitivity. For example, 0 == FALSE (a stretch, I know) or encoding issues.

It's probably fine to only check for case sensitivity for emails and usernames, and it's also probably unlikely that there's anything else to consider here, but still something that should be pondered for a minute before moving on IMO.

Are there other instances where two values may be considered the same by a database query but not in a PHP function like array_intersect?

What may need to happen is that this condition get updated to build out a 'LIKE' condition for each item rather than simply an 'IN' condition.

Building a 'LIKE' condition in Drupal

Note that you can't combine 'LIKE' and 'IN', but a loop should make short work of it.

The tests added in 📌 UniqueFieldValueValidator works only with single value fields Fixed should be easy to expand to check for case insensitivity.

I was not able to reproduce the issue on a vanilla Drupal 10.2.2 install, meaning a completely fresh installation with the standard profile and only the Save & Edit module installed+configured. I added an image to the "Image" field on the out-of-the-box "Article" content type and it saved just fine. I even tried editing the field to be multi-valued and had no issues.

Are you certain the issue is with an "Image"-type field and not, say, a Media or other Entity Reference-type field? The "Advances Image" field does not look at all familiar and appears customized.

Please provide specific, step-by-step instructions for reproducing the issue on a freshly-installed Drupal site, including installation and configuration of only the specific contributed modules that are necessary for the issue to occur.

If the issue only occurs with the addition of custom code then it's probably not an issue with Save & Edit and the issue will probably be closed, or at best updated to a "Support Request" with "Minor" priority.

I was able to replicate the issue on a vanilla Drupal 10 install. See screenshots.

Unexposed DESC sort followed by exposed sort; note that the SQL shows "ORDER BY DESC" for both sorts:

However, the default sort for the exposed filter is actually ASC:

caesius → created an issue.

Please read the issue description.

This patch only works on Drupal 10.1.

Drupal 10.0 is not EOL until December 13th and even then it would be up to the project owner to decide whether to apply this patch as-is, making the project incompatible with Drupal <10.1.

If you're able to provide a patch that also works on Drupal 9.5 and Drupal 10.0 then please do so.

I can become a maintainer, sure. I already have experience getting older projects up to date.

Follow-up ticket created at 💬 Is Hierarchical Term Formatter still being maintained? Active

caesius → created an issue.

Split off a new ticket to address the issue with duplicate Revisions tabs. 🐛 Drupal 10.1: Revisions tab appears twice on Groups Needs review

caesius → created an issue.

I think what we need more anything else is a way to test these changes in Drupal core without using the Field Group module at all, e.g. render array code that could be placed in a custom module or theme to render all the "types" of field groups that come with Drupal. Anything left over should be addressed in the Field Group module.

Also, using patch #74, I can avoid getting a "rogue" asterisk on a fieldset simply by unchecking "Mark as required" for it, which still allows the "required-ness" of fields it contains to percolate up to parent field groups.

So, for each of your suggesstions:

KEEP - The CSS updates for ":not(.claro-details__summary).form-required::after".

Yes, for now, assuming we don't find some better way of accomplishing this without excluding such a specific CSS class.

REMOVE? RenderElement: if (isset($settings['options'])) {

I agree that this needs to be removed, as unset() doesn't raise an error if a nonexistent array key is unset (I just tried it on PHP 8.1) and wrapping unset() in an apparently unnecessary if(unset()) isn't a code standard in Drupal core.

KEEP - RenderElement: $complete_form[$group]['#required'] = TRUE;

Again, we'll keep this for now if we don't find a better way.

REMOVE - The class removal in the RenderElement.

Definitely.

So am I reading the code correctly that we are:

- Adding #required to field groups, which in this context is only for the purpose of ensuring it gets .form-required for the red asterisk (rather than validation considerations)
- Checking whether the field group we just made #required is a fieldset, in which case we remove .required-fields, even though it only had that class because we made it #required?
- Ensuring .form-required items get the red asterisk, unless it specifically also has .claro-details__summary

This also seems backwards to me. I think that we should ensure that we are only adding #required on items that actually need it, e.g. tabs, and ensure the PHP code is completely agnostic to the CSS. The current patch kinda seems to be taking two different approaches in PHP and CSS to tackling fundamentally the same problem (duplicate asterisks).

Also, am I the only one who noticed that the only way to test this is to install the Field Group contributed module? Is there no way to confirm the issue and test it just in Drupal core? I skimmed and didn't see a discussion on whether this is something that should be addressed in the contrib module or in core. Is this actually an issue with core that happens to be most visible when using a popular contrib module?

Doesn't this still need tests? We're no longer removing a CSS class but I believe we still need to check whether affected elements are getting the required class.

Switched to using an MR. The crucial difference from the previous patches is the use of an elseif so as not to override use_raw_value.

Updated to apply to latest 3.x-dev.

caesius → made their first commit to this issue’s fork.

The actual issue appears to be that in Claro, the widget is given the form-actions class which makes the entire thing a flex container. This causes every DOM node in the widget -- the button, the word "to," and the <em>phasized text -- to be considered an independent flex item with no gap between them. I tried column-gap but this put much more spacing after the word "to" than before it (i.e. after the button) even after removing the margin for .placeholder

I think we should ensure block display of form-actions for the paragraphs widget to avoid the issue and ensure consistent spacing between themes. This also means we can get rid of some of the other Claro-specific CSS rules. Screenshots of Claro and Seven with this patch applied.

For the record, these patches seem to cause minimal changes/disruption in Seven, per the attached screenshots. The only downside to Patch #9 is that it adds more margin to multiple-item dropbuttons than it does to single-item dropbuttons, but this should be an okay trade-off when considering the theme is deprecated.

That said, other admin themes may end up seeing the same thing, so perhaps this is a Claro core issue where the right-hand spacing for multi-item dropbuttons is nonexistent compared to single-item buttons.

Since a release candidate is available, we can now just require simplesamlphp/simplesamlphp:"^1.19 || ^2.1" and drop dev-simplesamlphp-2.1, assuming 2.1-rc1 is still chugging along fine.

Also, since it seems people are currently using the 4.x-dev branch, we may want to just release this as 4.0.0-alpha1 with the intention of fully releasing 4.0.0 once 2.1.0 is available. Anyone updating this module for Drupal 10 compatibility would need to upgrade the simplesamlphp library and externalauth modules by major versions anyway.

Well, scratch that -- I'm having a lot of trouble getting the MR up-to-date even when following these instructions → so I'm just gonna post a patch.

If someone could just nuke the current MR and make a new one that would be very helpful -- I don't think we have enough changes going on here to merit trying to salvage an MR that's stuck on 9.2.x.

Anyway, this patch is identical to the above but removes the code to unset a class. I haven't tested this thoroughly but it seems to work when using horizontal tabs.

If there are any issues we should try to resolve them in CSS, or if the issue is the class we were previously trying to remove then we should:

- Document what CSS class is actually causing the issues
- See if we can resolve the issue further up the pipeline by avoiding adding that class in the first place (if CSS isn't an option)

Is it safe to assume that the problem class was .claro-details which is now wrapped in :not() in the CSS?

+          if ($complete_form[$group]['#type'] === 'fieldset') {
+            unset($complete_form[$group]['#attributes']['class'][0]);
+          }

This code is really enigmatic and should probably be removed. What class is being unset? How do we know the first element actually is the class we're trying to unset? If this is needed then we should be looping over the ['class'] array and unsetting the specific item that's causing issues, but it would be better to resolve whatever issues are coming up in some alternative way.

I tracked this to #50:

Confirmed I am seeing the issues in #44.

Appeared the form-required class was being added to both the legend and the span.

Took a shot at trying to remove from the legend.

The issues in #44 refer to duplicated red asterisks which have persisted through relatively recent patches but seem to have been resolved by the latest ones.

I'll also point out that this issue has an MR open. We should be working from the MR, not posting patches.

Correction:

It does seem that the 2nd patch addresses this issue better than the 1st one, since it appears the 1st patch also inadvertently affects single-paragraph-type buttons without the dropdown.

1st patch:

2nd patch:

You'll see the 1st patch adds undesired extra margin to the right of the button.

Attached are videos showing the issue on fresh installs of both Drupal 9.5 and 10.1 and then testing both patches in this issue. I could not see a difference between the patches on either version of Drupal and I certainly didn't see the problem @Berdir mentioned.

As far as I'm concerned either patch works perfectly fine, but I'm really confused by Berdir's comment re: spacing problems. Someone even minimally familiar with this project's CSS should make the decision on which patch to apply, but FWIW I've been using the first patch on a project for a few months now and have not noticed any issues.

I'll mark this RTBC and a maintainer can make a final decision.

I was able to replicate the issue on a fresh Drupal install, as demonstrated by the attached video. The error appears at 7:05.