992947 - Add "Open Link In New Tab" item to stylesheet list

Status: RESOLVED FIXED

(DevTools :: Style Editor, defect, P1)

Description

[Brian Grinstead [:bgrins]]

When I'm opening a page sometimes I want to get ahold of the original URL for the style sheets I'm looking at.  When I right click on a stylesheet in the style editor, there should be an option to open it in a new link right next to the 'show original sources' option.

I'd also like this in the style inspector - we can handle this in a different bug, but I know getting this original URL should be shared functionality since it is done in a couple of places.

Comment 1

[Willian Gustavo Veiga]

Attachment: bug-992947-add-open-link-in-new-tab-item-to-stylesheet-list-v1.patch

This is my draft.

It is based on "this.selectedEditor", which is wrong. If you select a stylesheet in the list and right click in another one, the selected one will be opened in a new tab. I don't know how to fix it yet. Any tips?

Inline stylesheets aren't considered.

Comment 2

[Brian Grinstead [:bgrins]]

Comment on attachment 8405109 [details] [diff] [review]
bug-992947-add-open-link-in-new-tab-item-to-stylesheet-list-v1.patch

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

OK, this is looking good besides the selected stylesheet thing.  We have a couple of options here on how to work around not needing to rely on the selected stylesheet.  Here is how I would do it:

First, we will add a capturing event handler on the document for the contextmenu event when the UI is being built (https://mxr.mozilla.org/mozilla-central/source/browser/devtools/styleeditor/StyleEditorUI.jsm#135):

    this._panelDoc.addEventListener("contextmenu", () => {
      this._contextMenuStyleSheet = null;
    }, true);

Next, we will add an event listener for the contextmenu event on the style sheet summary UI (https://mxr.mozilla.org/mozilla-central/source/browser/devtools/styleeditor/StyleEditorUI.jsm#425):

  summary.addEventListener("contextmenu", (event) => {
    this._contextMenuStyleSheet = editor.styleSheet;
  }, false);

Then on the popupshowing event, we have access to this._contextMenuStyleSheet, and we can handle the following three cases:

1) There was a stylesheet clicked on and it is external: show and enable the context menu item
2) There was a stylesheet clicked on and it is inline: show and disable the context menu item
3) There was no stylesheet clicked on (the right click happened below the list): hide the context menu item

::: browser/devtools/styleeditor/StyleEditorUI.jsm
@@ +353,5 @@
>      Services.prefs.setBoolPref(PREF_ORIG_SOURCES, !isEnabled);
>    },
>  
> +  _enableOpenLinkNewTabItemIfNotInlineCss: function() {
> +    this._openLinkNewTabItem.setAttribute('hidden', (!this.selectedEditor.styleSheet.href));

Instead of hiding it here, let's disable it if the click has happened on a stylesheet <li> summary element.  If the click happened outside of any summary element, then go ahead and hide it.

Comment 3

[Willian Gustavo Veiga]

Attachment: bug-992947-add-open-link-in-new-tab-item-to-stylesheet-list-v2.patch

Let's see if I got it :)
It seems to be working.
Thank you.

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8405784 [details] [diff] [review]
bug-992947-add-open-link-in-new-tab-item-to-stylesheet-list-v2.patch

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

Overall, the code works great - I've just left a few small comments.  Can you please add a test to styleeditor/test to test to cover the basic workflow (loading up a page with an external and inline script - the context menu item should be enabled/visible when right clicking the external style, disabled/visible when right clicking the inline style and hidden when right clicking on the main container.  An example of how to right click one of the stylesheets from a test can be seen here: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/styleeditor/test/browser_styleeditor_bug_851132_middle_click.js#50.

Heather, can you take a look at the style editor code and make sure it looks good to you?  Also, I was wondering if we should title the context menu item "Open Style Sheet In New Tab" instead of "Open Link In New Tab" to be more consistent with the tooltip text on the buttons in the style editor.

::: browser/devtools/styleeditor/StyleEditorUI.jsm
@@ +361,5 @@
> +   * This method handles the following cases related to the context menu item "_openLinkNewTabItem":
> +   *
> +   * 1) There was a stylesheet clicked on and it is external: show and enable the context menu item
> +   * 2) There was a stylesheet clicked on and it is inline: show and disable the context menu item
> +   * 3) There was no stylesheet clicked on (the right click happened below the list): hide the context menu 

Nit: Remove trailing whitespace

@@ +364,5 @@
> +   * 2) There was a stylesheet clicked on and it is inline: show and disable the context menu item
> +   * 3) There was no stylesheet clicked on (the right click happened below the list): hide the context menu 
> +   */
> +  _changeStateInOpenLinkNewTabItem: function() {
> +    this._openLinkNewTabItem.setAttribute("hidden", (!this._contextMenuStyleSheet));

Nit: don't need parens around this condition

@@ +365,5 @@
> +   * 3) There was no stylesheet clicked on (the right click happened below the list): hide the context menu 
> +   */
> +  _changeStateInOpenLinkNewTabItem: function() {
> +    this._openLinkNewTabItem.setAttribute("hidden", (!this._contextMenuStyleSheet));
> +    this._openLinkNewTabItem.setAttribute("disabled", (!this._contextMenuStyleSheet.href));

Should check for null here to prevent errors: (this.contextMenuStyleSheet && this.contextMenuStyleSheet.href)

::: browser/locales/en-US/chrome/browser/devtools/styleeditor.dtd
@@ +39,5 @@
>  <!ENTITY noStyleSheet-tip-action.label "append a new style sheet">
>  <!-- LOCALICATION NOTE  (noStyleSheet-tip-end.label): End of the tip sentence -->
>  <!ENTITY noStyleSheet-tip-end.label    "?">
> +
> +<!ENTITY openLinkNewTab.label     "Open link in new tab">

Please add a localization note in a comment above this to explain the purpose of the text for localizers.  Something like:

<!-- LOCALIZATION NOTE (openLinkNewTab.label): This is the text for the
     context menu item that opens a stylesheet in a new tab -->

Comment 5

[Brian Grinstead [:bgrins]]

And speaking of adding a test, we *just* switchted the command for running devtools tests to `mach mochitest-devtools` instead of `mach mochitest-browser` in Bug 984930, so make sure you pull down the latest code and build before adding the test.  I've updated the wiki page with the updated info: https://wiki.mozilla.org/DevTools/Hacking#Running_the_Developer_Tools_Tests.

Comment 6

[Heather Arthur [:harth]]

Comment on attachment 8405784 [details] [diff] [review]
bug-992947-add-open-link-in-new-tab-item-to-stylesheet-list-v2.patch

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

Overall good stuff, thanks Willian.

::: browser/devtools/styleeditor/StyleEditorUI.jsm
@@ +62,5 @@
>    this._onStyleSheetCreated = this._onStyleSheetCreated.bind(this);
>    this._onNewDocument = this._onNewDocument.bind(this);
>    this._clear = this._clear.bind(this);
>    this._onError = this._onError.bind(this);
> +  this._changeStateInOpenLinkNewTabItem = this._changeStateInOpenLinkNewTabItem.bind(this);

This function name is really long, and I don't think it has to be. I think something like `_updateOpenLinkItem` is unambiguous enough. Something shorter would be great. We usually break lines that are over 80 chars.

@@ +364,5 @@
> +   * 2) There was a stylesheet clicked on and it is inline: show and disable the context menu item
> +   * 3) There was no stylesheet clicked on (the right click happened below the list): hide the context menu 
> +   */
> +  _changeStateInOpenLinkNewTabItem: function() {
> +    this._openLinkNewTabItem.setAttribute("hidden", (!this._contextMenuStyleSheet));

The value of `document.popupNode` is the element that was right-clicked. So you should just be able to use `this._panelDoc.popupNode` instead of keeping track of a `this._contextMenuStyleSheet`. See https://developer.mozilla.org/docs/Web/API/document.popupNode

Comment 7

[Brian Grinstead [:bgrins]]

> > +   * 2) There was a stylesheet clicked on and it is inline: show and disable the context menu item
> > +   * 3) There was no stylesheet clicked on (the right click happened below the list): hide the context menu 
> > +   */
> > +  _changeStateInOpenLinkNewTabItem: function() {
> > +    this._openLinkNewTabItem.setAttribute("hidden", (!this._contextMenuStyleSheet));
> 
> The value of `document.popupNode` is the element that was right-clicked. So
> you should just be able to use `this._panelDoc.popupNode` instead of keeping
> track of a `this._contextMenuStyleSheet`. See
> https://developer.mozilla.org/docs/Web/API/document.popupNode

The concern I had with finding the node that was clicked was that it could be any of the children inside of the li (the eyeball, either of the labels) or completely outside of the list so we would have to do the work to find the container (if any), followed by some work to get ahold of the stylesheet object that the node is based on.  That's why I suggested keeping track of which sheet is selected on the contextmenu event.  If there was an easy way to handle it that I wasn't thinking of using the popupNode, that would be a couple less events to listen to.

Comment 8

[Willian Gustavo Veiga]

Attachment: bug-992947-add-open-link-in-new-tab-item-to-stylesheet-list-v3.patch

This patch does not have tests yet.

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 8406512 [details] [diff] [review]
bug-992947-add-open-link-in-new-tab-item-to-stylesheet-list-v3.patch

>+    this._window.openNewTabWith(this._contextMenuStyleSheet.href);

Use openLinkIn instead of openNewTabWith:

this._window.openLinkIn(this._contextMenuStyleSheet.href, "tab");

Comment 10

[Willian Gustavo Veiga]

Attachment: bug-992947-add-open-link-in-new-tab-item-to-stylesheet-list-v4.patch

dao, would you mind explaining why should I change it? As a beginner, I'd like to learn as much as I can. :)

My first attempt to write tests.

Thank you very much.

Comment 11

[Dão Gottwald [:dao]]

(In reply to Willian Gustavo Veiga from comment #10)
> Created attachment 8414097 [details] [diff] [review]
> bug-992947-add-open-link-in-new-tab-item-to-stylesheet-list-v4.patch
> 
> dao, would you mind explaining why should I change it? As a beginner, I'd
> like to learn as much as I can. :)

openNewTabWith is an old and crufty API that we only continue to expose for backwards-compatibility. If you look at its source, you will see that it only differs from openLinkIn in that it passes the current document's character set to the new tab, which I don't think you want here:

http://hg.mozilla.org/mozilla-central/annotate/b681a6daea3b/browser/base/content/utilityOverlay.js#l622

Comment 12

[Willian Gustavo Veiga]

Attachment: bug-992947-add-open-link-in-new-tab-item-to-stylesheet-list-v5.patch

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 8414118 [details] [diff] [review]
bug-992947-add-open-link-in-new-tab-item-to-stylesheet-list-v5.patch

>+    this._window.openLinkIn(this._contextMenuStyleSheet.href, "tab", {});

Hmm, openLinkIn shouldn't require you to pass in an empty object as the third argument if you don't want to set any parameters. You can avoid this inconvenience by calling openUILinkIn instead of openLinkIn; the former is more suitable here anyway.

Comment 14

[Brian Grinstead [:bgrins]]

Comment on attachment 8414118 [details] [diff] [review]
bug-992947-add-open-link-in-new-tab-item-to-stylesheet-list-v5.patch

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

We're talking about the necessary test changes in IRC, one more note on the code changes below:

::: browser/devtools/styleeditor/StyleEditorUI.jsm
@@ +377,5 @@
> +  /**
> +   * Open a particular stylesheet in a new tab.
> +   */
> +  _openLinkNewTab: function() {
> +    this._window.openLinkIn(this._contextMenuStyleSheet.href, "tab", {});

Check `if (this._contextMenuStyleSheet)` before running this line

Comment 15

[Willian Gustavo Veiga]

Attachment: bug-992947-add-open-link-in-new-tab-item-to-stylesheet-list-v6.patch

As Brian pointed out in IRC, we should add a third case for clicking outside of any stylesheet.

Comment 16

[Brian Grinstead [:bgrins]]

Comment on attachment 8414138 [details] [diff] [review]
bug-992947-add-open-link-in-new-tab-item-to-stylesheet-list-v6.patch

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

The test looks good, but as mentioned let's add another case to test for right clicking outside of all of the sheets.  I'm also thinking about if there would be a good way to trigger the click on the context menu item, then to listen for the new tab opening / checking the URL of it.

Comment 17

[Willian Gustavo Veiga]

Attachment: bug-992947-add-open-link-in-new-tab-item-to-stylesheet-list-v7.patch

Comment 18

[Brian Grinstead [:bgrins]]

Comment on attachment 8414922 [details] [diff] [review]
bug-992947-add-open-link-in-new-tab-item-to-stylesheet-list-v7.patch

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

We've discussed a way to test that the tab gets opened with the correct URL on irc

::: browser/devtools/styleeditor/test/browser_styleeditor_opentab.js
@@ +46,5 @@
> +  let defer = promise.defer();
> +
> +  onPopupShow(gUI._contextMenu).then(()=> {
> +    onPopupHide(gUI._contextMenu).then(() => {
> +      is(gUI._openLinkNewTabItem.getAttribute("disabled"), "false", "The menu item is not disabled");

These assertions should happen in the onPopupShow callback - only resolve the deferred inside of onPopupHide

Comment 19

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Assigning P1 priority for some devedition-40 bugs. 

Filter on '148DD49E-D162-440B-AE28-E22632FC20AC'

Comment 20

[Brian Grinstead [:bgrins]]

Attachment: styleeditor-open-link.patch

Went ahead and rebased this and expanded the test coverage.  Here's a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=51764229026f

Comment 21

[Brian Grinstead [:bgrins]]

Attachment: styleeditor-open-link.patch

Simplified the way the test checks for opened tabs by just overriding the openUILinkIn function to do an assertion instead of all the hoops to wait for the tab to actually be opened.

Here's a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f39829bc499

Comment 22

[Brian Grinstead [:bgrins]]

remote:   https://hg.mozilla.org/integration/fx-team/rev/c25c307b5fd3

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c25c307b5fd3

Comment 24

[[:Towkir] Ahmed]

On Nightly 31.0a1, (details bellow)

User Agent 	Mozilla/5.0 (Windows NT 6.1; rv:31.0) Gecko/20100101 Firefox/31.0
Build ID	20140407030203

This contains an option "Show CSS Sources" in Styel Editor , CSS list. But the latest beta (40.0) has this been replaced with "Open Link in New Tab". Details of fixed version is:

User Agent 	Mozilla/5.0 (Windows NT 6.1; rv:40.0) Gecko/20100101 Firefox/40.0
Build ID 	20150709163524

Comment 25

[[:Towkir] Ahmed]

Checked on Developer Edition 41.0a2 too
User Agent 	Mozilla/5.0 (Windows NT 6.1; rv:41.0) Gecko/20100101 Firefox/41.0
Build ID 	20150710004007

Sorry that I did not notice the whiteboard saying about DevEdition, I noticed the tracking flag only.