Selectmenu: Copy element title and accesskey attributes by fnagel · Pull Request #1298 · jquery/jquery-ui

fnagel · 2014-07-31T10:37:07Z

Changes for #10435

Question: what about attributes for list items?

jzaefferer · 2014-07-31T10:56:59Z

This forum thread was about the title attribute on <option> elements. Can you add that to this PR?

fnagel · 2014-07-31T11:09:58Z

Oh, did I misunderstand what we want here? So, adding title for option, not for select? Or do we want both?

jzaefferer · 2014-07-31T11:20:25Z

I think the discussion was about copying the title attribute on options, but I think it makes sense for both.

fnagel · 2014-07-31T11:54:49Z

OK, I will update this PR.

scottgonzalez · 2014-07-31T12:10:08Z

I'm really not sure what you're trying to test here.

Ah, got to the implementation. Now I see why you added this.

Is this about title, accesskey or both?

Wanted to check if the origin state of the select is restored.

TJ removed the accesskey atrribute in his fiddle so its not duplicated. After some research this seemed needed to me, too.

Let's add a comment about why we're checking this.

scottgonzalez · 2014-07-31T12:12:39Z

Yes, we want title for both.

fnagel · 2014-07-31T14:00:09Z

I've added title copying for options.

scottgonzalez · 2014-07-31T14:02:21Z

No space before the function.

tjvantoll · 2014-07-31T14:03:37Z

With the updates this looks good to me.

scottgonzalez · 2014-07-31T14:03:59Z

Use a single .attr() call.

fnagel · 2014-07-31T14:35:15Z

Fixed code style issues.

scottgonzalez · 2014-07-31T14:44:20Z

You added support for accesskey on the options, but you didn't add any tests for it and you're using different behavior than you used for the select.

scottgonzalez · 2014-07-31T14:46:07Z

Ignore that last comment, the diff was just confusing because it showed the _renderItem() definition above the accesskey code in _destroy(). But this does bring up the fact that we should support accesskey for options.

fnagel · 2014-07-31T17:23:33Z

It seems accesskey for options is not fully supported in all browsers but anyway, it seems like a valid requirement. I've added support for it, let me know what you guys think...

Using accesskeys on list items works (at least) in FF. Do we want to write specific test for accesskeys? Seems like a good idea as we need to make sure that the the select is updated when using accesskeys triggering our list items. Unfortunately we would need to test this for each browser separately. Or is there a helper for accesskey usage?

scottgonzalez · 2014-07-31T17:32:16Z

This looks good. As far as testing accesskey, I'd recommend just doing a manual test in each of the browsers (let's document that here and in the ticket; let me know if you want some help testing in all the browsers). Whenever we eventually switch to WebDriver testing, we can look into automated testing.

fnagel · 2014-07-31T18:16:04Z

Ok. I've tested some browser, can you test what's missing?

I've tested on Windows 8.1, IEs (except IE11) in VM. Tested accesskey for select and for option.

Latest Firefox: ok
Latest Chrome: ok
IE11: works on select but wont open menu and selects current item when pressing again, on option it fails silently
IE10 (Win7): like IE11
IE9 (Win7): like IE11
IE8 (WinXP): like IE11

scottgonzalez · 2014-08-06T11:59:52Z

OS X:

Firefox: Can activate the select, but not the option; same as native
Chrome: The select opens, but doesn't get focus so keyboard use is broken at that point. For the option, the menu doesn't open, but if you manually open it, the correct item is highlighted.
Safari: The select opens, but doesn't get focus so keyboard use is broken at that point. Option does nothing. Both work natively.
Opera: Nothing happens. No native support for options.

fnagel · 2014-08-07T12:32:23Z

I've tested the behavior for FF and Chrome on Win8 again:

Firefox: Can activate the select (gets focus) and the option (doesn't get focus, no native support for options)
Chrome: Can activate the select (doesn't get focus) and the option (doesn't get focus, native support for options)

Note: native selects do not open the menu when activated

fnagel · 2014-08-29T22:57:23Z

When neither copying nor removing the accesskey attributes:

Firefox: accesskey does not work on select or option (option does not work natively anyway)
Chrome: accesskey does not work on select or option
IE11: accesskey does not work on select or option (option does not work natively anyway)
IE10: like IE11
IE9: like IE11

Conclusion: At least on Win none of these browsers follows accesskeys on hidden selects and its options. Seems save to not remove the accesskey attributes.

fnagel · 2014-08-31T12:54:36Z

Can somebody give me a hint why the travis built failed?

tjvantoll · 2014-09-01T20:45:17Z

For reference here's the error: “Testing tests/unit/core/core.html Fatal error: spawn ENOMEM�”

I'm not sure what's up.

scottgonzalez · 2014-09-02T12:31:30Z

ENOMEM should be a Travis problem, not our problem.

scottgonzalez · 2014-09-02T12:38:28Z

Chrome: accesskey does not work on select or option

That's not what I'm seeing with http://jsbin.com/retuxotidero/2/edit

If I click the toggle button to hide the select, I can still change which option is selected via accesskey (verified by clicking the toggle button again to show the select).

scottgonzalez · 2014-09-02T12:41:21Z

I'm not thrilled about having accesskey on options cause the original select and custom select get out of sync, but since this isn't something that works cross-browser natively, I think I'm ok with this. We should, however, lobby to get both of these working everywhere (accesskey on options and accesskey on elements that aren't natively focusable).

fnagel · 2014-09-03T08:07:52Z

You're right, I've missed hat behavior. Note its not the case in FF.

Seems like a keenly idea but yes, we really should :-)

scottgonzalez · 2014-09-17T22:29:10Z

Marking as 1.12 since this is a behavior change that users may not be expecting.

scottgonzalez · 2014-09-18T13:56:32Z

Is this really necessary with the domEqual() check?

Good question. I've stolen this from the autocomplete test suite.

Where? I don't see any references to title in autocomplete.

Anyway, there's nothing to restore because we don't modify the original element at all. I think this whole test change can be removed.

Sorry, I think I misunderstood your intention.

Yes, seems unneeded and a more or less a leftover from the accesskey copying. Removed.

scottgonzalez · 2014-10-30T15:36:04Z

@fnagel Can you update this to handle the new menu item wrappers?

Fixes #10435

fnagel · 2014-11-04T00:24:44Z

I've squashed the commits and rebased onto master.

Fixes #10435 Closes gh-1298

scottgonzalez reviewed Jul 31, 2014
View reviewed changes

Comment thread tests/unit/selectmenu/selectmenu_core.js Outdated

Copy link
Copy Markdown

Member

scottgonzalez Jul 31, 2014

Choose a reason for hiding this comment

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

No space before the function.

scottgonzalez reviewed Jul 31, 2014
View reviewed changes

Comment thread tests/unit/selectmenu/selectmenu_methods.js Outdated

Copy link
Copy Markdown

Member

scottgonzalez Jul 31, 2014

Choose a reason for hiding this comment

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

Use a single .attr() call.

scottgonzalez mentioned this pull request Sep 3, 2014

Document selectmenu's lack of accesskey support jquery/api.jqueryui.com#228

Closed

scottgonzalez added this to the 1.12 milestone Sep 17, 2014

scottgonzalez reviewed Sep 18, 2014
View reviewed changes

Selectmenu: Copy title attributes

9326871

Fixes #10435

fnagel force-pushed the selectmenu-copy-attributes branch from fe629a2 to 9326871 Compare November 4, 2014 00:24

fnagel closed this in 9793739 Nov 4, 2014

fnagel added a commit that referenced this pull request Nov 7, 2014

Selectmenu: Copy title attributes

0f217de

Fixes #10435 Closes gh-1298

fnagel deleted the selectmenu-copy-attributes branch August 25, 2015 15:42

fnagel restored the selectmenu-copy-attributes branch August 25, 2015 15:42

fnagel deleted the selectmenu-copy-attributes branch August 25, 2015 15:42

Conversation

fnagel commented Jul 31, 2014

Uh oh!

jzaefferer commented Jul 31, 2014

Uh oh!

fnagel commented Jul 31, 2014

Uh oh!

jzaefferer commented Jul 31, 2014

Uh oh!

fnagel commented Jul 31, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scottgonzalez commented Jul 31, 2014

Uh oh!

fnagel commented Jul 31, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tjvantoll commented Jul 31, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fnagel commented Jul 31, 2014

Uh oh!

scottgonzalez commented Jul 31, 2014

Uh oh!

scottgonzalez commented Jul 31, 2014

Uh oh!

fnagel commented Jul 31, 2014

Uh oh!

scottgonzalez commented Jul 31, 2014

Uh oh!

fnagel commented Jul 31, 2014

Uh oh!

scottgonzalez commented Aug 6, 2014

Uh oh!

fnagel commented Aug 7, 2014

Uh oh!

fnagel commented Aug 29, 2014

Uh oh!

fnagel commented Aug 31, 2014

Uh oh!

tjvantoll commented Sep 1, 2014

Uh oh!

scottgonzalez commented Sep 2, 2014

Uh oh!

scottgonzalez commented Sep 2, 2014

Uh oh!

scottgonzalez commented Sep 2, 2014

Uh oh!

fnagel commented Sep 3, 2014

Uh oh!

scottgonzalez commented Sep 17, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scottgonzalez commented Oct 30, 2014

Uh oh!

fnagel commented Nov 4, 2014

Uh oh!

Reviewers

Assignees

Labels