1136101 - Add an 'Add rule' toolbar button in the rules view

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P1)

Comment 1

[Tim Nguyen :ntim]

Attachment: Patch

This works on top of :gl's patch in bug 1120616.

As for the automated testing, I changed the third Add rule test file to use the toolbar button, the 2 other files still use the context menu.

Comment 2

[Gabriel [:gl] (ΦωΦ)]

I had shorlander evaluate adding the rule toolbar button along with bug 987365. You might want to ask him to comment on how the 2 should interact together.

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to Gabriel Luong (:gl) from comment #2)
> I had shorlander evaluate adding the rule toolbar button along with bug
> 987365. You might want to ask him to comment on how the 2 should interact
> together.

I'd be interested in his opinion on this.  Has he commented about it on any bugs, or is he still looking into it?

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8594022 [details] [diff] [review]
Patch

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

The code looks generally good, but we should get some UX direction here (and/or in Bug 987365)

::: browser/devtools/styleinspector/test/browser_ruleview_add-rule_03.js
@@ -42,5 @@
>    info("Waiting for rule view to change");
>    let onRuleViewChanged = once(view, "ruleview-changed");
>  
>    info("Adding the new rule");
> -  view.menuitemAddRule.click();

I'd like if we can keep the context menu and add the new button in the test (instead of just replacing context menu with the button).

Comment 5

[Tim Nguyen :ntim]

Comment on attachment 8594022 [details] [diff] [review]
Patch

Stephen, this patch adds a toolbar button to the rule view that allows you to add a new rule. Bug 987365 also adds a toolbar button to the rule view. How should those buttons interact with each other ? Should the "Add rule" button add a rule for the currently emulated pseudo class ? Where should the "Add rule" button be located ?

It's worth noting that chrome just puts both buttons next to each other.

Comment 6

[Tim Nguyen :ntim]

Attachment: Screenshot

Here's a screenshot to ease up your review. There's also a try push in the previous comment if you'd like to try the feature.

Comment 7

[Tim Nguyen :ntim]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b89ecf3c765

Comment 8

[Stephen Horlander [:shorlander]]

Comment on attachment 8594022 [details] [diff] [review]
Patch

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

>> How should those buttons interact with each other?
Placing them next to each other is fine: http://cl.ly/image/1v3a0D0R3T2N

>> Should the "Add rule" button add a rule for the currently emulated pseudo class?
Yes, I think it makes sense to add the emulated state to the rule. It eliminates the added step of typing in the pseudo-classes and mirrors what you are actually looking at.

>> Where should the "Add rule" button be located?
I think it makes sense next to the Filter Field. Making that bar conceptually the “Manipulate Rules” bar.

It might be more obvious to have an item at the top of the list that says “Add New Rule”:

e.g.

+------------------+
| Filter Styles    |
+------------------+
| + Add New Rule   |
+------------------+
|                  |
|                  |
|                  |
|                  |
|                  |
|                  |
|                  |
|                  |
|                  |
+------------------+

But given the space constraints in this area I feel like putting an icon on that bar is a good trade-off.

I tweaked the plus icon a little since it was blurry. It also appears to be darker than the rest of the icons.

Comment 9

[Stephen Horlander [:shorlander]]

Attachment: add.svg

Comment 10

[Tim Nguyen :ntim]

Attachment: Patch v2

Comment 11

[Tim Nguyen :ntim]

(In reply to Stephen Horlander [:shorlander] from comment #8)
Thanks for the ui-review !

> >> Should the "Add rule" button add a rule for the currently emulated pseudo class?
> Yes, I think it makes sense to add the emulated state to the rule. It
> eliminates the added step of typing in the pseudo-classes and mirrors what
> you are actually looking at.
Postponed to bug 1165032, since it needs the pseudo class locks to be implemented first.

Comment 12

[Tim Nguyen :ntim]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a6df986e466

Comment 13

[Gabriel [:gl] (ΦωΦ)]

We may want to land this with the button hidden initially until we have all the weird edge cases handled for add rule.

Comment 14

[Brian Grinstead [:bgrins]]

Comment on attachment 8605999 [details] [diff] [review]
Patch v2

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

Looks great, a few notes

::: browser/devtools/styleinspector/cssruleview.xhtml
@@ +44,5 @@
>                   class="devtools-searchinput devtools-rule-searchbox"
>                   type="search" placeholder="&filterStylesPlaceholder;"/>
>            <button id="ruleview-searchinput-clear" class="devtools-searchinput-clear"></button>
>          </div>
> +        <button id="ruleview-add-rule-button" title="&addRuleButtonTooltip;" class="devtools-button"></button>

Gabriel mentioned that we may want to not make this so visible until a couple of the main bugs with this feature are sorted out (most prominently Bug 1139058).  I agree, but don't want to block landing this and have it bit rot.  So I'm thinking we land this with hidden="true" so the button isn't visible yet, and then have a separate patch that makes it visible that is blocked on anything necessary

::: browser/devtools/styleinspector/test/browser_ruleview_add-rule_01.js
@@ +58,3 @@
>  
> +    ok(!view.menuitemAddRule.hidden, "Add rule is visible");
> +    

Nit: whitespace

@@ +71,2 @@
>  
> +  if (method == "context-menu") {

Can we hide this popup unconditionally?  That would make it so that we have a function that takes in `view` and `method` (maybe called 'addNewRule').  Then once that's separated it could be copy/pasted into each of these 3 tests

::: browser/devtools/styleinspector/test/browser_ruleview_add-rule_02.js
@@ +34,4 @@
>    info("Selecting the test element");
>    yield selectNode("#testid", inspector);
>  
> +  if (method == "context-menu") {

See comment in add-rule_01.js about having a separate function for this pasted into this file

::: browser/devtools/styleinspector/test/browser_ruleview_add-rule_03.js
@@ +33,4 @@
>    info("Selecting the test element");
>    yield selectNode("#testid", inspector);
>  
> +  if (method == "context-menu") {

See comment in add-rule_01.js about having a separate function for this pasted into this file

::: browser/locales/en-US/chrome/browser/devtools/styleinspector.dtd
@@ +17,5 @@
>    -  the search box when no search term has been entered. -->
>  <!ENTITY filterStylesPlaceholder      "Filter Styles">
>  
> +<!-- LOCALIZATION NOTE (addRuleButtonTooltip): This is the tooltip shown when
> +  -  hovering the `Add new rule` button in the rules view toolbar -->

maybe include in the comment that this should match ruleView.contextmenu.addNewRule in styleinspector.properties

::: toolkit/locales/en-US/chrome/global/devtools/styleinspector.properties
@@ +116,1 @@
>  # rule view context menu for adding a new rule to the element.

Maybe include in the comment that this should match addRuleButtonTooltip in styleinspector.dtd

Comment 15

[Tim Nguyen :ntim]

Attachment: Patch v2.1

Comment 16

[Tim Nguyen :ntim]

Attachment: Patch v2.1

Comment 17

[Tim Nguyen :ntim]

Attachment: Patch v2.2

Removed some trailing whitespaces.

Comment 18

[Tim Nguyen :ntim]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf17a35a6966

Comment 19

[Tim Nguyen :ntim]

Attachment: Patch v2.3

Fixed a small issue with the new add icon in CSS filter tooltip.

Comment 20

[Gabriel [:gl] (ΦωΦ)]

Comment on attachment 8606028 [details] [diff] [review]
Patch v2.2

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

::: browser/devtools/styleinspector/test/browser_ruleview_add-rule_01.js
@@ +32,2 @@
>    info("Creating the test document");
>    content.document.body.innerHTML = PAGE_CONTENT;

Can you also update the test files to include the content inside the TEST_URI. See:
https://dxr.mozilla.org/mozilla-central/source/browser/devtools/styleinspector/test/browser_ruleview_search-filter_01.js

Comment 21

[Tim Nguyen :ntim]

Attachment: Patch v2.5

(In reply to Gabriel Luong [:gl] from comment #20)
> Comment on attachment 8606028 [details] [diff] [review]
> Patch v2.2
> 
> Review of attachment 8606028 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/styleinspector/test/browser_ruleview_add-rule_01.js
> @@ +32,2 @@
> >    info("Creating the test document");
> >    content.document.body.innerHTML = PAGE_CONTENT;
> 
> Can you also update the test files to include the content inside the
> TEST_URI. See:
> https://dxr.mozilla.org/mozilla-central/source/browser/devtools/
> styleinspector/test/browser_ruleview_search-filter_01.js

Done. :)

Comment 22

[Tim Nguyen :ntim]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=43ce0a2fa01c

Comment 23

[Tim Nguyen :ntim]

Comment on attachment 8606041 [details] [diff] [review]
Patch v2.5

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

::: browser/devtools/styleinspector/test/browser_ruleview_add-rule_02.js
@@ +17,5 @@
>    '<span>This is a span</span>'
>  ].join("\n");
>  
>  add_task(function*() {
> +  yield addTab("data:text/html;charset=utf-8," + encodeURIComponent(PAGE_CONTENT));

This change made the test fail. I'm guessing it's because the DOM is no longer the same.
This line must be failing now:
content.document.querySelector("body + style").innerHTML = "";

Anyway, I'll check that tomorrow.

Comment 24

[Gabriel [:gl] (ΦωΦ)]

It is probably easier to just add one new test file that does the toolbar button since it is calling the same function (In reply to Tim Nguyen [:ntim] (limited availability) from comment #23)
> Comment on attachment 8606041 [details] [diff] [review]
> Patch v2.5
> 
> Review of attachment 8606041 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/styleinspector/test/browser_ruleview_add-rule_02.js
> @@ +17,5 @@
> >    '<span>This is a span</span>'
> >  ].join("\n");
> >  
> >  add_task(function*() {
> > +  yield addTab("data:text/html;charset=utf-8," + encodeURIComponent(PAGE_CONTENT));
> 
> This change made the test fail. I'm guessing it's because the DOM is no
> longer the same.
> This line must be failing now:
> content.document.querySelector("body + style").innerHTML = "";
> 
> Anyway, I'll check that tomorrow.

I think you can probably just do content.document.body.innerHTML.

Comment 25

[Brian Grinstead [:bgrins]]

(In reply to Gabriel Luong [:gl] from comment #24)
> It is probably easier to just add one new test file that does the toolbar
> button since it is calling the same function

Yeah, I'd be fine with just having both the toolbar button + context menu being checked in the 01 test, and then making 02 and 03 just use the toolbar button since it's easier / less code

Comment 26

[Tim Nguyen :ntim]

Attachment: Patch v2.6

Simpler tests

Comment 27

[Tim Nguyen :ntim]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9629fe7832b2

Comment 28

[Brian Grinstead [:bgrins]]

Comment on attachment 8606347 [details] [diff] [review]
Patch v2.6

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

Nice! r=me with the changes below

::: browser/devtools/styleinspector/rule-view.js
@@ +1142,5 @@
>    this.searchClearButton.hidden = true;
>  
>    this.element.addEventListener("copy", this._onCopy);
>    this.element.addEventListener("contextmenu", this._onContextMenu);
> +  this.addRuleButton.addEventListener("click", this._onAddRule);

The other listeners are removed in the destroy() function below - may as well be symmetrical and remove this then

::: browser/devtools/styleinspector/test/browser_ruleview_add-rule_01.js
@@ +68,5 @@
> +    view.menuitemAddRule.click();
> +    view._contextmenu.hidePopup();
> +  }
> +  else {
> +    info("Adding the new rule");

Update this message to include that it's adding the new rule by clicking the button

Comment 29

[Tim Nguyen :ntim]

Attachment: Patch v2.7 (r=bgrins)

Comment 30

[Tim Nguyen :ntim]

Attachment: Patch v2.8 (r=bgrins)

Comment 31

[Pulsebot]

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

Comment 32

[Gabriel [:gl] (ΦωΦ)]

I am not quite if this is just my eyes, but the add button still looks quite dark compare to the other devtools button.

Comment 33

[Brian Grinstead [:bgrins]]

(In reply to Gabriel Luong [:gl] from comment #32)
> I am not quite if this is just my eyes, but the add button still looks quite
> dark compare to the other devtools button.

Filed Bug 1166029

Comment 34

[Guilherme Lima]

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

Comment 35

[Tim Nguyen :ntim]

Hope it's not too late.

Release Note Request (optional, but appreciated)
[Why is this notable]: new devtools rule view button
[Suggested wording]: New button to Add Rule in the rules view.
[Links (documentation, blog post, etc)]: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_CSS#Add_rules

Comment 36

[Sylvestre Ledru [:Sylvestre]]

This has been added to the release notes in the bug 1165122.

Comment 37

[Saheda Reza [:Antora]]

I didn't found 'Add rule' toolbar button on Firefox nightly Version 39.0a1

It's fixed and verified on Latest nightly and Developer Edition

 Latest nightly
    Build ID 	20150902030229
    User Agent 	Mozilla/5.0 (Windows NT 6.3; rv:43.0) Gecko/20100101 Firefox/43.0
             
 Developer Edition
    Build ID 	20150902004024
    User Agent 	Mozilla/5.0 (Windows NT 6.3; rv:42.0) Gecko/20100101 Firefox/42.0


Tested OS--- windows 8.1 32bit

Comment 38

[Nazir Ahmed Sabbir [:NaSb]]

I found this issue on Firefox Nightly 39.0a1 (2015-02-24) (Build ID: 20150224030228) on Linux x64

This Bug is now verified as fixed on Latest Beta 41.0b8 

Build ID: 20150907144446

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:41.0) Gecko/20100101 Firefox/41.0

As it is also verified on Windows (Comment 37), Marking it as verified!