Make WordPress Core

Opened 13 years ago

Closed 13 years ago

#17058 closed defect (bug) (fixed)

Wrong behavior when the dashboard menu is folded

Reported by: nuxwin's profile nuxwin Owned by:
Milestone: 3.2 Priority: normal
Severity: normal Version: 3.1
Component: Administration Keywords: has-patch
Focuses: Cc:

Description

When the dashboard menu is folded, the submenus cannot be displayed on hover event, and in other cases, cannot be hidden. The problems occur in the following cases:

I. An opened submenu cannot be hidden when the menu is folded

How to reproduce the bug:

  • 1. In unfolded state, toggle a submenu (open)
  • 2. Folding the menu by clicking on separator

Result:

The submenu appears now on the right, and cannot be hidden.

Expected result:

When the menu is folding, all submenu must be defaulted hidden

II. Hover event don't work when the menu is folded

How to reproduce the bug:

  • 1. In unfolded state, toggle a submenu (open)
  • 2. In unfoled state, toggle the submeu previously open (close)
  • 3. Fold the menu by clicking on separator

Result:

Now, the hover event for the menu (parent of the submenu that we have toggled) don't works and so, the submenu is never displayed.

Espected behavior:

On hover event, the submenu must be displayed.

III .Reason of the bug:

The reason of the bugs is the behavior of the the jQuery slideToggle() method used to toggle the submenus.

Note: I've a fix for that if you want.

Note: Sorry for my very poor English, I'm french.

Attachments (3)

patch.txt (393 bytes) - added by nuxwin 13 years ago.
patch to fix issue
#17058.patch (471 bytes) - added by nuxwin 13 years ago.
Updated patch
10646-3_17058_merge.patch (3.2 KB) - added by nuxwin 13 years ago.
Merge of patches (issue 10646 (3) and issue 17058)

Download all attachments as: .zip

Change History (20)

@nuxwin
13 years ago

patch to fix issue

#1 @nuxwin
13 years ago

  • Keywords has-patch reporter-feedback added

I've added a patch. The inline style added by the slideToggle() method must be removed at end of process to make sure that the hover/out events will works when the menu is folded.

#2 @nuxwin
13 years ago

  • Keywords reporter-feedback removed

#3 @scribu
13 years ago

Bug confirmed.

patch.txt is a raw diff. We prefer svn diffs.

See http://core.trac.wordpress.org/#HowtoSubmitPatches

#4 @scribu
13 years ago

Also, please name attachments name.diff or name.patch for coloring to kick in.

Last edited 13 years ago by scribu (previous) (diff)

@nuxwin
13 years ago

Updated patch

#5 @nuxwin
13 years ago

Hello ;

I've updated the patch according your rules. I'm sorry for the previous one. Please confirm that the fix works.

#6 follow-up: @nuxwin
13 years ago

Oh, I've not provided patch for the minified version. Don't forget it.

Thanks

#7 in reply to: ↑ 6 @duck_
13 years ago

Replying to nuxwin:

Oh, I've not provided patch for the minified version. Don't forget it.

That's fine. It's preferred that you only provide the patch for .dev.(js|css). Patching the minified version creates a very large patch which is almost impossible to review and so it will be redone by the committer anyway.

#8 @scribu
13 years ago

  • Milestone changed from Awaiting Review to 3.2
  • Severity changed from major to normal

Patch works.

#9 follow-up: @nacin
13 years ago

We can chain removeAttr() after toggleClass().

#10 in reply to: ↑ 9 @nuxwin
13 years ago

Replying to nacin:

We can chain removeAttr() after toggleClass().

Sorry but I thinks that you wrong. We can chain removeAttr() before toggleClass() but not after since we must operate on the submenu and not on the parent.

To resume:

var id = el.parent().toggleClass( 'wp-menu-open' ).attr('id');

becomes:

var id = el.removeAttr('style').parent().toggleClass( 'wp-menu-open' ).attr('id');

For the record, I've not done this in this way for a better understanding.

#11 @nacin
13 years ago

Sorry, yeah, meant before all of that.

#12 follow-up: @azaozz
13 years ago

Partially related #10646. @nuxwin could you see if https://core.trac.wordpress.org/attachment/ticket/10646/10646-3.patch also fixes this issue. It changes the CSS classes for open/closed menus a bit and removes some of the JS.

#13 in reply to: ↑ 12 @nuxwin
13 years ago

Replying to azaozz:

Partially related #10646. @nuxwin could you see if https://core.trac.wordpress.org/attachment/ticket/10646/10646-3.patch also fixes this issue. It changes the CSS classes for open/closed menus a bit and removes some of the JS.

Sure, I'll try by applying your patch (without my pacth) to see the behavior. however, I'm not sure that only applying your patch without my patch will solve this issue. I'll also try to apply both patch in same time to be sure that both work together.

#14 follow-up: @nuxwin
13 years ago

@azaozz

I. Your patch (10646-3.patch) without my patch.

Result: Your patch alone doesn't solve the #17058 issue.

Little question about the changes made by your patch. Now, when an admin page is loaded (at first time without any state data saved), the menu is defaulted folded. it's normal ? I say that because it 's not the case in current version.

Also, you can remove the following part:

	restoreMenuState : function() {
		// (perhaps) needed for back-compat
	},

Since it's not longer needed with your changes.

II. Both patches applyed:

All works fine.

I provides a merge of both patches (see 10646-3_17058_merge.patch).

@nuxwin
13 years ago

Merge of patches (issue 10646 (3) and issue 17058)

#15 in reply to: ↑ 14 ; follow-up: @azaozz
13 years ago

Replying to nuxwin:

...Now, when an admin page is loaded (at first time without any state data saved), the menu is defaulted folded. it's normal ? I say that because it 's not the case in current version.

The menu that has current submenu gets the classes wp-has-current-submenu wp-menu-open set from PHP. That overrides the saved state.

Also, you can remove the following part... Since it's not longer needed with your changes.

The comment there says it all :) It's unsafe to remove functions from WordPress, even JS functions. What if a plugin calls adminMenu.restoreMenuState() directly? It will throw a fatal error and probably break other JS.

#16 in reply to: ↑ 15 @nuxwin
13 years ago

Replying to azaozz:

Replying to nuxwin:

Also, you can remove the following part... Since it's not longer needed with your changes.

The comment there says it all :) It's unsafe to remove functions from WordPress, even JS functions. What if a plugin calls adminMenu.restoreMenuState() directly? It will throw a fatal error and probably break other JS.

Hmm, I've not thinking about this possibility. I understand now the purpose. Thanks for you explaination.

#17 @azaozz
13 years ago

  • Resolution set to fixed
  • Status changed from new to closed

(In [17623]) Don't refresh the admin menu after page load, apply the user-state from PHP, fix behaviour after folding the menu, props nacin, props nuxwin, fixes #10646, fixes #17058

Note: See TracTickets for help on using tickets.