Make WordPress Core

Opened 11 years ago

Closed 11 years ago

Last modified 9 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's profile travisnorthcutt Owned by: azaozz's profile 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 11 years ago.
return-true.25934.diff (372 bytes) - added by travisnorthcutt 11 years ago.
25934.diff (352 bytes) - added by nacin 11 years ago.
25934.2.diff (805 bytes) - added by azaozz 11 years ago.

Download all attachments as: .zip

Change History (24)

#1 @travisnorthcutt
11 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
11 years ago

  • Description modified (diff)

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

#5 @SergeyBiryukov
11 years ago

  • Milestone changed from Awaiting Review to 3.8

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

#13 @matt
11 years ago

  • Priority changed from normal to low

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

@nacin
11 years ago

#15 @azaozz
11 years ago

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

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

@azaozz
11 years ago

#16 @JustinSainton
11 years ago

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

#17 @JustinSainton
11 years ago

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

#18 follow-up: @azaozz
11 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
9 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
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.

Note: See TracTickets for help on using tickets.