Make WordPress Core

Opened 14 years ago

Closed 14 years ago

Last modified 14 years ago

#13615 closed defect (bug) (fixed)

Confusing behavior adding items to nonexistent menu

Reported by: filosofo's profile filosofo Owned by: nacin's profile nacin
Milestone: 3.0 Priority: normal
Severity: normal Version: 3.0
Component: Menus Keywords: has-patch
Focuses: Cc:

Description

If you try to add menu items on the create menu panel, nothing happens:it fails silently. Then if you realize your mistake and click another tab or submit the form from the name input box, you get the page unload AYS.

Patch:

  • Throws an error message when you try to add menu items to a nonexistent menu, both for js and non-js.
  • Removes unload AYS when there's no menu.

Attachments (2)

no-add-menu-items-nonexistent-menus.13615.diff (2.9 KB) - added by filosofo 14 years ago.
13615.fade-away-unusable-nav-menu-postboxes.diff (2.3 KB) - added by markjaquith 14 years ago.

Download all attachments as: .zip

Change History (15)

#1 @markjaquith
14 years ago

  • Cc jane added
  • Owner set to markjaquith
  • Status changed from new to accepted

Removes unload AYS when there's no menu.

Nice.

Throws an error message when you try to add menu items to a nonexistent menu, both for js and non-js.

We can do better. Instead of admonishing them for using the UI we presented to them, we can fade away the unusable portions and deactivate them if possible. This makes it clear what portion of the UI is meant to be used, and what part is not yet available, while still maintaining their proper spatial relationships.

Check out my patch.

Screenshot:

http://grab.by/4DLP

Jane, any UX objections?

#2 follow-ups: @filosofo
14 years ago

I think your reasoning about the UX is sound and agree that it's a better strategy.

As an aside, I'm really curious why you'd change the line in my patch from

if ( document.getElementById('menu-to-edit') )

to

if ( 0 != $('#menu-to-edit').length )

Would you please explain?

#3 in reply to: ↑ 2 @nacin
14 years ago

Replying to filosofo:

Would you please explain?

Oh no, not another jQuery argument :-)

Re the opacity, Jane has said she approves this approach. I think it deserves a slight non-JS modification, in that the submit buttons are additionally disabled, otherwise clicking those would cause a page refresh and that's odd.

#4 in reply to: ↑ 2 @markjaquith
14 years ago

Replying to filosofo:

As an aside, I'm really curious why you'd change the line in my patch from

if ( document.getElementById('menu-to-edit') )

to

if ( 0 != $('#menu-to-edit').length )

Would you please explain?

It's shorter, and because jQuery is WP's JS library and I like to use it wherever possible. It tends to reduce the incidence of browser-specific JS bugs.

I think it deserves a slight non-JS modification, in that the submit buttons are additionally disabled, otherwise clicking those would cause a page refresh and that's odd.

That would be a lot of code... wouldn't you have to add it to each of the buttons? I'm going to commit what I have, since what you propose could be added on.

#5 @markjaquith
14 years ago

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

(In [15062]) Do not load window.onbeforeonload event if we are not on an existing nav menu. props filosofo. Reduce opacity on nav menu postboxes if we are not on an existing nav menu, and for JS users, deactivate all links and form elements, to make it obvious that those UI elements are not usable at this stage. fixes #13615

#6 @ocean90
14 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

We still need the non-JS version. Also, we have no opacity in IE.

#7 @nacin
14 years ago

  • Owner changed from markjaquith to nacin
  • Status changed from reopened to accepted

Going to run with this.

ocean90 has pointed out that the tabs in the meta boxes also would need to get their href's stripped. I think that's excessive.

#8 @markjaquith
14 years ago

Also, we have no opacity in IE.

Third rate browser, third rate results. It's a travesty that Microsoft's "latest and greatest" browser doesn't support CSS 2.1. The opacity change is a nice visual cue, but it isn't functional, so IE users can deal, or upgrade to a real browser.

ocean90 has pointed out that the tabs in the meta boxes also would need to get their href's stripped. I think that's excessive.

It's "nice to have," which is why I did it for the JS version. It makes it more obvious that it's a read-only area. Non-JS doesn't have to be as polished as JS. We want it to work. It doesn't have to work as elegantly. So don't go overboard.

#9 @nacin
14 years ago

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

(In [15063]) Full no-JS and IE support for [15062]. fixes #13615.

#10 follow-up: @filosofo
14 years ago

Replying to markjaquith:

It's shorter, and because jQuery is WP's JS library and I like to use it wherever possible. It tends to reduce the incidence of browser-specific JS bugs.

OK. The reason I ask is that retrieving an element by its id using document.getElementById is one of the most widely-supported actions across browsers (many more than jQuery itself claims to support), and it's almost universally very fast.

I've tested it across a number of browsers, and at best (typically in the slowest browsers), jQuery is only twice as slow as the host method in retrieving an element by ID; in the fastest modern browsers it seems to be at least several times slower.

I guess I'm overdue for my promised blog post about the more general issue, but this particular line seems like a no-brainer: at the cost of a few characters you get wider support and better performance, guaranteed.

#11 @filosofo
14 years ago

Also, you can win back the characters and further improve performance if you, e.g., set d = document and then consistently use d.getElementById: less typing for you and less time going up the scope chain for document.

#12 @nacin
14 years ago

(In [15066]) Small IE fix. props ocean90. Also, remove outdated style. see #13615.

#13 in reply to: ↑ 10 @markjaquith
14 years ago

Replying to filosofo:

I've tested it across a number of browsers, and at best (typically in the slowest browsers), jQuery is only twice as slow as the host method in retrieving an element by ID; in the fastest modern browsers it seems to be at least several times slower.

I guess I'm overdue for my promised blog post about the more general issue, but this particular line seems like a no-brainer: at the cost of a few characters you get wider support and better performance, guaranteed.

I must have missed that fracas. The difference for this one selector can be measured in single digits of nanoseconds. So this one isn't going to make or break anything. I gather it's more of a general trend you're concerned about. If we want to make sure we're using the fastest way possible of doing simple DOM grabs or checks, I noticed that this is actually faster than using document.getElementById() in latest stable Firefox:

g = function(a){return document.getElementById(a);};

For me, it consistently saved about 0.2 nanoseconds per iteration. Averaged 2.9 ns vs 3.1 ns for the POJ version. Unexpected, but then the new JS engines do some fairly insane stuff to eke out speed in function calls.

And it doesn't get much shorter than g() :-)

If there is a separate ticket for speeding up our JS, we can continue this line of investigation there.

Note: See TracTickets for help on using tickets.