Opened 3 years ago

Closed 3 years ago

#10162 closed bug (fixed)

Menu: ui-state-focus styling applies to submenus

Reported by: tasso85 Owned by: tj.vantoll
Priority: blocker Milestone: 1.12.0
Component: ui.menu Version: 1.11.0
Keywords: regression Cc:
Blocked by: Blocking:

Description

From version 1.10.4 to version 1.11.0 there was a change in how the menu widget handled the focus on one of its element.

In version 1.10.4, the .ui-state-focus class was applied directly to the hovered <a/> element in the menu.

In version 1.11.0 it is applied to its parent <li/> instead.

This has a problem though: if the hovered element in the menu has sub-elements, the focus class is applied to them too!

This fiddle shows the problem: http://jsfiddle.net/ucF9M/1/

Here, open the hidden menu, and then hover "Inserimento multiplo", you'll see that the submenu has white text, inherited from its parent being hovered.

This fiddle instead shows the hold behaviour: http://jsfiddle.net/ucF9M/2/

If you do the same on this other fiddle, you'll see that the submenu can be viewed correctly.

Change History (16)

comment:1 Changed 3 years ago by tj.vantoll

Owner: set to tasso85
Status: newpending

Hi tasso85,

Thanks for taking the time to contribute to jQuery UI. Unfortunately it's very difficult to see what the issue is in your test cases because there's a whole lot of code involved. Can you reduce the test case to use the least amount of code necessary to show the bug? I'm not seeing issues in my trivial test case: http://jsfiddle.net/3fFTu/.

comment:2 Changed 3 years ago by Sebastien Briquet

Hi,

The issue is that sub-menus items inherit from parent's ui-state-active style. So depending of the theme, this can lead to unexpected display. For instance in the following fiddle, sub-menus items are bold: http://jsfiddle.net/sebfz1/rDT7T/

Thanks, Sebastien.

Last edited 3 years ago by Sebastien Briquet (previous) (diff)

comment:3 Changed 3 years ago by tasso85

Status: pendingnew

@tj.vantoll: the issue is exactly the one explained by @sebfz1, that is, sub-menus items seems to inherit from parent's ui-state-active/ui-state-focus style (at least when they contain <a/> tags).

I described my issue in the original post itself:

you will notice that the submenu has white text on a white background.

If you do the same on my other fiddle, which uses previous jQuery UI version, the submenu is rendered correctly.

See also your own, slighly modified, fiddle: http://jsfiddle.net/3fFTu/1/

comment:4 Changed 3 years ago by tj.vantoll

Keywords: regression added
Status: newopen
Summary: Menu focus behaviour changedMenu: ui-state-focus styling applies to submenus

Thanks, that helped a lot. I get the problem now, and see that it wasn't a problem in 1.10.4. This could be a tricky one to solve.

comment:5 in reply to:  4 ; Changed 3 years ago by tasso85

Replying to tj.vantoll:

Thanks, that helped a lot. I get the problem now, and see that it wasn't a problem in 1.10.4. This could be a tricky one to solve.

This may sound a little naive, since I've never tinkered with jQuery UI source code I do not know it's internals, but, in version 1.10.4, the focus class gets applied only to the <a/> tags inside the <li/> element, while now it is applied to the whole <li/> element, can't you just make it behave as it did before?

comment:6 in reply to:  5 Changed 3 years ago by tj.vantoll

Replying to tasso85:

Replying to tj.vantoll:

Thanks, that helped a lot. I get the problem now, and see that it wasn't a problem in 1.10.4. This could be a tricky one to solve.

This may sound a little naive, since I've never tinkered with jQuery UI source code I do not know it's internals, but, in version 1.10.4, the focus class gets applied only to the <a/> tags inside the <li/> element, while now it is applied to the whole <li/> element, can't you just make it behave as it did before?

Well, it defeats the purpose of removing the anchor tags to add them back, but I think we need *some* element to workaround this problem. Our best bet is probably to wrap each menu item with a span internally and apply the class name to that span.

comment:7 Changed 3 years ago by tj.vantoll

This is the smallest test case I can come up with: http://jsfiddle.net/tj_vantoll/3fFTu/2/. Now to work on fixing this.

comment:8 Changed 3 years ago by Scott González

Milestone: none1.12.0
Priority: minorblocker

In order to fix this, we need to add wrappers again. This time we won't require anchors, but we'll likely need to add a class to the wrapper in order to properly apply the styling and handle the upcoming classes option.

Unfortunately, since requiring the wrappers again is another breaking change, we'll need to wait until 1.12.0 to fix this.

comment:9 in reply to:  8 ; Changed 3 years ago by tasso85

Replying to scott.gonzalez:

In order to fix this, we need to add wrappers again. This time we won't require anchors, but we'll likely need to add a class to the wrapper in order to properly apply the styling and handle the upcoming classes option.

Unfortunately, since requiring the wrappers again is another breaking change, we'll need to wait until 1.12.0 to fix this.

Is there any chance to obtain some kind of patch/hotfix for this?

For example, I could create a new plugin which inherits from the menu plugin and hooks in the create method to put the wrappers dinamically, or use <a/> tags if present, but I fear my knowledge of jQuery UI internal is not good enough to make this patch by myself...

comment:10 in reply to:  9 Changed 3 years ago by Scott González

Replying to tasso85:

Is there any chance to obtain some kind of patch/hotfix for this?

Everything we implement is public, so once it's implemented, you can just grab the copy from master. However, with that being said, this won't land in master until we're getting ready for 1.12.0. I'd like to make that happen a lot sooner than we planned because of this.

comment:11 in reply to:  9 ; Changed 3 years ago by tj.vantoll

Replying to tasso85:

Is there any chance to obtain some kind of patch/hotfix for this?

You can also workaround this with a little CSS that undoes any rules you don't want applied to submenus. For example http://jsfiddle.net/tj_vantoll/3fFTu/3/.

comment:12 in reply to:  11 Changed 3 years ago by sparkybg

Replying to tj.vantoll:

You can also workaround this with a little CSS that undoes any rules you don't want applied to submenus. For example http://jsfiddle.net/tj_vantoll/3fFTu/3/.

Unfortunately, you cannot. It inherits some things other than just font weight. And if you correct them all with custom css, the moment you change the theme, you must again modify your CSS for the new theme.

I suspect the wrappers were removed because of mobile version. But anyway, I don't see other solution but wrappers. A widget option for this would satisfy both wrapper heaters and lovers I think. Or, if there are submenus, a wrappers only for submenus can be made. Submenus are unusable this way.

...and again - I don't know why the "smoothness" theme was chosen for the demo sites. It is the worst theme to see a bugs so obvious in some other themes.

comment:13 Changed 3 years ago by tj.vantoll

Owner: changed from tasso85 to tj.vantoll
Status: openassigned

I'm working on a PR for this.

comment:14 Changed 3 years ago by tj.vantoll

#10633 is a duplicate of this ticket.

comment:16 Changed 3 years ago by TJ VanToll

Resolution: fixed
Status: assignedclosed

Menu: Wrap menu items in a <div>

This avoids styling issues where ui-state-focus rules apply to submenus.

Fixes #10162 Closes gh-1342

Changeset: de2ef2a585447cf75fc8d4a734c12aa2bbd028c2

Note: See TracTickets for help on using tickets.