Closed
Bug 992947
Opened 11 years ago
Closed 10 years ago
Add "Open Link In New Tab" item to stylesheet list
Categories
(DevTools :: Style Editor, defect, P1)
DevTools
Style Editor
Tracking
(firefox40 fixed)
RESOLVED
FIXED
Firefox 40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: bgrins, Assigned: beberveiga, Mentored)
References
(Blocks 1 open bug)
Details
(Whiteboard: [lang=js][polish-backlog][difficulty=easy][bugday-20150710])
Attachments
(1 file, 8 obsolete files)
(deleted),
patch
|
bgrins
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•11 years ago
|
Assignee: nobody → contact
Updated•11 years ago
|
Blocks: firebug-gaps
Assignee | ||
Comment 1•11 years ago
|
||
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.
Attachment #8405109 -
Flags: review?(bgrinstead)
Reporter | ||
Comment 2•11 years ago
|
||
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.
Attachment #8405109 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 3•11 years ago
|
||
Let's see if I got it :)
It seems to be working.
Thank you.
Attachment #8405109 -
Attachment is obsolete: true
Attachment #8405784 -
Flags: review?(bgrinstead)
Reporter | ||
Comment 4•11 years ago
|
||
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 -->
Attachment #8405784 -
Flags: review?(bgrinstead) → feedback?(fayearthur)
Reporter | ||
Comment 5•11 years ago
|
||
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•11 years ago
|
||
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
Attachment #8405784 -
Flags: feedback?(fayearthur) → feedback+
Reporter | ||
Comment 7•11 years ago
|
||
> > + * 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.
Assignee | ||
Comment 8•11 years ago
|
||
This patch does not have tests yet.
Attachment #8405784 -
Attachment is obsolete: true
Attachment #8406512 -
Flags: review?(bgrinstead)
Comment 9•11 years ago
|
||
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");
Assignee | ||
Comment 10•11 years ago
|
||
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.
Attachment #8406512 -
Attachment is obsolete: true
Attachment #8406512 -
Flags: review?(bgrinstead)
Attachment #8414097 -
Flags: review?(bgrinstead)
Comment 11•11 years ago
|
||
(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
Assignee | ||
Comment 12•11 years ago
|
||
Attachment #8414097 -
Attachment is obsolete: true
Attachment #8414097 -
Flags: review?(bgrinstead)
Attachment #8414118 -
Flags: review?(bgrinstead)
Comment 13•11 years ago
|
||
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.
Reporter | ||
Comment 14•11 years ago
|
||
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
Attachment #8414118 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 15•11 years ago
|
||
As Brian pointed out in IRC, we should add a third case for clicking outside of any stylesheet.
Attachment #8414118 -
Attachment is obsolete: true
Attachment #8414138 -
Flags: review?(bgrinstead)
Reporter | ||
Updated•11 years ago
|
Whiteboard: [mentor=bgrins][lang=js]
Reporter | ||
Comment 16•11 years ago
|
||
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.
Attachment #8414138 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 17•11 years ago
|
||
Attachment #8414138 -
Attachment is obsolete: true
Attachment #8414922 -
Flags: review?(bgrinstead)
Reporter | ||
Comment 18•11 years ago
|
||
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
Attachment #8414922 -
Flags: review?(bgrinstead)
Updated•10 years ago
|
Mentor: bgrinstead
Whiteboard: [mentor=bgrins][lang=js] → [lang=js]
Updated•10 years ago
|
Whiteboard: [lang=js] → [lang=js][devedition-40]
Reporter | ||
Updated•10 years ago
|
Whiteboard: [lang=js][devedition-40] → [lang=js][devedition-40][difficulty=easy]
Comment 19•10 years ago
|
||
Assigning P1 priority for some devedition-40 bugs.
Filter on '148DD49E-D162-440B-AE28-E22632FC20AC'
Priority: -- → P1
Reporter | ||
Comment 20•10 years ago
|
||
Went ahead and rebased this and expanded the test coverage. Here's a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=51764229026f
Attachment #8414922 -
Attachment is obsolete: true
Attachment #8591148 -
Flags: review+
Reporter | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Reporter | ||
Comment 21•10 years ago
|
||
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
Attachment #8591148 -
Attachment is obsolete: true
Attachment #8591828 -
Flags: review+
Reporter | ||
Updated•10 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 22•10 years ago
|
||
Keywords: checkin-needed
Whiteboard: [lang=js][devedition-40][difficulty=easy] → [lang=js][devedition-40][difficulty=easy][fixed-in-fx-team]
Comment 23•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [lang=js][devedition-40][difficulty=easy][fixed-in-fx-team] → [lang=js][devedition-40][difficulty=easy]
Target Milestone: --- → Firefox 40
Comment 24•9 years ago
|
||
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
QA Whiteboard: [bugday-20150710]
Whiteboard: [lang=js][devedition-40][difficulty=easy] → [lang=js][devedition-40][difficulty=easy][bugday-20150710]
Comment 25•9 years ago
|
||
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.
Updated•9 years ago
|
Whiteboard: [lang=js][devedition-40][difficulty=easy][bugday-20150710] → [lang=js][polish-backlog][difficulty=easy][bugday-20150710]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•