Show Add New Rule button by default

VERIFIED FIXED in Firefox 41

Status

DevTools
Inspector
VERIFIED FIXED
3 years ago
a month ago

People

(Reporter: ntim, Assigned: ntim)

Tracking

unspecified
Firefox 41
Dependency tree / graph

Firefox Tracking Flags

(firefox41 verified, relnote-firefox 41+)

Details

(Whiteboard: [testday-20150821])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

3 years ago
Bug 1136101 landed with the functionality hidden. This bug is about exposing it.
(Assignee)

Updated

3 years ago
Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED
(Assignee)

Comment 1

3 years ago
Created attachment 8616366 [details] [diff] [review]
unhide-add-button.patch
Attachment #8616366 - Flags: review?(bgrinstead)
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
Attachment #8616366 - Flags: review?(bgrinstead) → review-
(Assignee)

Comment 3

3 years ago
Created attachment 8616761 [details] [diff] [review]
patch

Urgh, auto whitespace "correction" addon.
Attachment #8616366 - Attachment is obsolete: true
Attachment #8616761 - Flags: review?(bgrinstead)
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.
Flags: needinfo?(jgriffiths)
Flags: needinfo?(gabriel.luong)
Depends on: 1165032
(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.
Flags: needinfo?(gabriel.luong)
(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 ?
Flags: needinfo?(jgriffiths) → needinfo?(gabriel.luong)
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.
Flags: needinfo?(gabriel.luong)
(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.
Depends on: 1166959
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
Attachment #8616761 - Flags: review?(bgrinstead) → review+
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
No longer depends on: 1166959

Updated

3 years ago
Depends on: 1166959
(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?
Flags: needinfo?(ntim.bugs)
Attachment #8616761 - Flags: review+
(Assignee)

Updated

3 years ago
Depends on: 1173298
(Assignee)

Comment 12

3 years ago
Filed bug 1173298
Flags: needinfo?(ntim.bugs)
Depends on: 1175661
Created attachment 8626363 [details] [diff] [review]
1166959.patch

Rebased. Gonna check if I should land this.
Attachment #8616761 - Attachment is obsolete: true

Updated

3 years ago
Attachment #8626363 - Flags: review+
Created attachment 8626372 [details] [diff] [review]
1165122.patch
Attachment #8626363 - Attachment is obsolete: true
Attachment #8626372 - Flags: review+

Updated

3 years ago
Blocks: 1177564
https://hg.mozilla.org/mozilla-central/rev/e93d8ce6e114
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
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)]:
relnote-firefox: --- → 41+
Correction. Noted as "Quickly add new CSS rule with New Rule button in the Inspector"
QA Whiteboard: [good first verify]
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
QA Whiteboard: [good first verify] → [good first verify][testday-20150821]
Whiteboard: [testday-20150821]
(Assignee)

Updated

3 years ago
Status: RESOLVED → VERIFIED
status-firefox41: 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.