Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

For web-compat, please avoid using negative margins instead of defining a proper width. #35277

Closed
3 tasks done
wisniewskit opened this issue Oct 26, 2021 · 14 comments
Closed
3 tasks done

Comments

@wisniewskit
Copy link

Prerequisites

Describe the issue

As was discovered at https://webcompat.com/issues/90988 (and others), there is a web-compatibility issue with bootstrap where it is relying on quirky Chromium behaviour. In short, relying on negative margins on items to force them to wrap, in this kind of pattern:

.row {
  display: flex;
  flex-wrap: wrap;
  margin-left: -30px;
  margin-right: -30px;
}

In this kind of case, using width is more interoperable and reliable:

.row {
  display: flex;
  flex-wrap: wrap;
  width: calc(100% - 60px);

The code relying on negative margins appears to be here, according to @bfgeek on the webcompat issue. Would it be possible to alter it to use calc as above?

Reduced test cases

n/a

What operating system(s) are you seeing the problem on?

Windows, macOS, Android, iOS, Linux

What browser(s) are you seeing the problem on?

Firefox

What version of Bootstrap are you using?

5.1.1

@mdo
Copy link
Member

mdo commented Oct 27, 2021

Is this only a Firefox issue? If so, why does this affect Firefox only? Very hesitant to make a change here that could affect how customizable and approachable our grid system is today. I'm also thinking about how we're likely to shift to something with gap in the future and avoid this all together.

@WinterSilence
Copy link
Contributor

@mdo many browsers base on Chromium

@wisniewskit
Copy link
Author

wisniewskit commented Oct 27, 2021

@mdo , using negative margins in this way is relying on an acknowledged bug in Chromium, as linked. In fact a Chrome dev suggested that I file this issue, so it doesn't strike me as something I would want to rely on, especially if an interoperable drop-in replacement seems to exist.

Is there a reason why the width method strikes you as any less customizable or approachable as negative margins? To me it speaks at least as clearly of your intent, while not risking running into other unexpected quirks with negative margins.

Edit: as WinterSilence mentioned, Edge and Opera and Brave and many other browsers use Chromium under-the-hood, so it shouldn't be a surprise as to why they would "work", since they would share the same Chromium bug.

@mdo
Copy link
Member

mdo commented Oct 27, 2021

I understand many browsers are based on Chromium—the question was in regards to the issue as reported specifically mentions Firefox and no other browser. I'm very hesitant to change something here without fully understanding what's happening given this technique has been in use for the nearly 10 years we've been working on Bootstrap. That's not a reason to keep it, but it's also a reason to not walk away from it outright.

Looking at the page at https://curriculum.gov.bc.ca/curriculum/arts-education/10/media-arts, I'm super confused by the HTML in here. Feels rather convoluted and there's too much float: left and width: 100% happening for what appears to be no reason. Clearing that out prevents the issue entirely fwiw.

@wisniewskit
Copy link
Author

wisniewskit commented Oct 27, 2021

That's exactly the reason why relying on negative margins is a risk, especially for a framework used by many sites. They have subtle, not-well-defined behaviour, and won't always behave as you might expect, especially with floats and such. In this site's case, it's Firefox that's bitten, but Webkit is also very different from Chromium these days and doesn't share all of the same layout quirks and bugs anymore.

If you want to better understand the underlying reason, the Chromium team has some notes in the crbug. But from what I can tell, Safari and Chrome still have enough code in common that they share this bug, while Firefox does not. So that's why only Firefox seems affected, whereas all three seem to work fine with the width example.

Not that I mean to call you out on this or anything; I'll understand if you don't feel it's even worth trying this. But it's just a race to the bottom in that case. How likely are individual sites to fix this, if bootstrap isn't? We'll ultimately all just end up stuck with the buggy and subtle behaviour as Chrome won't be able to fix it, Firefox will have to burn time adopting Chromium bugs instead of working on other things web devs want, etc. I'd rather try to avoid that if possible.

@karlcow
Copy link

karlcow commented Nov 4, 2021

@mdo

Using the test created by @dholbert
https://jsfiddle.net/dholbert/ujphfkqz/

From left to right:

  • Gecko. Firefox Nightly 96.0a1 (2021-11-02) (64-bit)
  • WebKit. Safari Tech Preview Release 133 (Safari 15.4, WebKit 17613.1.2.2)
  • Blink. Edge Canary 97.0.1064.0

Capture d’écran 2021-11-04 à 10 43 42

If you do not want to modify the negative margin, maybe you could instead add the rule width: fit-content;

.row {
	display: flex;
	flex-wrap: wrap;
	margin-right: -15px;
	margin-left: -15px;
	width: fit-content;
}

@karlcow
Copy link

karlcow commented Dec 9, 2021

And yet another site breaking because of this.
webcompat/web-bugs#95687

@mdo
Copy link
Member

mdo commented Feb 11, 2022

Unfortunately most of the options don’t exactly do what the current Bootstrap grid behavior does. Check this Codepen where I’ve shown the current sizing, with the calc() functions, and with fit-content. Only the last option works, assuming there’s no change to the negative margins, but I’m not sure that fixes the issue with Firefox. Can someone verify?

https://codepen.io/emdeoh/pen/JjOJrro

@wisniewskit
Copy link
Author

@mdo thanks for circling back to this! If it helps, I'm seeing the same rendering and responsive wrapping behavior on all of the lines, in Chrome, Firefox and Safari on my Macbook.

But I'm not 100% certain of what your expected behavior here is meant to be, so if there are other cases/tests for which you'd like the rendering to match across browsers, would you mind sharing them all (assuming that's practical)?

That would help us narrow down any interop issues and hopefully find variants which pass all the cases that are deemed important.

@mdo
Copy link
Member

mdo commented Feb 11, 2022

I'm on my phone right now, but this should still illustrate the problem I see with the suggested changes. Note the difference in where the red background of each column starts and ends.

image

@wisniewskit
Copy link
Author

Forgive me for being slow on the uptake here, but it looks to me as though the browsers are all just doing what you're asking them to for flexboxes that are assigned different widths;

Row 1: 100% width, so no empty space, the cols are distributed with default of even width.
Row 2: 100%-1.5rem width, so 1.5rem rem of empty space, not distributed in any particular way (as that was unspecified).
Row 3: 100%+1.5rem width, which I believe is considered nonsensical, but at least seems to render the same across browsers (though I wouldn't be surprised if there are inconsistencies).
Row 4: fit-content, so the width of all items is only as wide as it needs to be to contain the content not 100%, again leaving the rest of the width as empty space.

What were the results you were hoping for? Was it perhaps for the empty space to be distributed equally, such that the items are "centered" and have equal empty space to their left and right? If so, you likely want to add .row { align-self:center }, and for that to work, .container { display:flex; flex-direction:column; }.

@mdo
Copy link
Member

mdo commented Feb 12, 2022

Back at laptop and have desktop screenshots, from Safari and Firefox.

Screen Shot 2022-02-12 at 10 26 25 AM

Screen Shot 2022-02-12 at 10 26 10 AM

Here's the breakdown on what works and what doesn't:

  1. First row works great, it's what we currently have in production.
  2. Your suggestion from the opening comment of using calc to subtract the gutter value doesn't work. Note how the red background doesn't line up with row 1.
  3. I added another example of calc using addition just because. This also doesn't work for obvious reasons, but if one were to use negative margin (which is part of the problem I think), translate, or position-ing to move it over, it might work? Super unclear, and this feels like a hack that would cause additional layout and/or performance issues.
  4. Lastly, using width: fit-content doesn't work here.

Without any other viable options, I'm likely going to close this issue as I feel I've spent too much time on it already knowing none of the suggestions will work. I'm also just realizing I never saw a specific reduced test case for this issue when building with Bootstrap. All that means is there's less information for me to act on and inform my research on potential fixes.

@wisniewskit
Copy link
Author

@mdo, then could you please help to me understand what purpose the negative margins are meant to serve? Is it just to force the rows back into the padding of the container? If so, wouldn't it be viable to reduce the padding on the container instead? (they both seem to use the same variable in your demo).

@mdo
Copy link
Member

mdo commented Feb 13, 2022

https://getbootstrap.com/docs/5.1/layout/grid/#how-it-works outlines that nicely already, and the grid examples show how content is meant to align thanks to the combination of:

  • horizontal padding on columns
  • negative margin on rows to counter the left padding of the first column and right padding of the last column
  • and, padding on a container to counter the negative margin of the row.

All that together gets you Bootstrap's grid system in a nutshell.


Closing this as a won't fix for now as I see no immediate path forward without significant complication or regression. Open to further testing and ideas on how to solve for this though, as well as clearer demonstrations (live, reduced test cases) of how Bootstrap may be causing this in some browsers.

❤️

@mdo mdo closed this as completed Feb 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants
@mdo @wisniewskit @karlcow @WinterSilence and others