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

[Menu Item] Not rendered properly with RightIcon on FF 45.0.1 #3845

Closed
vspiewak opened this issue Mar 30, 2016 · 10 comments
Closed

[Menu Item] Not rendered properly with RightIcon on FF 45.0.1 #3845

vspiewak opened this issue Mar 30, 2016 · 10 comments
Labels
component: menu This is the name of the generic UI component, not the React module!

Comments

@vspiewak
Copy link

Problem Description

MenuItem is not rendered properly with RightToggle on FF 45.0.1, see screenshots taken from your official documentation :

capture d ecran 2016-03-30 a 10 22 29
capture d ecran 2016-03-30 a 10 22 16

Versions

v0.15.0-alpha.2

  • Material-UI: v0.15.0-alpha.2
  • Browser: 45.0.1

Works fine on Chrome

@mbrookes mbrookes added this to the 0.15.0-beta.1 Release milestone Mar 30, 2016
@mbrookes mbrookes changed the title [Menu Item] Not rendered properly with RightToggle on FF 45.0.1 [Menu Item] Not rendered properly with RightIcon on FF 45.0.1 Mar 30, 2016
@mbrookes
Copy link
Member

Confirmed in HEAD @3d9007b

@nathanmarks
Copy link
Member

@mbrookes This is a bug in firefox:

https://bugzilla.mozilla.org/show_bug.cgi?id=874718

https://drafts.csswg.org/css-flexbox-1/#abspos-items

An absolutely-positioned child of a flex container does not participate in flex layout.

@nathanmarks nathanmarks added bug 🐛 Something doesn't work out of scope The problem looks valid but we won't fix it (maybe we will revisit it in the future) external dependency Blocked by external dependency, we can’t do anything about it Cross Browser Support labels Apr 8, 2016
@nathanmarks nathanmarks reopened this Apr 8, 2016
@nathanmarks nathanmarks removed the out of scope The problem looks valid but we won't fix it (maybe we will revisit it in the future) label Apr 8, 2016
@nathanmarks
Copy link
Member

@mbrookes

Should we fix it anyways by changing our approach?

@nathanmarks nathanmarks removed the external dependency Blocked by external dependency, we can’t do anything about it label Apr 8, 2016
@nathanmarks
Copy link
Member

@mbrookes

I think this PR caused the regression: #3597

@mbrookes
Copy link
Member

mbrookes commented Apr 8, 2016

Nice 🔍 on the FF bug and cause. 👍

I'm all for moving to modern web standards (even though we've broken a few other things in older but not ancient browsers as a result), but since this breaks layout in a current browser version, we'll have to rethink this one.

@tintin1343 - care to take a look?

@mbrookes mbrookes removed the bug 🐛 Something doesn't work label Apr 8, 2016
@nathanmarks
Copy link
Member

@mbrookes @tintin1343

I dug into fixing it while keeping flex, but the main issue is that we're combining a flex parent with absolutely positiong children and depending on that for layout -- which as we can see is not working as intended in firefox.

As long as MenuItem and ListItem are used like this, they should both use either flex, or abspos/floats but not both until this issue is fixed in firefox.

Otherwise you end up having to patch a ton of ListItem styles from MenuItem

@tintin1343
Copy link
Contributor

@mbrookes @nathanmarks : Let me take shot here. I had fixed this for chrome when I was working on the PR.

If its too much work , will need to fix it some other way.

@nathanmarks
Copy link
Member

@tintin1343 IMO you are best of rolling back your previous changes and fixing the other issue without converting MenuItem to flexbox. It won't play nice in FF with the abspos children.

@tintin1343
Copy link
Contributor

@nathanmarks : You are right. I just made some changes and checked it. The "absolute" positioning is the culprit and also ListItem is completely messed up component.

@mbrookes @nathanmarks : I will revert back my previous changes.

@nathanmarks
Copy link
Member

@tintin1343 thanks, can you please submit a PR first reverting the previous changes? And then approach the problem from the start line again 👍

mbrookes added a commit that referenced this issue Apr 10, 2016
[MenuItem] Revert flex props from #3597, fixes #3845, reopens #3531
und3fined pushed a commit to und3fined/material-ui that referenced this issue Apr 20, 2016
@oliviertassinari oliviertassinari added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Dec 21, 2022
@zannager zannager added component: menu This is the name of the generic UI component, not the React module! and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: menu This is the name of the generic UI component, not the React module!
Projects
None yet
Development

No branches or pull requests

7 participants