1165122 - Show Add New Rule button by default

Status: VERIFIED FIXED

(DevTools :: Inspector, defect)

Description

[Tim Nguyen :ntim]

Bug 1136101 landed with the functionality hidden. This bug is about exposing it.

Comment 1

[Tim Nguyen :ntim]

Attachment: unhide-add-button.patch

Comment 2

[Brian Grinstead [:bgrins]]

Comment on attachment 8616366 [details] [diff] [review]
unhide-add-button.patch

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

There's a bunch of unrelated whitespace changes in this patch

Comment 3

[Tim Nguyen :ntim]

Attachment: patch

Urgh, auto whitespace "correction" addon.

Comment 4

[Brian Grinstead [:bgrins]]

Jeff and Gabriel, do you think the 'add rule' feature is ready for full exposure through a button visible at the top of the rule view?  I remember there were some bugs we were waiting for (I believe the biggest of which was unmatched rules - bug 1139058).  If not, let's tag this block as depending on anything relevant.

Comment 5

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

(In reply to Brian Grinstead [:bgrins] from comment #4)
> Jeff and Gabriel, do you think the 'add rule' feature is ready for full
> exposure through a button visible at the top of the rule view?  I remember
> there were some bugs we were waiting for (I believe the biggest of which was
> unmatched rules - bug 1139058).  If not, let's tag this block as depending
> on anything relevant.

So, add rule and edit selector should be bug free.

The notable bugs to enabling 'add rule' button was:
https://bugzilla.mozilla.org/show_bug.cgi?id=1040199
https://bugzilla.mozilla.org/show_bug.cgi?id=1043835
https://bugzilla.mozilla.org/show_bug.cgi?id=1139058
which has all been landed.

Comment 6

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Gabriel Luong [:gl] from comment #5)
> (In reply to Brian Grinstead [:bgrins] from comment #4)
> > Jeff and Gabriel, do you think the 'add rule' feature is ready for full
> > exposure through a button visible at the top of the rule view?  I remember
> > there were some bugs we were waiting for (I believe the biggest of which was
> > unmatched rules - bug 1139058).  If not, let's tag this block as depending
> > on anything relevant.
> 
> So, add rule and edit selector should be bug free.
> 
> The notable bugs to enabling 'add rule' button was:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1040199
> https://bugzilla.mozilla.org/show_bug.cgi?id=1043835
> https://bugzilla.mozilla.org/show_bug.cgi?id=1139058
> which has all been landed.

What about https://bugzilla.mozilla.org/show_bug.cgi?id=1166959 ?

Comment 7

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

That one is still a WIP(In reply to Jeff Griffiths (:canuckistani) from comment #6)
> (In reply to Gabriel Luong [:gl] from comment #5)
> > (In reply to Brian Grinstead [:bgrins] from comment #4)
> > > Jeff and Gabriel, do you think the 'add rule' feature is ready for full
> > > exposure through a button visible at the top of the rule view?  I remember
> > > there were some bugs we were waiting for (I believe the biggest of which was
> > > unmatched rules - bug 1139058).  If not, let's tag this block as depending
> > > on anything relevant.
> > 
> > So, add rule and edit selector should be bug free.
> > 
> > The notable bugs to enabling 'add rule' button was:
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1040199
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1043835
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1139058
> > which has all been landed.
> 
> What about https://bugzilla.mozilla.org/show_bug.cgi?id=1166959 ?

That one is still a WIP, but it is not necessary a bug with the 'add rule' functionality. We can block until bug 1166959 is fixed.

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Gabriel Luong [:gl] from comment #7)
> That one is still a WIP(In reply to Jeff Griffiths (:canuckistani) from
> comment #6)
> > What about https://bugzilla.mozilla.org/show_bug.cgi?id=1166959 ?
> 
> That one is still a WIP, but it is not necessary a bug with the 'add rule'
> functionality. We can block until bug 1166959 is fixed.

Alright - blocking on that.  This one isn't going to bitrot since it's just an enabling patch and all the hard work has already landed in Bug 1136101.

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8616761 [details] [diff] [review]
patch

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

Looks good to me, let's wait for the blockers to land before landing this

Comment 10

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

I was looking through the rule-view code and noticed this:
this.menuitemAddRule.disabled = this.inspector.selection.isAnonymousNode(); [1]

I believe the add rule button should be disabled if the current selection is an anonymous node.

[1] https://dxr.mozilla.org/mozilla-central/source/browser/devtools/styleinspector/rule-view.js#1378

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Gabriel Luong [:gl] from comment #10)
> I was looking through the rule-view code and noticed this:
> this.menuitemAddRule.disabled = this.inspector.selection.isAnonymousNode();
> [1]
> 
> I believe the add rule button should be disabled if the current selection is
> an anonymous node.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/browser/devtools/
> styleinspector/rule-view.js#1378

Good catch.  In selectElement we should disable this button if !this.inspector.selection.isElementNode() || this.inspector.selection.isAnonymousNode().  Tim, can you update this code and add a test case for that?

Comment 12

[Tim Nguyen :ntim]

Filed bug 1173298

Comment 13

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

Attachment: 1166959.patch

Rebased. Gonna check if I should land this.

Comment 14

[Pulsebot]

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

Comment 15

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

Backed out for now. https://hg.mozilla.org/integration/fx-team/rev/84af61ed4887

Comment 16

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

Attachment: 1165122.patch

Comment 17

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

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

Comment 18

[Pulsebot]

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

Comment 19

[Carsten Book [:Tomcat]]

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

Comment 20

[Lawrence Mandel [:lmandel] (use needinfo)]

Release Note Request (optional, but appreciated)
[Why is this notable]: New toolbar button
[Suggested wording]: Quickly add new CSS rule with New Rule button
[Links (documentation, blog post, etc)]:

Comment 21

[Lawrence Mandel [:lmandel] (use needinfo)]

Correction. Noted as "Quickly add new CSS rule with New Rule button in the Inspector"

Comment 22

[Nazir Ahmed Sabbir [:NaSb]]

I found this issue in Nightly 41.0a1 (2015-05-14) (Build ID: 20150514032200) on Linux x64

'Add new rule' button is showing now by default in Latest Firefox Beta 41.0b3

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