Closed Bug 1221545 Opened 9 years ago Closed 8 years ago

Cannot disable style editor like the other tools

Categories

(DevTools :: Framework, defect)

defect
Not set
normal

Tracking

(firefox50 verified)

VERIFIED FIXED
Firefox 50
Tracking Status
firefox50 --- verified

People

(Reporter: fitzgen, Assigned: pep, Mentored)

References

Details

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

Attachments

(1 file, 6 obsolete files)

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: → 1171939
^
Flags: needinfo?(ttromey)
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]
Assignee: nobody → kevin.m.wern
Component: Developer Tools → Developer Tools: Framework
Attached patch bug_1221545.patch (obsolete) — Splinter Review
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)
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.
Attached patch bug_1221545.patch (obsolete) — Splinter Review
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
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)
Attached patch bug_1221545.patch (obsolete) — Splinter Review
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+
Attached patch bug_1221545.patch (obsolete) — Splinter Review
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+
> 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!
Attached patch bug1221545.rebased.patch (obsolete) — Splinter Review
Rebased and updated the test. Carry over r+
Attachment #8763853 - Attachment is obsolete: true
Attachment #8763904 - Flags: review+
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
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
https://hg.mozilla.org/mozilla-central/rev/55531d6fbd7d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
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]
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]
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.