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 appears at incorrect position #143

Closed
OvermindDL1 opened this issue Aug 4, 2016 · 10 comments
Closed

Menu appears at incorrect position #143

OvermindDL1 opened this issue Aug 4, 2016 · 10 comments
Labels

Comments

@OvermindDL1
Copy link
Contributor

As of elm-mdl version 7.0.0 the menus are not opening at the button that opened them, see image:
bug demonstration

This is in the latest released Chrome that was updated yesterday.

@debois debois changed the title Menu not placed in correct area Menu appears at incorrect position Aug 9, 2016
@aforemny
Copy link
Collaborator

I cannot reproduce this behavior on the demo page using Chrome 52.0.2743.116. If the problem still persists, please let me know what version of Chrome you are using and also provide a self-contained minimal example that I can test with.

@OvermindDL1
Copy link
Contributor Author

OvermindDL1 commented Aug 10, 2016

It works fine on the demo, but this happens in this thing on Chrome, Firefox, IE11, and Edge. I've tried to replicate it but simple demo's work fine. I've tried to mutate the DOM and CSS on-site in chrome's inspector to see if I can get it to work properly but I've not been able to yet (almost like the geometry is stored at load or something)...

Some examples, the menu appears (just like above, it is still doing the same thing) in the lower-right corner of some parent element (offset in by 32px up and 0px left). If I remove the element-styles for bottom and right on the popup div then it actually appears in the correct place, it gets auto-calculated values of 122px for bottom and 625.141px for right, so 'something' is telling it to be in the correct location but the element styles is overriding it with odd values like bottom:32px and right:0px. I look through chrome's css tree inspector to see everything that is assigning CSS and nothing else is explicitely setting bottom or right on it (although something is setting 'left:0pxandtop:100%`, that is some bootstrap themeing that I cannot control because it is managed by the site administrator, killing those via chrome does not change the menu position either, killing every-single css in the parent admin's css file does nothing to change the position or look).

Keeping the element-assigned-styles for bottom/right makes it appear in the lower-right corner of the ... hold on, I fixed! It appears to be a bug in Material.Menu here in elm-mdl! Whoo!

The parent div that holds the menu button and the menu div is currently unstyled (and unnamed, and un-everything, it is a plain div). It needs to have a style added it to of position: relative;. Apparently this is documented in mdl itself that the parent div of a menu button and menu div must-not be position: static and in most/all cases should probably be position: relative.

Sooo, found the bug, and a fix! On https://github.com/debois/elm-mdl/blob/v7/src/Material/Menu.elm#L544 it looks like it needs "position: relative" added as a style (and of course checked that it does not break anything anywhere, but from what I read it looks like that is what it should always be). :-)

We have no way of specifying a style on that div since it is not exposed to us like we do for Tooltips (why do menu and tooltips follow two different patterns when they are both 'something' and a 'div' that follow immediately after), but adding it in elm-mdl itself still seems like the right thing to do from what I'm reading.

Reference: google/material-design-lite#948

@OvermindDL1
Copy link
Contributor Author

For note, they also thought about using fixed positioning like they do with tooltips. Here is where they (shortly) talked about fixing it: google/material-design-lite#952

@OvermindDL1
Copy link
Contributor Author

And it is documented at: https://github.com/google/material-design-lite/blob/mdl-1.x/src/menu/README.md

Note: The menu requires a non-static positioned parent element. Positioning options may not work properly if the menu is inside of a statically positioned node.

@debois
Copy link
Owner

debois commented Aug 12, 2016

Just to be sure I understand this: If the top-level Menu div gets position: relative;, this bug goes away?

I ask because we don't currently style it, so it should get that positioning by default?

@debois debois added the bug label Aug 12, 2016
@OvermindDL1
Copy link
Contributor Author

Just to be sure I understand this: If the top-level Menu div gets position: relative;, this bug goes away?

I ask because we don't currently style it, so it should get that positioning by default?

Precisely, as per the linked document it needs to be postioned to something other than static, and the official devs in the listed bug report on mdl state that position: relative should be used in almost all cases.

At the very least that unstyled outer div needs position: relative at the very least, though giving a way to custom style it might be useful as well but is not needed.

But yes, if the top-level Menu div gets position: relative; then the bug goes away, same as in the mdl issue report itself. :-)

@aforemny
Copy link
Collaborator

@OvermindDL1 Great catch!

However, wouldn't it suffice to set relative position on some parent? I am thinking about separating the menu from the button, so there would be no containing div anymore.

@debois debois closed this as completed in 58ac249 Aug 12, 2016
@OvermindDL1
Copy link
Contributor Author

@aforemny I tried that but it did not work, it seems like it has to be the immediate parent. But separating them out and following the Tooltip style will definitely fix it as I can then style the parent div myself. :-)

Definitely add to the docs that the parent element/container needs to not be position: static;, which is default a lot of times, and that position: relative; should probably be the go-to value in almost all cases. :-)

@debois
Copy link
Owner

debois commented Aug 12, 2016

I made a temporary fix while we're waiting for a larger refactoring of menu. It'll be out shortly.

@debois
Copy link
Owner

debois commented Aug 12, 2016

@OvermindDL1: Thanks for the comprehensive bug-chase!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants