984980 - Keyboard shortcut to switch between preview mode and edit mode

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Anthony Ricaud (:rik)]

Github uses Cmd-Shift-P on Mac to do that and it's pretty neat.

Comment 1

[Acho Arnold (:bePolite)]

Attachment: Patch for bug 984980

This patch uses the keyboard shortcut Ctrl + Shift + P to switch between preview mode and edit mode when writing comments

Comment 2

[Gervase Markham [:gerv]]

Comment on attachment 8396636 [details] [diff] [review]
Patch for bug 984980

I'm fairly sure I remember reading that the a11y team said not to use aria roles for doing things like this. Surely there's a better way to detect whether we are previewing or not?

glob: any issues with us watching every key event on these pages?

Gerv

Comment 3

[Anthony Ricaud (:rik)]

It would be nice to use Cmd-Shift-P on Mac instead of Ctrl-Shift-P.

Comment 4

[Acho Arnold (:bePolite)]

>I'm fairly sure I remember reading that the a11y team said not to use aria roles for doing things like this

Okey I didn't know about that fact

>It would be nice to use Cmd-Shift-P on Mac instead of Ctrl-Shift-P.
I'm sorry I only have access to a linux computer at the moment so I can't test Cmd on Mac. I would be very grateful if you could give me the keycode of Cmd on Mac

Comment 5

[Anthony Ricaud (:rik)]

Cmd is 224.

Comment 6

[Acho Arnold (:bePolite)]

Attachment: Updated patch for bug 984980

This patch checks whether we are previewing or not without using aria

It also enables Cmd-Shift-P to be used on Mac.

Comment 7

[Acho Arnold (:bePolite)]

Attachment: Updated patch for bug 984980

This patch checks whether we are previewing or not without using aria

It also enables Cmd-Shift-P to be used on Mac.

Comment 8

[Gervase Markham [:gerv]]

Comment on attachment 8399150 [details] [diff] [review]
Updated patch for bug 984980

r=gerv. We should get someone with a Mac to test it once it is checked in.

Gerv

Comment 9

[:glob ✱]

Comment on attachment 8399150 [details] [diff] [review]
Updated patch for bug 984980

Review of attachment 8399150 [details] [diff] [review]:
-----------------------------------------------------------------

there's some minor few style issues with this patch that i'd like to see addressed first.

::: js/field.js
@@ +1089,5 @@
> +    // Store an entry for every key pressed
> +    keys[e.keyCode] = true;
> +
> +    // (Ctrl XOR cmd) + Shift + P
> +    if ( (!keys[17] != !keys[224]) && keys[16] && keys[80]) {

remove space between ( (

@@ +1092,5 @@
> +    // (Ctrl XOR cmd) + Shift + P
> +    if ( (!keys[17] != !keys[224]) && keys[16] && keys[80]) {
> +        // Check if we are already in preview mode
> +        var Dom = YAHOO.util.Dom;
> +        if (Dom.hasClass('comment_preview_tab', 'active_comment_tab')){

you're only referencing Dom once, there's no need to create a variable for it

@@ +1098,5 @@
> +            document.getElementById('comment').focus();
> +            YAHOO.util.Event.preventDefault(e);
> +        }
> +
> +        else{

need a space after else

::: template/en/default/attachment/create.html.tmpl
@@ +14,5 @@
> +[% javascript = BLOCK %]
> +  YAHOO.util.Event.onDOMReady(function() {
> +    YAHOO.util.Event.addListener(window, 'keydown', keys_pressed, [% bug.id FILTER none %]);
> +    YAHOO.util.Event.addListener(window, 'keyup', keys_released);
> +    });

this line is indented one level too much

Comment 10

[Acho Arnold (:bePolite)]

Attachment: A revision of my previous patch for Bug 984980

This patch solves the issues raised by glob in comment 9 above

Comment 11

[Gervase Markham [:gerv]]

Comment on attachment 8401683 [details] [diff] [review]
A revision of my previous patch for Bug 984980

r=gerv.

Gerv

Comment 12

[Dave Miller [:justdave]]

I guess this gives us a framework for doing more stuff with keys in the future, too.

My only question on this is should we do something to make this discoverable?  It looks nice but it only works if you know about it.

Comment 13

[Gervase Markham [:gerv]]

Counting objects: 21, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (9/9), done.
Writing objects: 100% (11/11), 1.57 KiB | 0 bytes/s, done.
Total 11 (delta 8), reused 0 (delta 0)
To ssh://[e-mail]/bugzilla/bugzilla.git
   23bad39..08f83a7  master -> master

Thanks, Acho!

Gerv

Comment 14

[Anthony Ricaud (:rik)]

\o/

If you can point me to a Bugzilla instance with that patch, I can do the Mac test.

Comment 15

[Frédéric Buclin]

The patch contained tabs, making Tinderbox to turn orange.

To ssh://[e-mail]/bugzilla/bugzilla.git
   08f83a7..b639a1a  master -> master

Comment 16

[Frédéric Buclin]

Comment on attachment 8401683 [details] [diff] [review]
A revision of my previous patch for Bug 984980

>diff --git a/template/en/default/bug/show-header.html.tmpl b/template/en/default/bug/show-header.html.tmpl

>++[% javascript = BLOCK %]

The extra + before [% javascript = BLOCK %] makes + to be displayed before <!DOCTYPE html>, which breaks the rendering of HMTL pages. Has this patch been tested??


+        YAHOO.util.Event.onDOMReady(function() {
+            YAHOO.util.Event.addListener(window, 'keydown', keys_pressed, [% bug.id FILTER none %]);
+            YAHOO.util.Event.addListener(window, 'keyup', keys_released);
+        });
+[% END %]

The indentation is wrong. Templates use 2 whitespaces, neither 4 nor 8.


To ssh://[e-mail]/bugzilla/bugzilla.git
   536c249..ae77f29  master -> master

Comment 17

[Dave Miller [:justdave]]

This broke URL rewrites apparently, need to back it out for now (we just released with this in it and now have a broken release :( about to re-roll for unrelated reasons (wasn't the only regression) and the release will be easier without this, we can fix it and put it back after the release is re-rolled.

Comment 18

[Frédéric Buclin]

Patch backed out.

To ssh://[e-mail]/bugzilla/bugzilla.git
   ae77f29..406dcbb  master -> master

Comment 19

[Frédéric Buclin]

Comment on attachment 8401683 [details] [diff] [review]
A revision of my previous patch for Bug 984980

See comments 15-17: 3 regressions in a single patch!

Comment 20

[Gervase Markham [:gerv]]

:-(

I tested an earlier version but not the very latest one, as I understood it to be only fixing the "minor style issues" from comment 9. :-| So that's how I missed the extra "+". bePolite: can you tell us how you didn't spot that problem?

My editor (geany) has very un-useful "show white space" (puts tiny dots in all whitespace) and "show line endings" (ugly "CR" or "LF" black text) options, but not the far more useful "show tab characters, plus any whitespace at end of lines" option that I'd like to see. :-( So I don't have an easy way of detecting patches with tabs. Is it required that reviewers run the tests with all patches they review, or do we expect the patch author to have done that?

Gerv

Comment 21

[Frédéric Buclin]

(In reply to Gervase Markham [:gerv] from comment #20)
> an easy way of detecting patches with tabs. Is it required that reviewers
> run the tests with all patches they review, or do we expect the patch author
> to have done that?

The reviewer is supposed to run tests, especially with new contributors who probably don't even know that these tests exist.

Comment 22

[Acho Arnold (:bePolite)]

(In reply to Gervase Markham [:gerv] from comment #20)
> bePolite: can you tell us how you didn't spot that
> problem?

It seems i was just focused on solving the problem addressed by this bug that I didn't notice the side effects of my patch.

I noticed the broken URL rewrites later but I also had no Idea that it was related to the javascript changes in my patch.

I will endevour to be much more careful next time

Comment 23

[Kohei Yoshino]

Bugzilla 6.0 will ship with the new modal UI created for BMO, which offers the keyboard shortcut.