Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use the built-in keyboard commands system #73

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+64 −286
Open

Use the built-in keyboard commands system #73

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion webex/background.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ function assertNoResponse(response) {
}

function notifyError(error) {
browser.notifications.create({
browser.notifications?.create({
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like an unrelated change. Can you either remove it or add it as a separate commit with a commit message explaining it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When testing, and constantly reloading the add-on from the dev directory, the extension console did not work flawlessly. I added optional chaining in several places when some things did not appear to be working in the debugger.

type: "basic",
title: "Textern",
message: "Error: " + error + "."
Expand Down Expand Up @@ -126,3 +126,16 @@ function onMessage(message, sender, respond) {
}

browser.runtime.onMessage.addListener(onMessage);

browser.commands.onCommand.addListener(function(command) {
if (command === 'textern') {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I wonder if the shortcut name should be something more specific, in case we add others in the future. Though if I understand correctly the API docs, we should be free to change this identifier in a future version, so maybe that's OK.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these names are internally namespaced to each extension to avoid such collisions. If you want to make it more reflective of the action, that's fine.

browser.tabs.query({
currentWindow: true,
active: true
}).then(function(tabs) {
if (tabs?.length && tabs[0].id != browser.tabs.TAB_ID_NONE) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the ? here? From the docs, it should never be undefined.

browser.tabs.sendMessage(tabs[0].id, {type: 'shortcut'}).then(assertNoResponse, logError);
}
});
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not logError here?

}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we log to the console here if we received any other commands we don't know?

});
59 changes: 26 additions & 33 deletions webex/content.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ function assertNoResponse(response) {
}

function notifyError(error) {
browser.notifications.create({
browser.notifications?.create({
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here too.

type: "basic",
title: "Textern",
message: "Error: " + error + "."
Expand Down Expand Up @@ -91,8 +91,7 @@ function slackSetText(e, text) {
}
}

function registerText(event) {
var e = event.target;
function registerText(e) {
if (e.nodeName == "TEXTAREA") {
var id = watchElement(e);
/* don't use href directly to not bring in e.g. url params */
Expand Down Expand Up @@ -171,43 +170,37 @@ function setText(id, text) {
fadeBackground(e);
}

function getActiveElement(element = document.activeElement) {
if (element.nodeName === 'IFRAME' || element.nodeName === 'FRAME')
return null; // skip to allow child documents to handle shortcut
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using all_frames and handling it like this here, would it be more elegant to recurse into it in this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quite possibly, I don't recall if I tried that approach.


const shadowRoot = element.shadowRoot
const contentDocument = element.contentDocument

if (shadowRoot && shadowRoot.activeElement) {
return getActiveElement(shadowRoot.activeElement)
}
Comment on lines +180 to +182
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, didn't know about shadow roots before. Do you have an example page where a textarea on which you'd want to use Textern is actually nested under a shadow root?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it was only to ensure we attach to the right element. I read this answer on SO, and didn't want neglect pages that used more complex DOMs.


if (contentDocument && contentDocument.activeElement) {
return getActiveElement(contentDocument.activeElement)
}

return element
}

function onMessage(message, sender, respond) {
if (sender.id != "textern@jlebon.com")
return;
if (message.type == "set_text")
if (message.type == 'shortcut') {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's call this message register?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's already effectively namespaced by sender.id, so you can call it whatever. I think register is a bit generic. In the context of code, I think of "registering" as attaching a listener, or in some way saying "I want to know about this event", not the occurrence of the relevant event.

const activeElement = getActiveElement();
if (activeElement)
registerText(activeElement);
}
else if (message.type == "set_text")
setText(message.id, message.text);
else {
console.log(`Unknown message type: ${message.type}`);
}
}

browser.runtime.onMessage.addListener(onMessage);

var currentShortcut = undefined;
function registerShortcut(force) {
browser.storage.local.get({shortcut: "Ctrl+Shift+D"}).then(val => {
if ((val.shortcut == currentShortcut) && !force)
return; /* no change */
if (currentShortcut != undefined)
shortcut.remove(currentShortcut);
currentShortcut = val.shortcut;
shortcut.add(currentShortcut, registerText);
});
}

registerShortcut(true);

/* meh, we just re-apply the shortcut -- XXX: should check what actually changed */
browser.storage.onChanged.addListener(function(changes, areaName) {
registerShortcut(false);
});

/* we also want to make sure we re-register whenever the number of iframes changes */
var lastNumFrames = window.frames.length;
const observer = new MutationObserver(function() {
if (window.frames.length != lastNumFrames) {
registerShortcut(true);
lastNumFrames = window.frames.length;
}
});
observer.observe(document, {childList: true, subtree: true});
29 changes: 23 additions & 6 deletions webex/manifest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"description": "Edit text in your favourite external editor!",
"homepage_url": "https://github.com/jlebon/textern",
"manifest_version": 2,
"version": "0.8",
"version": "0.9b1",

"browser_specific_settings": {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was updating the manifest to use the shortcut APIs, it made sense to me to use the recommended keys for the whole document.

"gecko": {
Expand All @@ -17,15 +17,32 @@
},

"content_scripts": [
{
"matches": ["<all_urls>"],
"js": ["shortcut.js", "content.js"]
}
{
"matches": ["<all_urls>"],
"js": [
"content.js"
],
"all_frames": true
}
],

"commands": {
"textern": {
"suggested_key": {
"default": "Shift+Alt+D"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about "Ctrl+Shift+F"? Looks like that's not taken by Firefox already, and is close to the current default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wary of any specifying any shortcut with a Ctrl in it. They are regularly commandeered by both applications, and software running at the desktop level. There aren't many spare Ctrl + Shift + Letter keys available to begin with, so you're practically guaranteed to annoy some of your users.

},
"description": "Edit externally"
}
},

"options_ui": {
"page": "options.html"
},

"permissions": ["notifications", "nativeMessaging", "storage"]
"permissions": [
"tabs",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we reduce this permission to https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/permissions#activetab_permission only?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're welcome to try it. the tabs permission is required here, as far as I know. but If you can make it work with fewer perms, that's great.

"notifications",
"nativeMessaging",
"storage"
]
}
3 changes: 1 addition & 2 deletions webex/options.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,14 @@
<input type="text" id="editor" required=true
pattern='\[ *"[^"]*"(, *"[^"]*")* *\]'
title='Must be in the form: ["executable", "arg", "arg", ...]'>
<label for="shortcut">Shortcut</label>
<input type="text" id="shortcut" required=true>
<label for="extension">Document extension</label>
<input type="text" id="extension" required=true pattern="[.a-zA-Z0-9_-]+"
title='Must be an alphanumerical string (excluding leading dot)'>
<label for="backupdir">Backup files to directory (experimental)</label>
<input type="text" id="backupdir">
<button type="submit">Save</button><span id="saved"></span>
</form>
<p>Shortcut can be configured on the Manage Extension Shortcuts settings page.</p>
<script src="options.js" charset="utf-8"></script>
</body>
</html>
5 changes: 0 additions & 5 deletions webex/options.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ function saveOptions(e) {
e.preventDefault();
browser.storage.local.set({
editor: document.querySelector("#editor").value,
shortcut: document.querySelector("#shortcut").value,
extension: document.querySelector("#extension").value,
backupdir: document.querySelector("#backupdir").value
});
Expand All @@ -32,10 +31,6 @@ function restoreOptions() {
result.editor || "[\"gedit\", \"+%l:%c\"]";
}, onError);

browser.storage.local.get("shortcut").then(result => {
document.querySelector("#shortcut").value = result.shortcut || "Ctrl+Shift+D";
}, onError);

browser.storage.local.get("extension").then(result => {
document.querySelector("#extension").value = result.extension || "txt";
}, onError);
Expand Down
Loading