Menu: Wrap menu items in a <div> #1342
Conversation
07496a3 to
cd9f65e
Compare
This avoids styling issues where ui-state-focus rules apply to submenus. Fixes #10162 Closes jquerygh-1342
cd9f65e to
01f28be
Compare
This avoids styling issues where ui-state-focus rules apply to submenus. Fixes #10162 Closes jquerygh-1342
|
This is ready for review now. cc @scottgonzalez @jzaefferer @arschmitz @kborchers and anyone else interested. May @kborchers one day forgive me for this transgression. |
| tabIndex: -1, | ||
| role: this._itemRole() | ||
| }); | ||
| .find( "> div" ) |
There was a problem hiding this comment.
Is there a reason to use .find with a child selector here instead of using .children, like everywhere else in this PR?
|
Looking at the visual tests for Menu, I noticed our old friend the 1px glitch. This one happens when looking at the "Menu with icons" test. Move the mouse to open the last submenu, then hover the first submenu item. Look at the third second submenu. Move the mouse to the item below the foucsed one. Note the second submenu jumping one pixel down, before the other submenu is displayed. Also happens the other way round, where the lower submenu glitches before it disappears. As usual, its just one 1px, but rather annoying anyway. Also happens for the "Menu with custom markup" test. On the "Menu with custom markup, multi-line items and a custom submenu icon" test, the submenu has bad styling: The focus state doesn't fill out the container. Code itself looks fine, apart from the one tiny issue I noted above. And the big glaring issue of sprinkling divs everywhere. |
|
@jzaefferer I fixed the demo issue and made the code change you suggested. And I'll try to refigure out our favorite glitch issue this weekend. |
| tabIndex: -1, | ||
| role: this._itemRole() | ||
| }); | ||
| .children( "div" ) |
There was a problem hiding this comment.
I don't think there should be a hard-coded element here. Can't this just be .children()?
There was a problem hiding this comment.
I just did some testing around this and it works fine. Good catch.
|
I went through and incorporated @scottgonzalez's changes/feedback. At least now you can build The 1px dance should be the only action item left, but it continues to mystify me. I'm going to enlist @tcrowley's help. |
|
I've resolved most of the dancing issues, but one related to submenus still remains. You can see it on this fiddle: http://jsfiddle.net/tj_vantoll/8ym9u177/. I'm going to dig back into that tomorrow, but if anyone has an idea what might be going on let me know. |
f78b6f6 to
e51252e
Compare
This avoids styling issues where ui-state-focus rules apply to submenus. Fixes #10162 Closes jquerygh-1342
e51252e to
5c96672
Compare
This avoids styling issues where ui-state-focus rules apply to submenus. Fixes #10162 Closes jquerygh-1342
|
I finally resolved the last outstanding issue here and rebased this branch. If anyone else wants to take a look feel free, otherwise this is ready as soon as we're putting 1.12 stuff in master. |
| @@ -24,7 +24,9 @@ TestHelpers.menu = { | |||
|
|
|||
| click: function( menu, item ) { | |||
| lastItem = item; | |||
| menu.children( ":eq(" + item + ")" ).trigger( "click" ); | |||
| menu.children( ":eq(" + item + ")" ) | |||
| .find( ".ui-menu-item-wrapper" ) | |||
There was a problem hiding this comment.
Shouldn't this be .children() so that it handles items with submenus properly?
There was a problem hiding this comment.
Only if you click a menu item that contains a submenu, which none of the existing tests that use this method do. But I'll change this for future proofing.
1df9197 to
b388155
Compare
This avoids styling issues where ui-state-focus rules apply to submenus. Fixes #10162 Closes jquerygh-1342
This avoids styling issues where ui-state-focus rules apply to submenus. Fixes #10162 Closes jquerygh-1342
b388155 to
e9643f6
Compare
|
Changes made based on @scottgonzalez's comments and I rebased this against master again. |
This avoids styling issues where ui-state-focus rules apply to submenus. Fixes #10162 Closes gh-1342
Fixes #10162