Progress bar breaks flow of subsequent page elements

Added clearfix class to stable. Attached page for this

You have added the class cleafixinstead of clearfix, see the missing R.

We need to add the clearfix to classy and stable, I have reproduced the bug in classy and stable template... so the system template can be used with arbritrary theme and we cannot be sure that the clearfix class exists. I have added the class in core/modules/system/templates because I think that could be a good default class, 99% of themes have a clearfix class from core, and the theme can overwrite the progess template to explicite remove the clearfix class if it's desired.

Yes, we need to add the clearfix class in the system template, probably the same bug it's reproducible in bartik and seven.

Can anyone test it and set the status to RTBC?

I am at DrupalCon Dublin and I am going to test this (https://www.drupal.org/contributor-tasks/manual-testing)

Also at drupalcon, looking at this

Me and autopoietic have been able to confirm that this is working correctly

Let's add some before and after screenshots to help the committer!

Before & after screenshots attached

All ready! Thanks!

Before

After

Thanks - that shows the issue (and resolution) much better!

Bumping to 8.3.x, so that we can have the issue fixed in the future versions as well

Changing the status back to 'Needs review' just to see that the issue applies to 8.3.x

Screenshots updated with annotations

I am going to review to review the code change in the patch in #5 right now.
I am going to follow the recommendations as posted here: https://www.drupal.org/contributor-tasks/review

Changing back to RTBC - path applies to 8.3.x (also tested on simplytest.me), screenshots updated

@joachim: from #5

Reordering files to have the patch displayed

We checked if the patch in #5 applies to all files with class="progress" and/or data-drupal-progress
grep -RF 'class="progress"' core
grep -RF 'data-drupal-progress' core

There are no other files with class="progress" and/or data-drupal-progress.

Patch stays within the scope of this issue.

Not sure about Joachim's comment in https://www.drupal.org/node/2781165#comment-11659703, though. Should we open a new issue for it?

Trying to connect with Joachim but LimeChat crashed.

@criscom: it looks like my comment in #23 overlapped with your one. Didn't mean to cancel your intention to review the summary, sorry for that

#29 Quoting mortendk from DrupalCon Dublin

Fix should be done in core cause stable is a theme that people should be using. No need to wait for D9.

From https://www.drupal.org/core/d8-bc-policy

We will change the markup and CSS that core generates for user interface and theme improvements. Specific core themes (Bartik, Seven) are considered internal and may change. The stable base themes (Stable, Classy) are considered public API, with stable templates, markup, and CSS, so themes needing BC support should extend one of those base themes.

Basically that means that changes should not be made in stable and classy. Also before and after screenshots (maybe with markup snippets) need to be done in both seven and bartik and code changes for seven and bartik

--- a/core/modules/system/templates/progress-bar.html.twig
+++ b/core/modules/system/templates/progress-bar.html.twig
@@ -13,7 +13,7 @@
  * @ingroup themeable
  */
 #}
...
+<div class="progress clearfix" data-drupal-progress>
   {% if label %}

and keep this part.

This is a bug fix in classy/stable so different story than the quoted policy in #33. I'm assigning to Cottser to help sort out the policy issues on this. If it's not right in system it should be fixed there too.

Actually I'm going to make a decision on this, @Cottser is a committer too so he can veto. But due to this being a bugfix it should be in Core, Stable and Classy as it is in #5

I'm kind of thinking we remove the fix from Stable and leave the other two while discussing with @lauriii in person since Stable is expected to not have classes(maybe even some utility classes like clearfix? which is the part I'm fuzzy on). Leaving this up to @Cottser for the decision.

Whoops meant to leave it RTBC

This issue would yet again benefit of solving this issue #2659890: [Policy] [Plan] Drupal 9 and 10 markup and CSS backwards compatibility since we could do classy fixes in one place and people could copy these fixes from there (or even use directly). Then we wouldn't necessarily have to fix this in Classy.

However, I don't think it is necessary to fix this in core since this is a bug, but it doesn't break any functionality. It just makes things look awkward. Are there other places that look awkward on Classy and Stable? Totally. This is more of a nice improvement for the default stylings than anything else.

Thanks for the work so far, everyone :)

Stable (and core) shouldn't have any visual classes, only functional things, like for example js- prefixed classes. As an example, some people don't want to have a clearfix class and will want to use a CSS-only clearfix applied to different elements. Stable should let them do that.

Along the lines of what @joelpittet was saying in #36, I don't think we should be adding new clearfix classes to Stable or core. So that leaves us with Classy, Seven, and Bartik. In this particular case IMO we should just suck it up and update Bartik and Seven. Leaving Classy, Stable, and core alone and "stable". This fixes the bug for a good chunk of scenarios and is in line with the policies that have already been documented above.

@joachim: classy, stable or core doesn't provide any visual design and therefore isn't usable out of box. The designer who is building themes is responsible for making the site work visually.

I think agree more with @joachim on here because the CSS that causes the visual bug was written in core (see referenced CSS below) and was not caught during testing.

core/modules/system/css/components/progress.module.css:34
core/themes/stable/css/system/components/progress.module.css:33

We could actually fix this entirely in CSS but the CSS would be added to Stable and Classy and core too.

Eg.

.progress:after {
  content: "";
  display: table;
  clear: both;
}

Using the .clearfix utility class seemed to be the better way to fix the problem and quickly discussed with @mortendk last week and he agreed.

Since the problem seems to be an accidental due to removal of this class, I see this as a bugfix as it is categorized.

When Stable and Classy were cut for 8.0.0 release the float: left/right were already in place but weren't tested for this use-case with items under.

I think system/Core should not be 'fixed' and represent the direction we want to go with this markup and classes. Stable IMO should be fixed due to the location of the bug.

Ok, if I collect all opinions:

Should we fix the bug?

• In core: no
• In classy: maybe
• In stable: maybe

So, new patch acording this...

Hmm - I'm a little confused by this. My understanding from way-back-when was that the purpose of Stable was to provide the 'stable' base. So, it allowed us to fix things in core and Classy, while Stable always represented a consistent point in time.

So, Core and Classy see fixes over time but Stable always gives consistent (even if 'wrong') output.

If (!) this is still true then...

We fix in Core and Classy BUT change Stable to reverse any change made in Core so that Stable is, well, stable...

So, it allowed us to fix things in core and Classy, while Stable always represented a consistent point in time.

Classy is as frozen as is Stable. There's proposal to add unstable Classy to core to allow this kind of changes to be included #2659890: [Policy] [Plan] Drupal 9 and 10 markup and CSS backwards compatibility

It would be useful to be able to point to policy documents not just on the frozenness or otherwise of core, Classy, Stable, Seven, Bartik etc: but also, what does frozen mean? When we used to freeze Drupal core for a major release, didn't we still permit bugfixes; wasn't that, in fact, the whole point? (Note that the policy page linked to doesn't even use the word "frozen", so maybe we shouldn't even use it.)

If this counts as a bugfix (and I think it's telling that, as far as I can see, nobody has changed this issue's category from "Bug report" since it was created) and if "frozen" only means "frozen to new features", then this bugfix should go in *everywhere* that's affected, and exactly where it gets changed becomes a matter of what code inherits from what, to make the change the simplest.

This issue was discussed in #bugsmash, with smustgrave, darvanen,and mstrelan. It was decided that this belongs in the contrib project. And that decision received support by larowlan and Tinto.

The Problem still exists in following themes: demo_umami, claro, stable9, starterkit_theme

I have provided a patch for same, Please review

In 2023, clearfix is not the answer, regardless of the question.

I think we can fix this by moving to modern CSS, instead of using float left and right, we can make .progress display: flex and position the percentage and description using flexbox, this fix would go in system module's progress.module.css

After that, Claro's ::after attribute to add clearfix (without the clearfix class) can probably be removed too.

Umami, classy etc will all likely be fixed by changing from float to flexbox and that means we don't need any fixes in templates, which means we have no BC issues.

@larowlan, thanks for chiming in.

I have updated the remaining tasks and the proposed resolution has a link to comment #65.