Skip to main content

Search and Top Navigation

#11145 open bug ()

Opened February 19, 2015 03:37PM UTC

Last modified February 23, 2015 08:44PM UTC

Selectmenu: Selection inside contenteditable is lost in Safari

Reported by: spjonez Owned by: spjonez
Priority: minor Milestone: none
Component: ui.selectmenu Version: 1.11.3
Keywords: Cc:
Blocked by: Blocking:
Description

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.

Attachments (0)
Change History (18)

Changed February 20, 2015 06:24PM UTC by scottgonzalez comment:1

owner: → spjonez
status: newpending

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

Changed February 20, 2015 07:38PM UTC by spjonez comment:2

status: pendingnew

Changed February 20, 2015 08:21PM UTC by scottgonzalez comment:3

status: newopen
summary: Losing selection when clicking menu in Safari (Yosemite)Selectmenu: Selection inside contenteditable is lost in Safari

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

Changed February 20, 2015 08:31PM UTC by spjonez comment:4

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.

Changed February 20, 2015 08:50PM UTC by scottgonzalez comment:5

_comment0: 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 see if we don't browser sniff. We test for functionality, which you can see in the actual conditional.1424465498660023

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.

Changed February 20, 2015 10:20PM UTC by spjonez comment:6

_comment0: http://jsfiddle.net/as7p79g4/ \ \ With this code selection is maintained in all browsers on Mac. It also works in FF and Chrome on Windows. I have not tested IE as I don't have to support that browser. \ \ I'm not suggesting that 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. \ \ 1424471003013512
_comment1: 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 that 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. \ \ 1424471031707071
_comment2: 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. \ \ 1424471417801783
_comment3: 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 Safari on Yosemite loses the selection and if you comment focus out it doesn't. \ 1424471438536243
_comment4: 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 Safari on Yosemite loses the selection. \ 1424471464619380

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.

Changed February 20, 2015 10:48PM UTC by scottgonzalez comment:7

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.

Changed February 20, 2015 11:23PM UTC by spjonez comment:8

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

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.

Changed February 22, 2015 03:51PM UTC by tj.vantoll comment:9

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

Changed February 23, 2015 06:42PM UTC by spjonez comment:10

_comment0: 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 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 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.1424717069731973

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.

Changed February 23, 2015 06:46PM UTC by scottgonzalez comment:11

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.

Changed February 23, 2015 06:48PM UTC by spjonez comment:12

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

Changed February 23, 2015 06:55PM UTC by scottgonzalez comment:13

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

Changed February 23, 2015 08:11PM UTC by spjonez comment:14

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?

Changed February 23, 2015 08:14PM UTC by scottgonzalez comment:15

Replying to [comment:14 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?

Changed February 23, 2015 08:21PM UTC by spjonez comment:16

_comment0: 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. \ \ #10144 used something similar to the original. I don't follow your Git repo to know when or what version the selection code was added or for what purpose. The original did support keypress to select and did not flicker the selection when accessing the menu as focus was not triggered. \ \ > 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?1424723013408709
_comment1: 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. \ \ #10144 used something similar to the original. 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 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?1424723035549589
_comment2: 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. \ \ #10144 used something similar to the original. 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 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?1424723052356591
_comment3: 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. \ \ #10144 used something similar to the original. 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 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?1424723106823510

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?

Changed February 23, 2015 08:30PM UTC by scottgonzalez comment:17

Replying to [comment:16 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.

Changed February 23, 2015 08:44PM UTC by spjonez comment:18

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.