Add Edit commands #3

nt1m · 2017-08-20T06:31:07Z

No description provided.

ExE-Boss

The following should be moved to the content script file at content/content.js.

Also, the Edit menu is already handled by the content script, it’s just that opening a browser action popup menu defocuses any element in the page, which makes the Edit menu not work.

ExE-Boss · 2017-08-20T15:05:19Z

src/background.js

+	range.selectNodeContents(document.documentElement);
+	selection.removeAllRanges();
+	selection.addRange(range);
+})();`


ExE-Boss · 2017-08-20T15:05:33Z

src/background.js

+		} case "editPaste": {
+			return browser.tabs.executeScript({
+				code: "document.execCommand('paste');"
+			});


ExE-Boss · 2017-08-20T15:05:52Z

src/background.js

+		} case "editCopy": {
+			return browser.tabs.executeScript({
+				code: "document.execCommand('copy');"
+			});


ExE-Boss · 2017-08-20T15:06:02Z

src/background.js

+		} case "editCut": {
+			return browser.tabs.executeScript({
+				code: "document.execCommand('cut');"
+			});


This should be moved to the content script.

ExE-Boss · 2017-08-20T16:08:55Z

src/background.js

+				browser.windows.update(browser.windows.WINDOW_ID_CURRENT, {
+					state: win.state === "fullscreen" ? "normal" : "fullscreen"
+				})
+			);


Also, we use await here instead of Promise.then().

@nt1m

Thanks to @nt1m for figuring out how to implement this using the `browser.windows` API in pull request #3. # TODO: - Make the Fullscreen icon dynamic in the Photon theme.

nt1m · 2017-08-20T19:33:51Z

@ExE-Boss Thanks for reviewing and merging the other PR! I have a working version of edit commands locally that even works with inputs. I just have to fix selectAll to work more reliably, then everything should be fine :)

nt1m · 2017-08-20T19:49:31Z

@ExE-Boss Updated the PR

ExE-Boss

Unfortunately, this still doesn’t work for me (I use the web-ext command for testing).

nt1m · 2017-08-20T20:39:31Z

@ExE-Boss Seems to work on macOS (I also use the web-ext command). Which commands have you tried ?

nt1m · 2017-08-21T07:21:28Z

@ExE-Boss I've updated the Pull request.

Cut works on all inputs.
Copy works everywhere
Paste, Undo, Redo, Delete only work on contentEditable fields (Firefox limitation)
Select all works everywhere

Let me know if you have issues with it.

nt1m · 2017-08-21T07:36:23Z

src/content/content.js

 			} case "editSelectAll": {
-				return document.execCommand("selectAll");
+				// Select all doesn't work reliably on inputs yet


Note to self: This comment needs to updated, select all works properly on inputs.

ExE-Boss

There are still some issues left to address (and updated to commit ad96f85).

ExE-Boss · 2017-08-21T10:02:05Z

src/content/content.js

+				} else if (isFocusable) {
+					result.enable.push("editCut", "editCopy", "editSelectAll");
+				} else {
+					result.enable.push("editCopy", "editSelectAll");


Copy is still broken in this context on Windows. That’s why I implemented commit ad96f85, so that this can be disabled on Windows and enabled on MacOS.

Do note that I only tested this on Stable and Developer Edition, but not on Nightly.

I use Nightly, which may have a different behaviour.

Hmm, actually, it doesn't work for me either in this context.

Actually it does work, it seems like your latest commit breaks this though.

ExE-Boss · 2017-08-21T10:02:28Z

src/popup/popup.xhtml

@@ -44,13 +44,13 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
 				<div class="panel-section">
 					<i class="eb-icon-placeholder"/>
 					<i class="panel-list-item text" data-i18n="popup_edit"/>
-					<div class="panel-list-item icon" data-ipc-message="editCut" data-remain-open="true">
+					<div class="panel-list-item icon" data-ipc-message="editCut">


data-remain-open="true" is needed here to accurately implement how the Application Menu worked in Firefox 4-28 and CTR for FF 29-56.

It doesn't work with data-remain-open=true

ExE-Boss · 2017-08-21T10:02:33Z

src/popup/popup.xhtml

 						<i class="eb-icon-placeholder" data-icon="clipboard-cut"/>
 					</div>
-					<div class="panel-list-item icon" data-ipc-message="editCopy" data-remain-open="true">
+					<div class="panel-list-item icon" data-ipc-message="editCopy">


ExE-Boss · 2017-08-21T10:02:36Z

src/popup/popup.xhtml

 						<i class="eb-icon-placeholder" data-icon="clipboard-copy"/>
 					</div>
-					<div class="panel-list-item icon" data-ipc-message="editPaste" data-remain-open="true">
+					<div class="panel-list-item icon" data-ipc-message="editPaste">


ExE-Boss · 2017-08-21T10:04:57Z

src/background.js

@@ -113,7 +113,6 @@ async function handlePopupMessage(message) {
 			let result = {
 				disable: [
 					"*",
-					"edit*",


This should stay here, because it ensures that the edit menu isn’t enabled when the content script can’t start (i.e. the page is privileged (about:, chrome:// or resource://)).

Yes, I do realize that this is contained in the * selector above, but that one will be removed once the menu is fully implemented, while the edit* selector and any other selector dependant on the content script will be kept.

nt1m · 2017-08-21T16:59:12Z

@ExE-Boss Your latest commit about platform info breaks pretty much all content commands. Platform info is only passed for the init method, so the content script will throw for other commands

ExE-Boss · 2017-08-21T22:22:44Z

@nt1m I have now pushed commit c7f6d5a, which should resolve this.

ExE-Boss suggested changes Aug 20, 2017

View reviewed changes

ExE-Boss reviewed Aug 20, 2017

View reviewed changes

ExE-Boss force-pushed the master branch from df8e7d5 to 28de787 Compare August 20, 2017 16:12

ExE-Boss added a commit that referenced this pull request Aug 20, 2017

Add Fullscreen feature

35f8b15

Thanks to @nt1m for figuring out how to implement this using the `browser.windows` API in pull request #3. # TODO: - Make the Fullscreen icon dynamic in the Photon theme.

nt1m changed the title ~~Add Edit and Fullscreen commands~~ Add Edit commands Aug 20, 2017

ExE-Boss reviewed Aug 20, 2017

View reviewed changes

Add support for Edit commands

3c6b302

nt1m commented Aug 21, 2017

View reviewed changes

ExE-Boss suggested changes Aug 21, 2017

View reviewed changes

ExE-Boss force-pushed the master branch from 08ed201 to 826b8c8 Compare August 23, 2017 23:16

ExE-Boss force-pushed the master branch 2 times, most recently from a26c7b2 to 7efa1d7 Compare September 16, 2017 22:15

ExE-Boss force-pushed the master branch 2 times, most recently from 82a52cb to 6d89fee Compare October 5, 2017 20:16

ExE-Boss force-pushed the master branch from 458cf05 to c1e22e3 Compare May 28, 2018 18:10

ExE-Boss added status: in progress 🐌 Pull requests that are not yet ready to be merged and issues that are being worked on. needs: rebase 🚧 This PR needs to resolve a merge conflict labels May 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Edit commands #3

Add Edit commands #3

nt1m commented Aug 20, 2017

ExE-Boss left a comment

ExE-Boss Aug 20, 2017

ExE-Boss Aug 20, 2017

ExE-Boss Aug 20, 2017

ExE-Boss Aug 20, 2017

ExE-Boss Aug 20, 2017

nt1m commented Aug 20, 2017 •

edited

Loading

nt1m commented Aug 20, 2017

ExE-Boss left a comment

nt1m commented Aug 20, 2017 •

edited

Loading

nt1m commented Aug 21, 2017

nt1m Aug 21, 2017 •

edited

Loading

ExE-Boss left a comment •

edited

Loading

ExE-Boss Aug 21, 2017 •

edited

Loading

nt1m Aug 21, 2017

nt1m Aug 21, 2017 •

edited

Loading

nt1m Aug 21, 2017

ExE-Boss Aug 21, 2017

nt1m Aug 21, 2017

ExE-Boss Aug 21, 2017

ExE-Boss Aug 21, 2017

ExE-Boss Aug 21, 2017

nt1m commented Aug 21, 2017

ExE-Boss commented Aug 21, 2017

Add Edit commands #3

Are you sure you want to change the base?

Add Edit commands #3

Conversation

nt1m commented Aug 20, 2017

ExE-Boss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nt1m commented Aug 20, 2017 • edited Loading

nt1m commented Aug 20, 2017

ExE-Boss left a comment

Choose a reason for hiding this comment

nt1m commented Aug 20, 2017 • edited Loading

nt1m commented Aug 21, 2017

nt1m Aug 21, 2017 • edited Loading

Choose a reason for hiding this comment

ExE-Boss left a comment • edited Loading

Choose a reason for hiding this comment

ExE-Boss Aug 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nt1m Aug 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nt1m commented Aug 21, 2017

ExE-Boss commented Aug 21, 2017

nt1m commented Aug 20, 2017 •

edited

Loading

nt1m commented Aug 20, 2017 •

edited

Loading

nt1m Aug 21, 2017 •

edited

Loading

ExE-Boss left a comment •

edited

Loading

ExE-Boss Aug 21, 2017 •

edited

Loading

nt1m Aug 21, 2017 •

edited

Loading