#25934 closed defect (bug) (fixed)
Spacebar does not toggle checkbox to open link in new window/tab when in distraction free writing mode
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 3.8 | Priority: | low |
Severity: | minor | Version: | 3.7.1 |
Component: | Editor | Keywords: | needs-testing has-patch |
Focuses: | Cc: |
Description (last modified by )
To reproduce:
- Edit a post
- Enter distraction free writing mode
- Select text
- Click button to create a link (or press alt+shift+a)
- Tab to focus the "Open link in a new window/tab" checkbox
- Press the spacebar
- Nothing happens
This seems to have been introduced in [18069], which is said to have fixed #17399, although the patch in #17399 did not introduce the code that causes this. Regardless, the attached patch has been tested against #17399 and seems to introduce no regressions. More testing welcome, though.
Attachments (4)
Change History (24)
#3
follow-up:
↓ 4
@
11 years ago
Thanks for the kudos. Worth noting that we both tested somewhat extensively The patch attached It doesn't appear to reintroduce any regressions addressed in [18069] - it seems to fix the issue here. I wasn't sure from the DFW API whether or not it'd be more proper to have no return value, or to return true. Both seem to work fine. I suppose I'd lean towards returning true...but I don't really know why :)
#4
in reply to:
↑ 3
@
11 years ago
Replying to JustinSainton:
I wasn't sure from the DFW API whether or not it'd be more proper to have no return value, or to return true. Both seem to work fine. I suppose I'd lean towards returning true...but I don't really know why :)
Actually I forgot we'd discussed that at first. I'll add another patch with return true so people smarter than me can choose :)
#6
@
11 years ago
I have a little bit of feedback based on my testing this issue - but I understand this might be straying from the issue a little.
Looking at this, after applying the patch, the spacebar does now toggle the checkbox in the scenario described, however, before and after the patch, I'm finding that there's no visual clue that the focus is on the checkbox after tabbing.
Whereas trying the same steps in 3.7.1 there is indication the focus is on the checkbox.
Best course of action for my findings to open a new ticket?
#7
@
11 years ago
before and after the patch, I'm finding that there's no visual clue that the focus is on the checkbox after tabbing.
Whereas trying the same steps in 3.7.1 there is indication the focus is on the checkbox.
Is 3.7.1 not "before...the patch"?
#8
@
11 years ago
Ah - dang, yes but I didn't test it in 3.7.1.
My findings were that the patch enables the checkbox toggle when the spacebar is pressed on the tested 3.8.xxx revision.
The "same steps" used in 3.7.1 to remind me and compare the current functionality in the current public release were those indicated in the original ticket description. I found that in 3.7.1 there was focus / visual cue on the checkbox when I tabbed to it but in the tested 3.8.xxx revision there was not.
#9
@
11 years ago
In Firefox 25, I see a dotted border around the focused checkbox in both 3.7.1 and trunk, Visual and DFW mode.
#10
@
11 years ago
I do see the same as you report with Firefox 25.0.1
Additionally, in Chrome 32 and Safari 6.1, after following the steps in the ticket description I see no visual indicator on that checkbox with trunk in visual or DFW.
So, perhaps further testing is required (I'm not sure how many positive tests are needed for this to be considered patched / completed) to verify that the patch works but there might still be inconsistent checkbox highlighting to address now in this ticket or move to another ticket to be addressed sometime in the future.
#11
@
11 years ago
I suggest opening a separate ticket for the highlighting of the checkbox. That should be strictly a CSS issue and unaffected by this patch.
#14
@
11 years ago
return false in this context is equivalent to event.preventDefault() and event.stopPropagation(). Obviously, one/both of those is causing the problems here. returning null is equivalent to returning true; neither of these are prevented.
I imagine that one or both of these were intended to be prevented here. You never know, though; that's why it is good to be explicit with these functions.) It's the preventDefault() that's triggering this bug. If you have only event.stopPropagation(), things work fine.
Now the question is, was the preventDefault() necessary for other things? That I don't know. It seems like the Escape key still works, which is what [18069] was trying to fix.
Then the question would be, is event.stopPropagation() necessary? Seems like it isn't, either. In which case [18069] didn't need to change it.
#15
@
11 years ago
Thinking that preventDefault() is needed as the keystroke combinations (Alt +, Alt -, Alt 0) could be used in some browser actions.
#16
@
11 years ago
The only problem with 25934.2.diff is that it doesn't fix the originally reported problem.
#17
@
11 years ago
Just kidding, y'all. 25934.2.diff is utterly flawless. Reported problem fixed, no regressions on ESC issue.
#18
follow-up:
↓ 19
@
11 years ago
- Owner set to azaozz
- Resolution set to fixed
- Status changed from new to closed
In 26649:
#19
in reply to:
↑ 18
;
follow-up:
↓ 20
@
9 years ago
Replying to azaozz:
In 26649:
I am facing this same issue, what should I do? Create a new ticket or what?
Here's the demo http://recordit.co/RYYVahsmHX/
#20
in reply to:
↑ 19
@
9 years ago
Replying to mrahmadawais:
I am facing this same issue, what should I do? Create a new ticket or what?
Yes, please. I could not reproduce on a clean install though.
Major thanks to Justin Sainton for help with debugging this, by the way. In reality he did way more of the work, I just noticed the bug and he was kind enough to help me find it.