Skip to main content

Search and Top Navigation

#10162 closed bug (fixed)

Opened July 11, 2014 08:31AM UTC

Closed October 30, 2014 01:21PM UTC

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.

Attachments (0)
Change History (16)

Changed July 11, 2014 12:31PM UTC by tj.vantoll comment:1

owner: → 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/.

Changed July 13, 2014 12:23PM UTC by sebfz1 comment:2

_comment0: Hi, \ \ The issue is that sub-menus 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 are bold: http://jsfiddle.net/sebfz1/rDT7T/ \ \ Thanks, \ Sebastien.1405254553971415

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.

Changed July 14, 2014 09:16AM UTC by tasso85 comment:3

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/

Changed July 14, 2014 12:55PM UTC by tj.vantoll comment:4

keywords: → regression
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.

Changed July 14, 2014 02:06PM UTC by tasso85 comment:5

Replying to [comment:4 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?

Changed July 14, 2014 02:51PM UTC by tj.vantoll comment:6

Replying to [comment:5 tasso85]:

Replying to [comment:4 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.

Changed July 23, 2014 04:02PM UTC by tj.vantoll comment:7

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

Changed July 23, 2014 05:09PM UTC by scottgonzalez comment:8

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.

Changed July 24, 2014 07:51AM UTC by tasso85 comment:9

Replying to [comment:8 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...

Changed July 24, 2014 12:29PM UTC by scottgonzalez comment:10

Replying to [comment:9 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.

Changed July 24, 2014 12:31PM UTC by tj.vantoll comment:11

Replying to [comment:9 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/.

Changed August 14, 2014 01:18PM UTC by sparkybg comment:12

Replying to [comment:11 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.

Changed August 29, 2014 12:30PM UTC by tj.vantoll comment:13

owner: tasso85tj.vantoll
status: openassigned

I'm working on a PR for this.

Changed September 30, 2014 09:53PM UTC by tj.vantoll comment:14

#10633 is a duplicate of this ticket.

Changed October 11, 2014 02:33AM UTC by tj.vantoll comment:15

Changed October 30, 2014 01:21PM UTC by TJ VanToll comment:16

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