Skip to content

Menu: Remove display block rule for list-items. #1235

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

Closed
wants to merge 1 commit into from

Conversation

tcrowley
Copy link
Contributor

Fixes #9995

@tjvantoll
Copy link
Member

@jzaefferer I verified this fixes the IE8 bug with @tcrowley. We were confused by your comment about disabled items though—neither of us could recreate that problem.

@jzaefferer
Copy link
Member

This fix looks good for the one issue, but the other still exists. Here's a screenshot:

screenshot

To reproduce, open the menu default demo, give the menu keyboard focus, then use the cursor keys to move the focus to the last (disabled) item. You should the bad display as in the screenshot above.

Hope that helps to reproduce the issue.

@tjvantoll
Copy link
Member

Thanks @jzaefferer, the screenshot helped a lot. I can recreate the disabled issue in IE8–9, but not IE10–11.

@tjvantoll
Copy link
Member

It's dependent on the font-size, but in IE8–9—for whatever reason—the floating <span>s that contain the icons are not cleared. Adding overflow: hidden to the .ui-menu-item selector fixes the problem and I can't think of any adverse effect: http://jsfiddle.net/tj_vantoll/Je35Q/. @tcrowley?

@tcrowley
Copy link
Contributor Author

tcrowley commented May 2, 2014

The problem is that the li's aren't clearing the floated right span which is exactly the purpose of clear.
http://jsfiddle.net/t_crowley/Je35Q/3/

Both solutions (and the current code actually) still wiggle, and they don't keep the icon centered. Switching the icon back to absolute positioning and vertically centering it with margin: auto fixes both problems.
http://jsfiddle.net/t_crowley/Je35Q/12/

Adding a min-height large enough to contain the icon could also be a solution but I like the bonus centering and not forcing a min-height on users.

@jzaefferer
Copy link
Member

Thanks @tjvantoll and @tcrowley. Could one of you add a commit to this PR with one of the suggested solutions?

@jzaefferer
Copy link
Member

@tcrowley @tjvantoll could you look into this again?

@tcrowley
Copy link
Contributor Author

Absolutely. Sorry about the lack of communication. Just got back from Disneyland.

@tcrowley
Copy link
Contributor Author

I added a commit (and rebased) that fixes both issues and vertically centers both left and right icons. Tested in all the supported browsers.

@jzaefferer
Copy link
Member

Great, thanks! Will be in 1.11.0-beta.2, hopefully this week.

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

Successfully merging this pull request may close these issues.

3 participants