WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 3 years ago

#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: travisnorthcutt Owned by: azaozz
Milestone: 3.8 Priority: low
Severity: minor Version: 3.7.1
Component: Editor Keywords: needs-testing has-patch
Focuses: Cc:

Description (last modified by SergeyBiryukov)

To reproduce:

  1. Edit a post
  2. Enter distraction free writing mode
  3. Select text
  4. Click button to create a link (or press alt+shift+a)
  5. Tab to focus the "Open link in a new window/tab" checkbox
  6. Press the spacebar
  7. Nothing happens

https://dl.dropboxusercontent.com/s/drxdlxqiax61hca/2013-11-12%20at%2011.04%20PM.png

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)

spacebar-toggle-checkbox.25934.diff (356 bytes) - added by travisnorthcutt 5 years ago.
return-true.25934.diff (372 bytes) - added by travisnorthcutt 5 years ago.
25934.diff (352 bytes) - added by nacin 5 years ago.
25934.2.diff (805 bytes) - added by azaozz 5 years ago.

Download all attachments as: .zip

Change History (24)

#1 @travisnorthcutt
5 years ago

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.

#2 @SergeyBiryukov
5 years ago

  • Description modified (diff)

#3 follow-up: @JustinSainton
5 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 @travisnorthcutt
5 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 :)

#5 @SergeyBiryukov
5 years ago

  • Milestone changed from Awaiting Review to 3.8

#6 @seanchayes
5 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 @travisnorthcutt
5 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 @seanchayes
5 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 @SergeyBiryukov
5 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 @seanchayes
5 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 @travisnorthcutt
5 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.

#13 @matt
5 years ago

  • Priority changed from normal to low

#14 @nacin
5 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.

@nacin
5 years ago

#15 @azaozz
5 years ago

Thinking that preventDefault() is needed as the keystroke combinations (Alt +, Alt -, Alt 0) could be used in some browser actions.

Last edited 5 years ago by azaozz (previous) (diff)

@azaozz
5 years ago

#16 @JustinSainton
5 years ago

The only problem with 25934.2.diff is that it doesn't fix the originally reported problem.

#17 @JustinSainton
5 years ago

Just kidding, y'all. 25934.2.diff is utterly flawless. Reported problem fixed, no regressions on ESC issue.

#18 follow-up: @azaozz
5 years ago

  • Owner set to azaozz
  • Resolution set to fixed
  • Status changed from new to closed

In 26649:

DFW: don't block the "keyup" event, fixes toggling the checkbox in the Link dialog with the spacebar. Fixes #25934.

#19 in reply to: ↑ 18 ; follow-up: @mrahmadawais
3 years ago

Replying to azaozz:

In 26649:

DFW: don't block the "keyup" event, fixes toggling the checkbox in the Link dialog with the spacebar. Fixes #25934.

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 @SergeyBiryukov
3 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.

Note: See TracTickets for help on using tickets.