Closed Bug 730898 Opened 8 years ago Closed 8 years ago

Reuse editMenuKeys in scratchpad.xul

Categories

(DevTools :: Scratchpad, defect, P4, trivial)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 13

People

(Reporter: msucan, Assigned: capella)

References

Details

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

Attachments

(1 file, 3 obsolete files)

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: nobody → markcapella
Status: NEW → ASSIGNED
Attached patch Patch (v1) (obsolete) — Splinter Review
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
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!
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.
(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".
I must be missing something ... I tried (for example) and get runtime errors ...

     <menuitem id="cMenu_cut" key="key_cut"/>
----------------------------------^
Attached patch Patch (v2) (obsolete) — Splinter Review
I spoke too soon and found the problem ...
Attachment #602697 - Attachment is obsolete: true
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+
Mark: for subsequent patch updates please ask for feedback or review from me - set the patch flags accordingly.
Attached patch Patch (v3) (obsolete) — Splinter Review
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)
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+
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 ...
(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! ;)
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)
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+
Blocks: 731394
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)
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
Closed: 8 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
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.