Cannot disable style editor like the other tools

VERIFIED FIXED in Firefox 50

Status

DevTools
Framework
VERIFIED FIXED
3 years ago
a month ago

People

(Reporter: fitzgen, Assigned: Maxime Buquet, Mentored)

Tracking

unspecified
Firefox 50

Firefox Tracking Flags

(firefox50 verified)

Details

(Whiteboard: [good next bug][lang=js])

Attachments

(1 attachment, 6 obsolete attachments)

STR:

* Go to options

ER:

* Checkbox to disable style editor

AR:

* No such checkbox

-----------------------------

I never use the style editor, its just taking up space. Let me get rid of it.
+1
Ideally, I think we should be able to do this with any tool (see bug 1171939).
Tom: with the inspector <-> styleeditor interaction (due to as-authored), are there anything we should be particularly careful about when making it possible to disable the style-editor?
See Also: → bug 1171939
^
Flags: needinfo?(ttromey)

Comment 3

3 years ago
The new interactions all go via the actors, so there is no worry there.

However, the style inspector shows clickable links that take one to the
appropriate spot in the style sheet.  If the style editor is completely disabled,
I guess those should not be clickable?
Flags: needinfo?(ttromey)
(In reply to Tom Tromey :tromey from comment #3)
> The new interactions all go via the actors, so there is no worry there.
> 
> However, the style inspector shows clickable links that take one to the
> appropriate spot in the style sheet.  If the style editor is completely
> disabled,
> I guess those should not be clickable?
Good point. Here's how the link become clickable:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/styleinspector/rule-view.js#2704
And here's how the click is handled (the style-editor is open):
https://dxr.mozilla.org/mozilla-central/source/devtools/client/styleinspector/style-inspector.js#133
Mentor: pbrosset
Whiteboard: [good next bug][lang=js]

Updated

3 years ago
Assignee: nobody → kevin.m.wern
Component: Developer Tools → Developer Tools: Framework
(Assignee)

Comment 5

3 years ago
Created attachment 8713935 [details] [diff] [review]
bug_1221545.patch

It has been a while since Kevin assigned himself, so if he's ok with it, I would like to be assigned to this bug.

I have a patch, thanks to bgrins and jdescottes on IRC.
Attachment #8713935 - Flags: review?(bgrinstead)

Comment 6

3 years ago
Hey, sorry, I ended up getting caught up with other stuff. Glad you were able to work on it.
Assignee: kevin.m.wern → pep
Comment on attachment 8713935 [details] [diff] [review]
bug_1221545.patch

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

Thanks for working on this, looks on the good track!

I don't mean to steal the review from Brian, but I tested the patch and thought I would try to give some feedback. 
Hope that's fine :)

::: devtools/client/inspector/rules/test/browser_rules_style-editor-link.js
@@ +144,5 @@
> +  yield toolbox.selectTool("inspector");
> +
> +  info("Disabling style editor");
> +  Services.prefs.setBoolPref("devtools.styleeditor.enabled", false);
> +  gDevTools.emit('tool-unregistered', 'styleeditor');

Strings in devtools are using double quotes.  

Overall, you can have a look at https://wiki.mozilla.org/DevTools/CodingStandards 

We use ESLint for improving our coding standards. So the ESLint rules should be applied for all new code. Many files in our codebase are still not compliant with all the rules, but new code should be :)

@@ +188,5 @@
> +function testUnselectableRuleViewLink(view, index) {
> +  let link = getRuleViewLinkByIndex(view, index);
> +  let unselectable = link.hasAttribute("unselectable");
> +
> +  is(unselectable, true, "rule view is unselectable");

fyi: you can also use ok(unselectable, "rule view is unselectable");

::: devtools/client/inspector/rules/views/rule-editor.js
@@ +72,5 @@
>  RuleEditor.prototype = {
>    destroy: function() {
>      this.rule.domRule.off("location-changed");
> +    gDevTools.off("tool-registered");
> +    gDevTools.off("tool-unregistered");

You should provide the event handler, otherwise all listeners to "tool-registered" and "tool-unregistered" will be unregistered.

gDevtools.off("tool-registered", this._toolRegistrationChanged)

@@ +104,4 @@
>      // Add the source link.
> +    this.source = createChild(this.element, "div", Object.assign(
> +      {class: "ruleview-rule-source theme-link"},
> +      !selectable && {unselectable: true}

Could be easier to update this in updateSourceLink, where we set "unselectable" under some conditions. You could add a new condition linked to the style editor ?

if (!this._isStyleEditorEnabled()) {
  this.source.setAttribute("unselectable", "");
}

@@ +245,5 @@
> +  },
> +
> +  _toolRegistrationChanged: function (event, toolId) {
> +    if ((typeof toolId !== 'string' && toolId.id !== 'styleeditor') &&
> +        toolId !== 'styleeditor') {

typeof toolId !== 'string' is redundant with toolId !== 'styleeditor'

Alternatively, we could also not care about the tool being selected, and simply update the source link (not too worried about performance here).

@@ +249,5 @@
> +        toolId !== 'styleeditor') {
> +      return;
> +    }
> +
> +    let source = this.element.querySelector(".ruleview-rule-source");

You could reuse this.source here.
(Assignee)

Comment 8

3 years ago
Created attachment 8714199 [details] [diff] [review]
bug_1221545.patch

Thank you for the review!

I applied the changes. Sorry for the linting step, I knew about it but totally forgot.

> Alternatively, we could also not care about the tool being selected, and simply update the source
> link (not too worried about performance here).

Ok, I remember bgrins saying that we might move the event handler on an upper component depending on the perfs, that's why I wrote this. I removed it for now.

Tell me if you see anything else.
Attachment #8713935 - Attachment is obsolete: true
Attachment #8713935 - Flags: review?(bgrinstead)
Attachment #8714199 - Flags: review?(bgrinstead)
Comment on attachment 8714199 [details] [diff] [review]
bug_1221545.patch

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

Looks good to me overall.  Please see notes inline.  Specifically I'll attach another patch that gives the APIs you need from the toolbox, although some of the comments could be worked on in the mean time.

::: devtools/client/inspector/rules/test/browser_rules_style-editor-link.js
@@ +159,5 @@
> +
> +  info("Clicking on a link");
> +  clickLinkByIndex(view, 1);
> +
> +  Services.prefs.setBoolPref("devtools.styleeditor.enabled", gToolsPref);

Instead of storing gToolsPref and re-setting here you could do Services.prefs.clearUserPref("devtools.styleeditor.enabled")

::: devtools/client/inspector/rules/views/rule-editor.js
@@ +64,2 @@
>  
>    this.rule.domRule.on("location-changed", this._locationChanged);

I'd like to move the handlers for gDevTools at least up to the ruleView level, if not the toolbox level.  This is because gDevTools exists for the entire process and if for some reason an editor doesn't get properly destroyed it'd be worse to leak on that module than on the toolbox / rule view.  So it will look something like:

  this.toolbox = ruleView.inspector.toolbox;
  this.toolbox.on("tool-registered", this.updateSourceLink);
  this.toolbox.on("tool-unregistered", this.updateSourceLink);

@@ +244,5 @@
> +  _isStyleEditorEnabled: function() {
> +    return !!gDevTools.getToolDefinition("styleeditor");
> +  },
> +
> +  _toolRegistrationChanged: function() {

I'd prefer to see this logic moved into updateSourceLink and have this function go away.  Then have the handler for tool registration / unregistration call this.updateSourceLink directly.  This would limit the amount of places we have rendering logic and even though it'd be doing more work than strictly necessary this should only happen when the tool is added/removed which won't happen very much.

Going off of the comment above, I think I'm going to add toolbox APIs for this

    if (this.toolbox.isToolRegistered("styleeditor")) {
      this.source.removeAttribute("unselectable");
    } else {
      this.source.setAttribute("unselectable", "true");
    }


Note: you'd also need to do `this.updateSourceLink = this.updateSourceLink.bind(this);` up in the constructor

@@ +248,5 @@
> +  _toolRegistrationChanged: function() {
> +    if (this._isStyleEditorEnabled()) {
> +      this.source.removeAttribute("unselectable");
> +    } else {
> +      this.source.setAttribute("unselectable", "true");

It'd be really cool if instead this opened a view-source page to the relevant CSS.  That's follow-up material though that could be done in another bug
Attachment #8714199 - Flags: review?(bgrinstead) → feedback+
Status: NEW → ASSIGNED
Depends on: 1245287
Created attachment 8715027 [details]
MozReview Request: Bug 1221545 - Add toolbox API for isToolRegistered and events for tool-registered and tool-unregistered;r=jryans

Review commit: https://reviewboard.mozilla.org/r/33307/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33307/
Attachment #8715027 - Flags: review?(jryans)
Comment on attachment 8715027 [details]
MozReview Request: Bug 1221545 - Add toolbox API for isToolRegistered and events for tool-registered and tool-unregistered;r=jryans

Sorry, wrong bug # in the commit message for this patch.  Moved to Bug 1245287
Attachment #8715027 - Attachment is obsolete: true
Attachment #8715027 - Flags: review?(jryans)
(Assignee)

Comment 12

2 years ago
Created attachment 8762764 [details] [diff] [review]
bug_1221545.patch

Sorry for the 4 months gap. I added in the feedback.

> It'd be really cool if instead this opened a view-source page to the relevant
> CSS.  That's follow-up material though that could be done in another bug

That would be a nice idea indeed.
Attachment #8714199 - Attachment is obsolete: true
Attachment #8762764 - Flags: review?(bgrinstead)
Comment on attachment 8762764 [details] [diff] [review]
bug_1221545.patch

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

Forwarding review to Julian
Attachment #8762764 - Flags: review?(bgrinstead) → review?(jdescottes)
Comment on attachment 8762764 [details] [diff] [review]
bug_1221545.patch

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

Thanks for the patch Maxime! 

In the good direction, but I'd like to see a new patch with my comments addressed.
By the way, your patch is missing a commit message, could you add one?

::: devtools/client/inspector/rules/test/browser_rules_style-editor-link.js
@@ +168,5 @@
> +  Services.prefs.setBoolPref("devtools.styleeditor.enabled", true);
> +  gDevTools.emit("tool-registered", "styleeditor");
> +
> +  info("Clicking on a link");
> +  clickLinkByIndex(view, 1);

After clicking on the link we should check that the style editor gets selected.

You can yield on toolbox.once("styleeditor-selected"); to wait for the style editor to be selected. 

After that we can check that the currentToolId is styleeditor, as expected.

@@ +189,5 @@
> +function testUnselectableRuleViewLink(view, index) {
> +  let link = getRuleViewLinkByIndex(view, index);
> +  let unselectable = link.hasAttribute("unselectable");
> +
> +  ok(unselectable, "rule view is unselectable");

nit: uppercase first letter of sentence -> "Rule view is unselectable"

::: devtools/client/inspector/rules/views/rule-editor.js
@@ +89,4 @@
>    },
>  
>    get isSelectorEditable() {
>      let toolbox = this.ruleView.inspector.toolbox;

We should reuse this.toolbox here

@@ +250,5 @@
>        }
>      }
>  
> +    if (this.toolbox.isToolRegistered("styleeditor")) {
> +      this.source.removeAttribute("unselectable");

In this method we have this.source === sourceLabel.parentNode. Please use this.source everywhere, makes the code easier to understand.

This implementation also creates a regression: if the "unselectable" attribute is set to true in the previous if() statement, "unselectable" will now be removed if the styleeditor is enabled.

You could move this code before the previous if() statement, or compute an "unselectable" boolean in order to perform the remove/setAttribute only once.
Attachment #8762764 - Flags: review?(jdescottes) → feedback+
(Assignee)

Comment 15

2 years ago
Created attachment 8763853 [details] [diff] [review]
bug_1221545.patch

I updated the patch with your comments! Please tell me if there is anything else I need to update.
Attachment #8763853 - Flags: review?(jdescottes)
Attachment #8762764 - Attachment is obsolete: true
Comment on attachment 8763853 [details] [diff] [review]
bug_1221545.patch

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

Thanks, looks good to me! Just one minor comment in the test, but no need for another review round.

Regarding the commit message, I am sorry, I linked you to the wrong documentation ...
The actual format to use here is "Bug 12345 - Summary of bug. r=<name of reviewer>" for the first line.

Your current patch is probably based on an old revision, and it doesn't apply directly on top of the fx-team branch.
Can you rebase your patch? (If you prefer, I can also amend your patch, let me know!)

In the meantime I have submitted your patch to our test platform : https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3e698dfb266.
This will run the devtools test suites on various machines & platform, in order to check if nothing broke because of the patch.

::: devtools/client/inspector/rules/test/browser_rules_style-editor-link.js
@@ +169,5 @@
> +  gDevTools.emit("tool-registered", "styleeditor");
> +
> +  info("Clicking on a link");
> +  clickLinkByIndex(view, 1);
> +  yield toolbox.once("styleeditor-selected");

Would be safer to attach the event before clicking on the link here:

let onStyleEditorSelected = toolbox.once("styleeditor-selected");
clickLinkByIndex(view, 1);
yield onStyleEditorSelected;
Attachment #8763853 - Flags: review?(jdescottes) → review+
(Assignee)

Comment 17

2 years ago
> Your current patch is probably based on an old revision, and it doesn't
> apply directly on top of the fx-team branch.
> Can you rebase your patch? (If you prefer, I can also amend your patch, let
> me know!)

That is certainly because I am based off mozilla-central ^^'
It's gonna take me a while to clone fx-team, so I'd appreciate if you could amend the patch for me! (And maybe modify the test case at the same time)

> In the meantime I have submitted your patch to our test platform :
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3e698dfb266.
> This will run the devtools test suites on various machines & platform, in
> order to check if nothing broke because of the patch.

Thanks!
Created attachment 8763904 [details] [diff] [review]
bug1221545.rebased.patch

Rebased and updated the test. Carry over r+
Attachment #8763853 - Attachment is obsolete: true
Attachment #8763904 - Flags: review+
Created attachment 8763906 [details] [diff] [review]
bug1221545.v2.patch

My bad, I missed a few changes when rebasing. New try push : https://treeherder.mozilla.org/#/jobs?repo=try&revision=fcde296ca337
Attachment #8763904 - Attachment is obsolete: true
Attachment #8763906 - Flags: review+
The tests are now finished. There was a test failure (browser_se_first-run.js on OS X 10.10 opt) but it is a known intermittent test and is not related to your change.

The next step here is to land your changeset on fx-team. To do so we add the keyword checkin-needed to this bug. A sheriff will then pick up your patch and push it to fx-team, which will later be merged to mozilla central if there is no issue. After that, your patch will be available in Nightly and will ride the release train of Firefox 50.

Thanks a lot for your help here Maxime!
Keywords: checkin-needed

Comment 21

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/55531d6fbd7d
Allow for the Style Editor to be disabled. r=jdescottes
Keywords: checkin-needed

Comment 22

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/55531d6fbd7d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50

Comment 23

2 years ago
I have reproduced this bug with Nightly 45.0a1 (2015-11-04) on Windows 8.1 , 64 bit!

This bug's fix is verified on Latest Aurora 50.0a2 .

Build ID : 20160810004000
User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0

[bugday-20160810]

Comment 24

2 years ago
The Bug was about to Add Checkbox to disable style editor like the other tools . I have seen the implementation on Latest Aurora with ubuntu 16.04 (64 bit).

This Bug is now verified as fixed on Latest Aurora 50.0a2
Build ID: 20160810004000
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0
QA Whiteboard: [bugday-20160810]
I have successfully reproduced this issue on Nightly according to(2015-09-18)

Fixing bug is verified on Latest Developer Edition--  Build ID:( 20160826004001 ), User Agent: Mozilla/5.0 (Windows NT 10.0; rv:50.0) Gecko/20100101 Firefox/50.0

Tested OS -- Windows10 32bit
QA Whiteboard: [bugday-20160810] → [testday-20160826]
Comment hidden (obsolete)

Updated

2 years ago
Status: RESOLVED → VERIFIED
status-firefox50: fixed → verified

Updated

a month ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.