Close icon missing from filter editor in inspector ruleview

VERIFIED FIXED in Firefox 48

Status

()

Firefox
Developer Tools: CSS Rules Inspector
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: jdescottes, Assigned: Deepjyoti Mondal, Mentored)

Tracking

Trunk
Firefox 50
Points:
---
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox48 verified, firefox49 verified, firefox50 verified)

Details

(Whiteboard: [good first bug])

Attachments

(3 attachments, 1 obsolete attachment)

The X icon displayed at the end of each line of the filter editor is missing.

STRs:
- open inspector on any page
- add the rule: "filter: blur(0px) grayscale(0%)"
- click on the filter swatch to open the filter editor

ER:
I should be able to delete each filter with a close icon.

AR:
Close icon is missing (if you click where it "should", be the filter is removed though :) )

This is a regression from Bug 1225184. Inspector icons were migrated from PNG to SVG, but one occurrence in devtools/client/shared/widgets/filter-widget.css was missed. Namely, the icon chrome://devtools/skin/images/close@2x.png no longer exists and any remaining occurrence should be replaced by chrome://devtools/skin/images/close.
Marking as good first bug!
Blocks: 1225184
Whiteboard: [good first bug]
Created attachment 8766270 [details]
expected-filter-close-icon.png
Created attachment 8766271 [details]
actual-filter-close-icon.png

Addded screenshots of the expected result (current release), and the actual result (nightly).

As you can see, X icons are missing on the "actual" screenshot. Please note that the new SVG icons have been redesigned and don't look the same as the old PNG icons, so the fix will look different than the old visuals, but that's fine.
(Assignee)

Comment 4

2 years ago
Created attachment 8766459 [details] [diff] [review]
bug1283072.patch

Changed png icon reference with svg icon reference. Close button icons are now visible.
Attachment #8766459 - Flags: review?(jdescottes)
I have checked the patch by Deepjyoti, and it has run successfully and is showing the expected SVG close button.

Comment 6

2 years ago
Assigning to Deepjyoti
Assignee: nobody → djmdeveloper060796
Status: NEW → ASSIGNED
Comment on attachment 8766459 [details] [diff] [review]
bug1283072.patch

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

Thanks for taking care of this bug Deepjyoti!

I have sent the patch to our test platform, which will run our test suite on different OSes & configuration, to make sure nothing else broke due to the patch.
You can follow the tests at https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c714d36a0e4 (results may take a few hours to appear).

For this patch, there is no reason this would trigger a regression and I think we can move forward.
Could you amend the commit message to add ";r=jdescottes" at the end of the first line? 
Also (to be nitpicky :)) the first line should in theory describe what was changed rather than the issue ("Bug 1283072 - [what you fixed];r=jdescottes").

For your new patch, a new review will not be necessary, so you can set the review+ flag yourself (but I'll be happy to do it if you have any trouble with bugzilla, just ping me if needed!). We will then set the "checkin-needed" keyword on this bug so that a sheriff can pick up your patch and land it on fx-team. After that, the fix will make its way to the release channel following our release calendar. It will be available in Firefox 50.

Thanks again!
Attachment #8766459 - Flags: review?(jdescottes) → review+
(Assignee)

Comment 8

2 years ago
Created attachment 8766721 [details] [diff] [review]
bug1283072.patch

updated my previous patch.
Attachment #8766721 - Flags: review+
Thanks, looks good to me. Tests look good, except for unrelated intermittents. Adding checkin-needed.
Keywords: checkin-needed
Attachment #8766459 - Attachment is obsolete: true

Comment 10

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/897d2094bec1
replaced png icon reference with svg icon reference in image url of .remove-button; r=jdescottes
Keywords: checkin-needed

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/897d2094bec1
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50

Comment 12

2 years ago
Comment on attachment 8766721 [details] [diff] [review]
bug1283072.patch

Approval Request Comment
[Feature/regressing bug #]: Bug 1225184
[User impact if declined]: Icon missing from the DevTools UI
[Describe test coverage new/current, TreeHerder]: in Nightly
[Risks and why]: low, 1 line CSS fix
[String/UUID change made/needed]: no
Attachment #8766721 - Flags: approval-mozilla-beta?
Attachment #8766721 - Flags: approval-mozilla-aurora?

Updated

2 years ago
status-firefox48: --- → affected
status-firefox49: --- → affected
Comment on attachment 8766721 [details] [diff] [review]
bug1283072.patch

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

This patch fixes a missing icon in DevTools UI. Take it in 48 beta 6 and aurora.
Attachment #8766721 - Flags: approval-mozilla-beta?
Attachment #8766721 - Flags: approval-mozilla-beta+
Attachment #8766721 - Flags: approval-mozilla-aurora?
Attachment #8766721 - Flags: approval-mozilla-aurora+

Updated

2 years ago
Flags: qe-verify+

Comment 14

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/5e8bede01474
status-firefox49: affected → fixed

Comment 15

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/03b0c9698c91
status-firefox48: affected → fixed
I have reproduced this bug on Nightly 50.0a1 (2016-06-29) (Build ID: 20160629030209) on Linux, 64 bit!

The bug's fix is now verified on Latest Beta 48.0b6!

Build ID: 20160706215822
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:48.0) Gecko/20100101 Firefox/48.0
OS: Ubuntu 16.04, Linux 4.4.0-28-generic
QA Whiteboard: [testday-20160708]

Comment 17

2 years ago
I have reproduced this bug with nightly 50.0a1 (2016-06-29) on Windows 10, 64 bit!

The Bug's fix is verified on Latest Nightly 50.0a1, Aurora 49.0a2, Beta 48.0b6-

Nightly 50.0a1
Build ID 	20160708030216
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0

Aurora 49.0a2
Build ID 	20160708004052
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0

Beta 48.0b6
Build ID 	20160706215822
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0

[testday-20160708]
As this bug's fix is verified on Windows 10, 64 bit (comment 17), I am marking this as verified!
Status: RESOLVED → VERIFIED
status-firefox48: fixed → verified
status-firefox49: fixed → verified
status-firefox50: fixed → verified
You need to log in before you can comment on or make changes to this bug.