Selectmenu: Selection inside contenteditable is lost in Safari

[spjonez]

Clicking a selectmenu clears text selection in Safari on Yosemite. The issue appears to be in _setSelection when this.button.focus() is called. Commenting that line out or moving it into the IE if fixes the issue.

Would send PR but I doubt that's what you want since the comment says IE8, and I'm unsure what your policy is on browser detection.

[Scott González]

I'm not seeing this behavior. Here's a screen capture of me using ​http://view.jqueryui.com/1.11.3/demos/selectmenu/default.html in Safari 8 on OS X Yosemite via BrowserStack: ​https://cloudup.com/chXBSVgR9hM

[spjonez]

​http://jsfiddle.net/5g9gqcgr/

[Scott González]

Your proposed fix breaks the selectmenu since focus is lost, so key commands no longer work.

[spjonez]

Without this.button.focus() key commands work for me on Mac FF 35.0.1, Safari 8.0.3, Chrome 40. Your if block targets old versions of IE so focus shouldn't be inside there if its needed for IE9-11. I was simply pointing out what code breaks the selection since I don't know how jQUI targets Safari.

[Scott González]

I'm not sure what code you're referring to. You've referred to this.button.focus() and you've referred to the body of the conditional, but those are not the same code. The focus call is clearly not inside a conditional: ​https://github.com/jquery/jquery-ui/blob/1e7a1e811f7c9d624b2561ea3e2f34f80773ad0c/ui/selectmenu.js#L408

As far as how "jQUI targets Safari" all I can say is we don't browser sniff. We test for functionality, which you can see in the actual conditional.

[spjonez]

​http://jsfiddle.net/as7p79g4/

With this code selection is maintained in all browsers on Mac and FF/Chrome on Windows. Keypress to select also work. I have not tested IE as I don't have to support that browser. I'm not suggesting this is a proper fix, it was my hasty one since I don't have to worry about all versions of IE.

Your range check is too specific for the focus call to go inside the if (as I put above) if it's needed for IE9-11. I'm assuming you have a policy on how specific browser issues are handled and since I'm not familiar with it I'm only reporting where it breaks. If focus is called in Safari on Yosemite the selection is lost when you click the menu.

[Scott González]

As I said earlier, this breaks keyboard functionality. Here's your original test case with your patch applied: ​http://jsfiddle.net/5g9gqcgr/2/

In Chrome or Safari, select some text in the contenteditable div, click the selectmenu button, then try to use your arrow keys to navigate the menu.

[spjonez]

It only breaks the keyboard inside a contenteditable, outside a contenteditable the keyboard does work. In my situation that is sufficient and obviously it's not for the library. My comments are only to narrow your search.

[tj.vantoll]

fyi this is very similar to #10144, which fixed this issue for non-Safari browsers.

[spjonez]

It is the same issue, 101444 fixed it by blocking the event but the current code uses range/selection save/restore code. I'm guessing these changes were needed for keypress to select. It does have the side effect of making the selection flicker when accessing the menu where the event prevent did not.

IMO keypress to select an option while inside a contenteditable isn't a big deal if your selection is lost. Not many users would do that in a rich editor use case. Maintaining the selection when clicking the menu is necessary to apply commands to a range inside a contendeditable div. To resolve this ticket at the expense of keypress to select while inside a contenteditable only the focus call has to changed to only fire in IE. The focus call doesn't seem to effect FF/Chrome so it could also be changed to be ignored by Safari and called by all the rest.

I doubt you can use feature detection as this is obviously a bug/nuance in Safari since all the other browsers respond differently. Do you have a policy on how situations such as this are resolved? I'm willing to take a stab at a patch but I don't want to code anything without knowing what your expectations are.

[Scott González]

This isn't *just* about keyboard navigation. It's about focus handling. AT Users will be completely lost if focus isn't being applied properly.

[spjonez]

What about skipping the focus call if the range is inside a contenteditable div?

[Scott González]

That's exactly what would break the context. The focus has to be set to the button.

[spjonez]

The original plugin didn't call focus on click your version does. What about removing the selection save/restore and skipping the focus on line 88, then setting focus on keypress?

[Scott González]

Replying to spjonez:

The original plugin didn't call focus on click your version does.

That's fairly vague. I'm not sure what version you're referring to. Have you confirmed that whatever version you're referring to doesn't have any bugs that we've fixed over time?

What about removing the selection save/restore and skipping the focus on line 88

That would surely regress. We didn't just write that code for fun.

then setting focus on keypress?

Keypress on what? Any element in the document?

[spjonez]

​https://github.com/fnagel/jquery-ui/blob/selectmenu/ui/jquery.ui.selectmenu.js line 93

vs

​https://github.com/jquery/jquery-ui/blob/master/ui/selectmenu.js line 88

That would surely regress. We didn't just write that code for fun.

The original fix for #10144 prevented the event. I don't follow your Git repo to know when or what version the selection save/restore code was added or for what purpose. The original plugin did support keypress to select and did not flicker the selection when accessing the menu as focus was not called on click so no save/restore was needed.

Keypress on what? Any element in the document?

Keypress on the menu while it's open. I have not looked into how the original did this I'm assuming you'd know more since your version is based on it?

[Scott González]

Replying to spjonez:

The original fix for #10144 prevented the event. I don't follow your Git repo to know when or what version the selection save/restore code was added or for what purpose. The original plugin did support keypress to select and did not flicker the selection when accessing the menu as focus was not called on click so no save/restore was needed.

git blame or using GitHub's blame will tell you why it changed. See #10639.

Keypress on what? Any element in the document?

Keypress on the menu while it's open. I have not looked into how the original did this I'm assuming you'd know more since your version is based on it?

The menu isn't receiving keypresses if focus isn't changed. This is exactly what I've been saying over and over.

Focus has to be set to the button in order for anything to work. Clicking the button is causing the focus to move and the selection to clear. We're restoring the selection, which is then killing the focus on the button in some browsers. So we then have to fix the focus, which is now stupidly killing the selection in Safari. It's become a vicious cycle. Preventing the default behavior of mousedown prevents focus from moving, which caused a regression. No matter how many times you ask us to just not set focus, that will not be an acceptable solution.

[spjonez]

​https://github.com/fnagel/jquery-ui

Download it, open jquery-ui-selectmenu/demos/selectmenu/default.html in Safari select some text and click the menu. Selection does not flicker when the menu opens. Start typing the name of an option and you can verify the menu jumps between options without losing the selection.

All I'm asking for is the same functionality the original had. I've already had to rewrite a lot of code to use your version: two formatting functions vs one if you want both selected + menu items customized, new event name, different binding if you want to trigger, etc.