If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Reuse editMenuKeys in scratchpad.xul

RESOLVED FIXED in Firefox 13

Status

()

Firefox
Developer Tools: Scratchpad
P4
trivial
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: msucan, Assigned: capella)

Tracking

Trunk
Firefox 13
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=msucan][lang=xul])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
In scratchpad.xul we redefine a number of Edit-related keybindings that we could reuse from the edit menu overlay. See bug 684445 comment 20.
(Assignee)

Updated

6 years ago
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
(Assignee)

Comment 1

6 years ago
Created attachment 602697 [details] [diff] [review]
Patch (v1)

The description for this bug/change request is a little vague, but the attached may serve as a start point. Please let me know if this is along the lines of what you had in mind ...

Summarizing ...

I added context menu keys, and incorporated them into their context menuitems in editmenuoverlay.xul

I then used those items for cut/copy/paste/selectall in the Edit Menu dropdown, and for cut/copy/paste/selectall/delete in the Context Menu pop-up in scratchpad.xul

Finally, I removed (now) unused items for cut/copy/paste/selectall in scratchpad.dtd

--mark
(Reporter)

Comment 2

6 years ago
Mark: that's great! Thanks for taking the patch!

Yep, the description is a bit vague, indeed - my bad!

Patch looks great, but please limit the changes to scratchpad.xul and the dtd only - you do not need to change the edit menu overlay.

What we intend to fix with this bug is to only make better use of the existing editMenuKeys from the editMenuOverlay.

Thanks again!
(Assignee)

Comment 3

6 years ago
I got into the editMenuOverlay, as the context items for cut/copy/paste/delete wont display access keys on the right side, and kinda just expanded on the idea.
(Reporter)

Comment 4

6 years ago
(In reply to Mark Capella [:capella] from comment #3)
> I got into the editMenuOverlay, as the context items for
> cut/copy/paste/delete wont display access keys on the right side, and kinda
> just expanded on the idea.

Yeah. You can fix that in scratchpad.xul. Change the context menu items: add key="key_foo".
(Assignee)

Comment 5

6 years ago
I must be missing something ... I tried (for example) and get runtime errors ...

     <menuitem id="cMenu_cut" key="key_cut"/>
----------------------------------^
(Assignee)

Comment 6

6 years ago
Created attachment 602703 [details] [diff] [review]
Patch (v2)

I spoke too soon and found the problem ...
Attachment #602697 - Attachment is obsolete: true
(Reporter)

Comment 7

6 years ago
Comment on attachment 602703 [details] [diff] [review]
Patch (v2)

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

Thanks for your patch Mark! This is great work.

Please do the same for the find keys. We should use the editMenuOverlay keys for those cases as well.

For key_undo and key_redo we need to copy them from editMenyOverlay.xul to source-editor-overlay.xul. Rename the ids to se-key-undo/se-key-redo and associate them to se-cmd-undo/se-cmd-redo. Update the scratchpad.xul file accordingly.

(this patch is really about cleaning up code!)
Attachment #602703 - Flags: feedback+
(Reporter)

Comment 8

6 years ago
Mark: for subsequent patch updates please ask for feedback or review from me - set the patch flags accordingly.
(Assignee)

Comment 9

6 years ago
Created attachment 603328 [details] [diff] [review]
Patch (v3)

Here's the next incremental ... in addition to the previous changes, it does the "find" key changes you asked for ...

Note that I had to modify editMenuOverlay.xul to get the correct key mapping for "findagain" ...

Also note that the "key_findAgain2" and "key_findPrevious2" keys could be removed there as they are unused, and we could then go further and remove the "findAgainCmd.key2" entity from editMenuOverlay.dtd, which they are the sole consumers of ...
Attachment #603328 - Flags: feedback?(mihai.sucan)
(Reporter)

Comment 10

6 years ago
Comment on attachment 603328 [details] [diff] [review]
Patch (v3)

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

Thanks for your updated patch Mark! This is great!


(In reply to Mark Capella [:capella] from comment #9)
> Created attachment 603328 [details] [diff] [review]
> Patch (v3)
> 
> Here's the next incremental ... in addition to the previous changes, it does
> the "find" key changes you asked for ...

Awesome!


> Note that I had to modify editMenuOverlay.xul to get the correct key mapping
> for "findagain" ...

That's not needed, because key_findAgain2 has VK_F3 for Macs. Please do not change editMenuOverlay.xul.


> Also note that the "key_findAgain2" and "key_findPrevious2" keys could be
> removed there as they are unused, and we could then go further and remove
> the "findAgainCmd.key2" entity from editMenuOverlay.dtd, which they are the
> sole consumers of ...

They are not associated to other commands, buttons or menu items, but their existence is relevant. Those keyboard shortcuts work. I'm glad you did not remove them in your patch.


Please also address my request in comment 7 related to key_undo/key_redo, then we can land the patch. Thanks a lot!
Attachment #603328 - Flags: feedback?(mihai.sucan) → feedback+
(Assignee)

Comment 11

6 years ago
Ok, I can remove the code from editMenuOverlay.xul ... but on my Win machine that causes "Find Again" to display CTRL-G and map to the CTRL-G key ...

It might be time for me to do a clobber build, as I just also noticed that my versions REDO key displays CTRL-Y but maps to the CTRL-SHIFT-Z key ...
(Reporter)

Comment 12

6 years ago
(In reply to Mark Capella [:capella] from comment #11)
> Ok, I can remove the code from editMenuOverlay.xul ... but on my Win machine
> that causes "Find Again" to display CTRL-G and map to the CTRL-G key ...

Yeah, well it should display the same as in the Edit menu of Firefox. We are now making Scratchpad consistent with the rest of Firefox.

On Linux I get Ctrl-G as well. This is not broken - F3 also works.

> It might be time for me to do a clobber build, as I just also noticed that
> my versions REDO key displays CTRL-Y but maps to the CTRL-SHIFT-Z key ...

Yeah, no worries. Looking forward for the updated patch. Don't forget about comment 7 - the key_undo/key_redo changes! ;)
(Assignee)

Comment 13

6 years ago
Created attachment 603494 [details] [diff] [review]
[in-fx-team] Patch (v4)

Ok ... crosses fingers ... this seems to work :-)
Attachment #602703 - Attachment is obsolete: true
Attachment #603328 - Attachment is obsolete: true
Attachment #603494 - Flags: review?(mihai.sucan)
(Reporter)

Comment 14

6 years ago
Comment on attachment 603494 [details] [diff] [review]
[in-fx-team] Patch (v4)

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

This is looking good Mark! Great work!

Thanks a lot for your patch! I am currently working on bug 731394 and I will land these two patches together when I am also ready.
Attachment #603494 - Flags: review?(mihai.sucan) → review+
(Reporter)

Updated

6 years ago
Blocks: 731394
(Reporter)

Comment 15

6 years ago
Comment on attachment 603494 [details] [diff] [review]
[in-fx-team] Patch (v4)

Landed:
https://hg.mozilla.org/integration/fx-team/rev/82531ecd89f1

Thanks Mark for your contribution!
Attachment #603494 - Attachment description: Patch (v4) → [in-fx-team] Patch (v4)
(Reporter)

Updated

6 years ago
Whiteboard: [good first bug][mentor=msucan][lang=xul] → [good first bug][mentor=msucan][lang=xul][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/82531ecd89f1
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=msucan][lang=xul][fixed-in-fx-team] → [good first bug][mentor=msucan][lang=xul]
Target Milestone: --- → Firefox 13
You need to log in before you can comment on or make changes to this bug.