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

VERIFIED FIXED in Firefox 41

Status

P1
normal
VERIFIED FIXED
4 years ago
6 months ago

People

(Reporter: ntim, Assigned: ntim)

Tracking

unspecified
Firefox 41
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox -)

Details

(Whiteboard: [polish-backlog][difficulty=easy])

Attachments

(3 attachments, 9 obsolete attachments)

Comment hidden (empty)
Depends on: 1120616
Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED
Whiteboard: [devedition-40][difficulty=easy]
Created attachment 8594022 [details] [diff] [review]
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.
Attachment #8594022 - Flags: review?(bgrinstead)
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.
See Also: → bug 987365
Summary: Add rule toolbar button in the rules view → Add an 'Add rule' toolbar button in the rules view
(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?
Depends on: 1139058
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).
Attachment #8594022 - Flags: review?(bgrinstead) → feedback+
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.
Attachment #8594022 - Flags: ui-review?(shorlander)
Created attachment 8596263 [details]
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.
Priority: -- → P1
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.
Attachment #8594022 - Flags: ui-review?(shorlander) → ui-review+
Depends on: 1165032
Created attachment 8605999 [details] [diff] [review]
Patch v2
Attachment #8594022 - Attachment is obsolete: true
Attachment #8605999 - Flags: review?(bgrinstead)
(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.
We may want to land this with the button hidden initially until we have all the weird edge cases handled for add rule.
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
Attachment #8605999 - Flags: review?(bgrinstead) → feedback+
Blocks: 1165122
Created attachment 8606024 [details] [diff] [review]
Patch v2.1
Attachment #8605999 - Attachment is obsolete: true
Attachment #8606024 - Flags: review?(bgrinstead)
Attachment #8606024 - Flags: review?(bgrinstead)
Created attachment 8606026 [details] [diff] [review]
Patch v2.1
Attachment #8606024 - Attachment is obsolete: true
Attachment #8606026 - Flags: review?(bgrinstead)
Created attachment 8606028 [details] [diff] [review]
Patch v2.2

Removed some trailing whitespaces.
Attachment #8606026 - Attachment is obsolete: true
Attachment #8606026 - Flags: review?(bgrinstead)
Attachment #8606028 - Flags: review?(bgrinstead)
Created attachment 8606033 [details] [diff] [review]
Patch v2.3

Fixed a small issue with the new add icon in CSS filter tooltip.
Attachment #8606028 - Attachment is obsolete: true
Attachment #8606028 - Flags: review?(bgrinstead)
Attachment #8606033 - Flags: review?(bgrinstead)
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
Attachment #8606028 - Attachment is obsolete: false
Created attachment 8606041 [details] [diff] [review]
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. :)
Attachment #8606028 - Attachment is obsolete: true
Attachment #8606033 - Attachment is obsolete: true
Attachment #8606033 - Flags: review?(bgrinstead)
Attachment #8606041 - Flags: review?(bgrinstead)
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.
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.
(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
Created attachment 8606347 [details] [diff] [review]
Patch v2.6

Simpler tests
Attachment #8606041 - Attachment is obsolete: true
Attachment #8606041 - Flags: review?(bgrinstead)
Attachment #8606347 - Flags: review?(bgrinstead)
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
Attachment #8606347 - Flags: review?(bgrinstead) → review+
Created attachment 8606391 [details] [diff] [review]
Patch v2.7 (r=bgrins)
Attachment #8606347 - Attachment is obsolete: true
Attachment #8606391 - Flags: review+
Keywords: checkin-needed
Keywords: checkin-needed
Created attachment 8606603 [details] [diff] [review]
Patch v2.8 (r=bgrins)
Attachment #8606603 - Flags: review+
Attachment #8606391 - Attachment is obsolete: true
Keywords: checkin-needed
I am not quite if this is just my eyes, but the add button still looks quite dark compare to the other devtools button.
(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

4 years ago
https://hg.mozilla.org/mozilla-central/rev/db2bac6070b9
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Whiteboard: [devedition-40][difficulty=easy] → [polish-backlog][difficulty=easy]
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
relnote-firefox: --- → ?
This has been added to the release notes in the bug 1165122.
relnote-firefox: ? → -
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
QA Whiteboard: [bugday-20150902]
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!
Status: RESOLVED → VERIFIED

Updated

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