Menubar: move to next item applied focus() to too many items #953

sgharms · 2013-04-07T20:29:53Z

_move was believed to specify just one item,
called focusableTarget. Using the broader
selectors this would return anything matching a or
button. This means that the next item as well
as any buttons or as found thereunder would
also have their focus() event fired.

scottgonzalez · 2013-04-08T12:19:36Z

Thanks, can you add a unit test showing the behavior? Also, this new selector is unnecessarily duplicative.

I haven't reviewed menubar yet, but this is pointing out a pretty big problem to me. Menubar cannot use button's classes without a dependency on button.

sgharms · 2013-04-09T03:39:19Z

My feeling on menubar at the moment is that its entire construction is flawed. It's built like a giant piece of DOM that responds to events instead of a series of encapsulating widgets. I'm of the opinion that there should be better abstractions in place: yes there should be a menubar widget but also a menubarItem widget (using $.Widget) and a submenu widget, and probably others.

There's considerable mental pain and hidden bugs in the current design (which, yes, I had a hand in). I feel like this could be a whole lot less buggy if it were to be designed this way. Am I opening an unholy can of worms? Or should widgets not depend on other widgets? I personally think that would be a whole lot simpler (to say nothing of infinitely easier to test).

jzaefferer · 2013-04-09T09:33:09Z

Menubar actually uses the menu widget, so its not entirely monolithic. So we're generally open to composing widgets of other widgets. If further splits would actually help here remains to be seen. Is that something you're interested in prototyping?

In general considering the complexity of menu itself its not suprising that menubar ends up even more complex. The combination of mouse and keyboard handling, while managing focus, just leads to that. I'm not sure if further decomposition can really make a positive difference.

scottgonzalez · 2013-04-09T10:03:59Z

I am strongly opposed to the addition of a menubaritem widget.

kborchers · 2013-04-09T11:55:50Z

I also agree that a menubaritem widget is not a good idea.

sgharms · 2013-04-10T21:46:37Z

@jzaefferer It's reassuring that you see that menubar will be an increment of weirdness around menu.

Others: will not create a new widget.

@scottgonzalez Will create test case

All: Finishing up the top two bugs at http://wiki.jqueryui.com/w/page/38666403/Menubar.

sgharms · 2013-04-12T23:44:39Z

@scottgonzalez Changes made, tests provided. I should be more wary when writing code before work 🌔 .

sgharms · 2013-04-12T23:54:17Z

@jzaefferer Where does this leave things in terms of http://wiki.jqueryui.com/w/page/38666403/Menubar ? If this is squared away, I can move to "Disable items/menus with class="ui-state-disabled" "

sgharms · 2013-04-14T19:47:20Z

@kborchers and @scottgonzalez I know you both were against the idea of creating a new widget but...

I have this PR: #958 where I show what happened when I played with the idea. I think the code is much better for it, but I would really like a way to:

Get the benefits of $.Widget
Create a private widget ( or such as that is possible in Javascript ;) )

If you could take a look at that PR and see if there's anything good about it that can be brought over, or tell me how to implement a private widget, I think this code would be the better for it.

jzaefferer · 2013-04-16T14:25:56Z

Rebase this branch against origin/menubar, to allow a fast-forward merge, currently needs a merge commit
Clean up the commits, probably getting rid of all but the first two
Fix more bugs, for example: Tab to the first menubar, cursor right, shift tab. Moves focus to the first item, instead of something before the menubar. Similar when tabbing to the second menubar, cursor right, then shift-tab. It moves focus to the next(?) menubar instead of the previous.
There's probably a lot more issues like that. I don't really want to do more testing like that, since the back and forth is pretty inefficient. Can you do more manual testing like that, then encode those tests as unit tests?
There are a bunch of issues with the code itself, but I'd like to get rid of most of the bugs first, along with unit tests and then start refactoring.

sgharms · 2013-04-16T16:12:37Z

Can do these top 3 today. Writing more test suite will need to be ongoing.

`_move` was believed to specify just one item, called `focusableTarget`. Using the broader selectors this would return anything matching `a` or `button`. This means that the next item _as well as_ any `button`s or `a`s found thereunder would _also_ have their focus() event fired.

1. Load page 1. Tab to first bar 1. Right cursor 1. Down cursor 1. Right cursor 1. Error Test the object that is tested for an `active` for definition before querying that property. Create new method for assessing whether or not a new submenu should be opened. The testing here is a smell. This logic should be simplified.

sgharms · 2013-04-17T01:28:07Z

Closing. Will re-open when I have more tests.

sgharms · 2013-04-18T15:38:06Z

Is there a test I can crib from which tests TAB and shift-TAB behavior? I want to provide the
requested unit tests that ensure TAB order is working properly. I think i have
it working, but this has (ahem) proved faillible in the past and I'd like to
have the insurance.

scottgonzalez · 2013-04-18T15:39:25Z

jquery-ui/tests/unit/dialog/dialog_core.js

Line 119 in 71a332e

asyncTest( "Prevent tabbing out of dialogs", function() {

sgharms · 2013-04-24T14:43:34Z

@jzaefferer

Per your comments of 2013-04-16

Fix more bugs, for example: Tab to the first menubar, cursor right, shift tab. Moves focus to the first item, instead of something before the menubar. Similar when tabbing to the second menubar, cursor right, then shift-tab. It moves focus to the next(?) menubar instead of the previous.

These two specific bugs have been addressed. I've attempted to break things manually but have not been able to produce any other cursor key or TAB-order bugs.

There's probably a lot more issues like that. I don't really want to do more testing like that, since the back and forth is pretty inefficient. Can you do more manual testing like that, then encode those tests as unit tests?

Commit 4e47df2 brings in unit testing for some of the basics of this but, per its commit message, I don't see why my invocations are failing as the demo works and my other uses of simulate work. Perhaps someone can suggest a fix in this PR.

https://github.com/sgharms/jquery-ui/blob/4e47df29134b45cb19a6ce712b1ac6e9838a6186/tests/unit/menubar/menubar_events.js#L62

unit tests and then start refactoring.

My goal would be now to move to this phase of things with focus on getting the unit tests in place. I suspect I can mutate this code to look a great deal more like dialog (cf. previous discussion on internal code structuring) without too much supervision, but getting a bug free (but ugly) build that starts to get some testing around events (particularly cursor and TAB events) is what I'd like to see this PR achieve.

sgharms · 2013-04-24T16:38:41Z

@scottgonzalez Removed junk you pointed out in meeting today.

scottgonzalez · 2013-04-24T18:40:37Z

tests/unit/menubar/menubar_events.js

@@ -46,4 +46,76 @@ test( "hover over a menu item with no sub-menu should close open menu", function
 	equal($(".ui-menu:visible").length, 0,  "After triggering a sub-menu, a click on a peer menu item should close the opened sub-menu");
 });

+test ( "_findNextFocusableTarget should find one and only one item", function() {


Why are we testing an internal method?

You asked:

Thanks, can you add a unit test showing the behavior? Also, this new selector is unnecessarily duplicative

To ensure that I got the extracted method to return the right thing, I wrote this unit test. I didn't want to have to do the unit-test-as-integration test where a widget is created, actions are executed and then the state of some element is assessed. That seemed less in the spirit of unit test. I'm open to suggestions though: How would you recommend exercising this?

But, let me say, this is going to go away when I reorganize the internals so let's not spend much time on this.

Most of our tests aren't unit tests. We don't really care about that level of coverage. We care about our public APIs.

Very helpful. Will comply.

jzaefferer · 2013-04-30T10:49:23Z

ui/jquery.ui.menubar.js

-		});
+		} else {
+			menubar._off( $anItem, "click mouseenter" );
+			menubar._hoverable( $anItem );


Assuming the _off call above can be removed, this can probably be dropped as well, since _hoverable is already called in _initializeItems.

jzaefferer · 2013-04-30T11:14:43Z

Done with my review for now. I've left a bunch of comments on individual commits as well as the PR itself. I hope fixing the tabIndex=1 issue resolves a lot of the bugs we've seen before.

Otherwise I think you're doing a great job at improving things. The codebase looks much better now. There are still a few odditities, as noted, but overall I can understand what's going on even though it has been years since I wrote some of the original code.

jzaefferer · 2013-04-30T11:17:47Z

As for the tab unit test, I lost track of what the issue is there. Maybe I can get together with @scottgonzalez later today to dig into that together.

scottgonzalez · 2013-04-30T16:20:22Z

The reason the test fails is because you can't actually test the behavior of tab navigation. Simulating a TAB key won't actually move focus, because it's just triggering an event. We need Selenium/WebDriver in order to test this functionality, but we don't currently have a system for writing those tests. The reason this works in the dialog test suite is because the dialog plugin actually manages what happens when you press tab (assuming you're on the first or last tabbable element).

You can write a test that verifies that only one menu item is tabbable at a time, and that the tabindex updates appropriately as you move focus via arrow keys, but that's about the extent of the testing that can be done at this time.

sgharms · 2013-05-01T00:50:11Z

Completed fixing several small points of Jörn: mostly whitespace or incorrect method, etc. More "substantive" changes that require a test case are underway.

sgharms · 2013-05-01T03:04:55Z

Great Scott! I will have those tests added in a jiffy.

jzaefferer · 2013-05-01T12:37:09Z

tests/unit/menubar/menubar_events.js

 });

+asyncTest( "Cursor keys should move focus within the menu items", function() {


A few things here:

Since you're not actually triggering focus, it doesn't have to be an async test anymore.

The debugDelay variable can go away.

You should add an assertion that checks that the now focussed element is the only one that's tabbable. Can be one or more assertions.

jzaefferer · 2013-05-01T12:38:31Z

Looking pretty good already. Once you fixed the tabindex to 0 (instead of 1), along with whatever other changes are still missing, I'll get back to actual testing.

sgharms · 2013-05-02T10:37:32Z

@jzaefferer I have finished implementing the issues you mentioned in various commit notes, line notes, etc.

Outstanding Issues

unwrap() seeming not to work: see eaaf323
Style review

Steps Forward

Clear outstanding issues, above
Squash these tons of commits
Merge it! 🎏

Status

For the moment, I'm undertaking no new actions.

jzaefferer · 2013-05-03T08:55:38Z

I don't think those two issues are reason enough to stop. I've pinged Scott about the unwrap issue, since I don't see why it wouldn't work, and we can do the style review later, or step by step with other updates. Can you go ahead and squash commits? I'll merge then.

@scottgonzalez

This code base had been noted for being "unusual" in comparison with other widgets e.g. `dialog` by @scottgonzalez et al. Further drift was introduced by @sgharms. This commit reflects the work of bringing `menubar` closer to `dialog` by means of removing extremely unusual code invocations and by implementing feedback from a detailed review by @jzaefferer.

sgharms · 2013-05-03T14:05:41Z

@jzaefferer, @scottgonzalez: We're all squashed up and ready to merge.

jzaefferer · 2013-05-03T14:31:33Z

Did a bit of local testing, then merged this and origin/master to bring the branch up-to-date. Great work, Steven! Of course there's still more to do.

honeypc · 2013-05-21T02:02:17Z

Cannot call methods on menu prior to initialization; attempted to call method 'collapseAll'. When using menubar don''t exist submenu.

New
Edit
Delete

$(function () {
$('#command-menubar').menubar();
});

sgharms mentioned this pull request Apr 14, 2013

Refactor: Create subcomponent "ui.menubarMenuItem" #958

Closed

Steven G. Harms added 2 commits April 16, 2013 10:13

sgharms closed this Apr 17, 2013

sgharms reopened this Apr 18, 2013

scottgonzalez reviewed Apr 24, 2013
View reviewed changes

jzaefferer reviewed Apr 30, 2013
View reviewed changes

jzaefferer reviewed May 1, 2013
View reviewed changes

jzaefferer merged commit d81479b into jquery:menubar May 3, 2013

		});

		asyncTest( "Cursor keys should move focus within the menu items", function() {

Menubar: move to next item applied focus() to too many items #953

Menubar: move to next item applied focus() to too many items #953

Uh oh!

Conversation

sgharms commented Apr 7, 2013

Uh oh!

scottgonzalez commented Apr 8, 2013

Uh oh!

sgharms commented Apr 9, 2013

Uh oh!

jzaefferer commented Apr 9, 2013

Uh oh!

scottgonzalez commented Apr 9, 2013

Uh oh!

kborchers commented Apr 9, 2013

Uh oh!

sgharms commented Apr 10, 2013

Uh oh!

sgharms commented Apr 12, 2013

Uh oh!

sgharms commented Apr 12, 2013

Uh oh!

sgharms commented Apr 14, 2013

Uh oh!

jzaefferer commented Apr 16, 2013

Uh oh!

sgharms commented Apr 16, 2013

Uh oh!

sgharms commented Apr 17, 2013

Uh oh!

sgharms commented Apr 18, 2013

Uh oh!

scottgonzalez commented Apr 18, 2013

Uh oh!

sgharms commented Apr 24, 2013

Uh oh!

sgharms commented Apr 24, 2013

Uh oh!

scottgonzalez Apr 24, 2013

Choose a reason for hiding this comment

Uh oh!

sgharms Apr 25, 2013

Choose a reason for hiding this comment

Uh oh!

scottgonzalez Apr 25, 2013

Choose a reason for hiding this comment

Uh oh!

sgharms Apr 25, 2013

Choose a reason for hiding this comment

Uh oh!

jzaefferer Apr 30, 2013

Choose a reason for hiding this comment

Uh oh!

jzaefferer commented Apr 30, 2013

Uh oh!

jzaefferer commented Apr 30, 2013

Uh oh!

scottgonzalez commented Apr 30, 2013

Uh oh!

sgharms commented May 1, 2013

Uh oh!

sgharms commented May 1, 2013

Uh oh!

jzaefferer May 1, 2013

Choose a reason for hiding this comment

Uh oh!

jzaefferer commented May 1, 2013

Uh oh!

sgharms commented May 2, 2013

Outstanding Issues

Steps Forward

Status

Uh oh!

jzaefferer commented May 3, 2013

Uh oh!

sgharms commented May 3, 2013

Uh oh!

jzaefferer commented May 3, 2013

Uh oh!

honeypc commented May 21, 2013