Menubar #829

sgharms · 2012-11-19T05:19:04Z

Implement unit test
Move away from bind to prefer _on

Ensure open sub-menus close on sub-menu-less `click` or `mouseenter` events. See 9f53e0.

@scottgonzalez

mikesherov · 2012-11-19T13:28:33Z

ui/jquery.ui.menubar.js

-				});
+        that._on( input, {
+          click:  function( event ) {
+            if ( that.open ){ that._close(); }


This wasn't being checked for that.open previously, but you're checking now. Is that the correct behavior?

@mikesherov It seems to work the same (mouseenter is fired before the click) but I can see the wisdom in making the fewest required changes. Will change.

jzaefferer · 2012-11-19T23:06:17Z

There are three more uses of bind which should also get replaced. Also, when you look at the file (not the diff), you can see the whitespace being messed up.

sgharms · 2012-11-20T00:30:33Z

@jzaefferer I've got those bind calls sorted out in another commit. I'll handle them globally when I use @scottgonzalez comments

jzaefferer · 2012-11-22T09:48:39Z

Okay, so you're still working on this, right? When you push an update, please make sure that you fix your whitespace settings first. It affects all files.

sgharms · 2012-11-28T17:04:39Z

I went to Hawaii for Thanksgiving. It was beautiful. There were palm
trees, sandy beaches, beautiful reefs as far as the eye could see.

There was not any menubar work done. I'll pick it back up this week.

Steven

On Thu, Nov 22, 2012 at 01:48:42AM -0800, Jörn Zaefferer wrote:

Okay, so you're still working on this, right? When you push an update, please make sure that you fix your whitespace settings first. It affects all files.

Reply to this email directly or view it on GitHub:
#829 (comment)

sgharms · 2012-12-01T01:02:19Z

@jzaefferer I have removed the other bind calls thanks to @scottgonzalez for help earlier today. Also, this passed the grunt lint task. My understanding of the jQ UI is imperfect and my JS-fu fairly naïve, so if I've erred let me know.

mikesherov · 2012-12-01T13:46:57Z

ui/jquery.ui.menubar.js

-			});
+			}
+		});
+		if ( this.items.length > 0 ) {


Why is this conditional empty?

@mikesherov Good catch. It's vestigial. Will rm.

sgharms · 2012-12-02T05:36:06Z

ui/jquery.ui.menubar.js

@@ -31,7 +31,7 @@ $.widget( "ui.menubar", {
 		}
 	},
 	_create: function() {
-		var that = this;
+		var that = this, subMenus;


@scottgonzalez @jzaefferer When I look at this _create() method, it seems there's a lot of initializing sub-tasks going on.

We're initializing menuItems

We're instantiating a $.ui.menus underneath all of these menuItems

We're setting behavior on the menuItems and on the sub-menu items

We're defining callbacks that are applied on the sub-menu items....

etc.

Combine with the jQuery 'dot-chaining' syntax, the method seems to strain easy readability (in my book). This makes the contribution bar higher (IMNSHO) and makes testing / debugging harder. Maybe this isn't so for jQuery gurus such as yourselves, but I think the code would be more approachable if the long methods used intention-revealing sub-methods. I don't see this done elsewhere in the jQuery UI plugin set, but I wanted to get your thoughts on why this is or if this is just my relative unfamiliarity with plugin design.

Questions:

Is it against jQuery standard to split these _create()-homed activities into intention-revealing sub-methods?

Doing so might mean that you have a single var declaration in the sub-method and thus chaining becomes less attractive (given the "only one var" statement) style rule

It's fine to split up _create() if the split makes sense. I don't see how the one-var rule has any impact on chaining method calls.

I'll do that in another request, then.

Uninitialized vars at the top, on their own line:

var subMenus, that = this;

sgharms · 2012-12-03T19:33:28Z

@scottgonzalez Will amend momentarily.

sgharms · 2012-12-03T21:02:11Z

@scottgonzalez Updated to remove namespacing of the event.

The `_create()` method had quite a lot going on in it that made it hard to refactor, analyze, or understand. Use sub-functions to encapsulate certain activities.

sgharms · 2012-12-04T17:15:59Z

@jzaefferer, @scottgonzalez Can we merge this?

scottgonzalez · 2012-12-06T16:49:14Z

ui/jquery.ui.menubar.js

-			.bind( "keydown.menubar", function( event ) {
+			});
+		this._on( subMenus, {
+


no blank line here

@jzaefferer

…s item Per @jzaefferer: > Something I didn't write down on the wiki page, > but seems like it should get adressed: When > clicking to open a submenu, hovering the > non-menu-item closes the menu, and another hover > on the closed menu should probably reopen it, > instead of requiring another click. When it gets > closed by a click, it should require the click to > reopen. If that makes sense to you to, we can put > it on the wiki.

sgharms · 2013-01-18T16:54:12Z

@jzaefferer : Addressed point 2 in bb8e2e0

I should have finished my coffee before I committed this.

sgharms · 2013-01-18T18:01:30Z

@jzaefferer I think that takes care of your concerns from your last update. I'll await your review.

jzaefferer · 2013-01-19T18:02:00Z

Yep, much better. Another minimal round of testing, I hope to have more time for this soonish:

The "About" button/link on the second menubar in the demo shouldn't have the dropdown icon, since there is no menu.

Otherwise, to keep you busy a bit further, can you take a look at the TODOs at the top here? http://wiki.jqueryui.com/w/page/38666403/Menubar

sgharms · 2013-01-19T18:43:15Z

@jzaefferer Thanks for the wiki URL, I didn't have that anywhere.

sgharms · 2013-01-20T04:58:52Z

@jzaefferer aafdc17 should take care of the issue mentioned in the demo. Simple change 🍰 Strangely I didn't know that the demo directory existed. I'll see what I can knock out from the wiki.

jzaefferer · 2013-02-01T19:03:41Z

@sgharms along with the issues described on the wiki, here's a few more, all with the default demo:

Tab to the first menubar, then cursor right, down, left.

Expected: Same as pressing escaping, closing the menu and moving focus back to the bar
Actual: Menu closes, but there's no focus indicator. Cursor left/right still opens menus, but can't move focus to the non-menu item.

Tab to the first menubar, then cursor right twice, then shift-tab.

Expected: Focus moves to previous tabbable.
Actual: Focus moves from "Edit" to "File". Looks like bad tab-index handling.

Tab to the second menubar, press cursor left/right.

Expected: Moving focus to other items in the second menubar.
Actual: Nothing, can't move with cursor keys. Works fine on the other two, so likely related to the menuIcon option.

There's more, but I suspect they are related to those above and should go away when those are fixed.

jzaefferer · 2013-03-08T17:27:26Z

@sgharms would be great if you could take another look at the issues from my previous comment. And afterwards we should either just land this PR (yay) or create a new one (okay), since this one has gotten pretty long.

sgharms · 2013-03-14T04:48:06Z

Looking at last update....

sgharms · 2013-03-15T01:00:39Z

@jzaefferer As I read your bullet #2, I'm confused between what it says and what is said about tab/shift-tab in the wiki:

Wiki says:

Tab/Shift-Tab: Moves focus to the next or previous focusable element. Must be outside the menubar. To move focus within the menubar, use the other keys as described.

That seems to imply to me that it should go to something else on the page entirely. In the case of the demo page, it should loop around to the third menubar demo at the bottom. Tab should be able to move between menubars and other inputs but should not be used to move between ui-buttons. Is that right?

Assuming your "expected" scenario is the proper behavior to tab to the last menubar of the demo page?

jzaefferer · 2013-03-15T16:51:24Z

Tab/Shift-Tab: Moves focus to the next or previous focusable element. Must be outside the menubar. To move focus within the menubar, use the other keys as described.

That's correct, but I don't see that contradicting what I wrote above.

it should loop around to the third menubar demo at the bottom.

Not entirely sure if that's correct, but it certainly shouldn't move focus within the menubar, as it does right now.

Tab should be able to move between menubars and other inputs, but should not be used to move between ui-buttons

Correct.

When this is removed it makes the menu item with a submenu eligible for tabbing / shift-tabbing to / from.

sgharms · 2013-03-16T18:53:14Z

@jzaefferer I think this takes care of those three bullets you mentioned.

jzaefferer · 2013-03-18T12:05:51Z

Okay, thanks for the update. Though it doesn't look like we're done here. Would be great if you could do more manual keyboard testing as well, as these seem pretty obvious:

Tab to first menubar, press cursor right, right, down, left. Should now open the File submenu, instead just closes the Edit submenu. Now press Shift-Tab. Should move focus to something else, instead moves focus to About button inside the menubar.

Tab to second menubar. Use cursor left/right. Note that focus doesn't wrap around like it does on the other two menus.

Something new I noticed: On touchscreens without hover events, the second menubar is broken. The click event should still open submenus when there's no hover event. I'm fine with ignoring that for this PR and addressing it later though.

sgharms · 2013-03-18T21:01:05Z

Willfix.

On Mon, Mar 18, 2013 at 5:06 AM, Jörn Zaefferer [email protected]:

Okay, thanks for the update. Though it doesn't look like we're done here.
Would be great if you could do more manual keyboard testing as well, as
these seem pretty obvious:

Tab to first menubar, press cursor right, right, down, left. Should now
open the File submenu, instead just closes the Edit submenu. Now press
Shift-Tab. Should move focus to something else, instead moves focus to
About button inside the menubar.

Tab to second menubar. Use cursor left/right. Note that focus doesn't wrap
around like it does on the other two menus.

Something new I noticed: On touchscreens without hover events, the second
menubar is broken. The click event should still open submenus when there's
no hover event. I'm fine with ignoring that for this PR and addressing it
later though.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/829#issuecomment-15051223
.

sgharms · 2013-03-22T15:46:35Z

@jzaefferer Fixed the egregious problems from the previous commitset.

I still have some funniness around TAB behavior but the menus should be visitable, wrap properly, extend properly, and save extended state properly.

jzaefferer · 2013-03-23T10:14:34Z

That looks much better. There's still the issue where shift-tab moves focus to the About item instead of outside the menubar ("funniness around TAB behaviour"?). Should we address that in a new PR?

jzaefferer · 2013-03-24T17:35:15Z

I've landed this, squashed into one commit, in 86417ef and merged master in 2253df8 (updating the menubar files to use jQuery 1.9.1). I've made a note of the open issues mentioned here on the wiki page: http://wiki.jqueryui.com/w/page/38666403/Menubar

Steven G. Harms added 2 commits November 18, 2012 19:25

Adds unit test: correct sub-menu-less menu item event behavior

0f33062

Ensure open sub-menus close on sub-menu-less `click` or `mouseenter` events. See 9f53e0.

Convert from bind() to _on()

82d0056

Per comment from @scottgonzalez

mikesherov reviewed Nov 19, 2012
View reviewed changes

menubar: remove remaining bind() calls; code standards compliance.

15bd67f

mikesherov reviewed Dec 1, 2012
View reviewed changes

menubar: rm stray, empty conditional

d4ca89b

sgharms reviewed Dec 2, 2012
View reviewed changes

menubar: rm namespacing on events in _on

f30c853

menubar: decompose _create

ec67b65

The `_create()` method had quite a lot going on in it that made it hard to refactor, analyze, or understand. Use sub-functions to encapsulate certain activities.

scottgonzalez reviewed Dec 6, 2012
View reviewed changes

ui/jquery.ui.menubar.js

.bind( "keydown.menubar", function( event ) {

});

this._on( subMenus, {

Copy link

Member

scottgonzalez Dec 6, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no blank line here

Steven G. Harms added 2 commits January 18, 2013 09:02

menubar: relocate focus event onto *item* v. menuItem

0efe66e

I should have finished my coffee before I committed this.

menubar: fix pixel-shifting visual error

fdc200d

menubar: apply drop-down glyph only on menus w/ subMenu

aafdc17

LEFT cursor in an expanded menu approximates ESCAPE

1e8089b

Steven G. Harms added 4 commits March 16, 2013 11:22

Remove unused variable: seenFirstItem

09d51e8

Do not remove tabIndex varaible

a4a95cc

When this is removed it makes the menu item with a submenu eligible for tabbing / shift-tabbing to / from.

rm stray debugger

b2a3814

Change selector for next .ui-menubar-link -> .ui-button

bab98f2

Steven G. Harms added 4 commits March 22, 2013 08:14

Repair LEFT cursor

79b06b2

Refactor _move by menuItems knowing their neighbors

480de7f

Method rename

56f6469

Correct submenus triggering bad focusout behavior

100f552

jzaefferer closed this Mar 24, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Menubar #829

Menubar #829

sgharms commented Nov 19, 2012

mikesherov Nov 19, 2012

sgharms Nov 19, 2012

jzaefferer commented Nov 19, 2012

sgharms commented Nov 20, 2012

jzaefferer commented Nov 22, 2012

sgharms commented Nov 28, 2012

sgharms commented Dec 1, 2012

mikesherov Dec 1, 2012

sgharms Dec 1, 2012

sgharms Dec 2, 2012

scottgonzalez Dec 3, 2012

sgharms Dec 3, 2012

scottgonzalez Dec 6, 2012

sgharms commented Dec 3, 2012

sgharms commented Dec 3, 2012

sgharms commented Dec 4, 2012

scottgonzalez Dec 6, 2012

sgharms commented Jan 18, 2013

sgharms commented Jan 18, 2013

jzaefferer commented Jan 19, 2013

sgharms commented Jan 19, 2013

sgharms commented Jan 20, 2013

jzaefferer commented Feb 1, 2013

jzaefferer commented Mar 8, 2013

sgharms commented Mar 14, 2013

sgharms commented Mar 15, 2013

jzaefferer commented Mar 15, 2013

sgharms commented Mar 16, 2013

jzaefferer commented Mar 18, 2013

sgharms commented Mar 18, 2013

sgharms commented Mar 22, 2013

jzaefferer commented Mar 23, 2013

jzaefferer commented Mar 24, 2013

Menubar #829

Menubar #829

Conversation

sgharms commented Nov 19, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jzaefferer commented Nov 19, 2012

sgharms commented Nov 20, 2012

jzaefferer commented Nov 22, 2012

sgharms commented Nov 28, 2012

sgharms commented Dec 1, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgharms commented Dec 3, 2012

sgharms commented Dec 3, 2012

sgharms commented Dec 4, 2012

Choose a reason for hiding this comment

sgharms commented Jan 18, 2013

sgharms commented Jan 18, 2013

jzaefferer commented Jan 19, 2013

sgharms commented Jan 19, 2013

sgharms commented Jan 20, 2013

jzaefferer commented Feb 1, 2013

jzaefferer commented Mar 8, 2013

sgharms commented Mar 14, 2013

sgharms commented Mar 15, 2013

jzaefferer commented Mar 15, 2013

sgharms commented Mar 16, 2013

jzaefferer commented Mar 18, 2013

sgharms commented Mar 18, 2013

sgharms commented Mar 22, 2013

jzaefferer commented Mar 23, 2013

jzaefferer commented Mar 24, 2013