1283072 - Close icon missing from filter editor in inspector ruleview

Status: VERIFIED FIXED

(DevTools :: Inspector: Rules, defect)

Description

[Julian Descottes [:jdescottes]]

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/[e-mail] no longer exists and any remaining occurrence should be replaced by chrome://devtools/skin/images/close.

Comment 1

[Julian Descottes [:jdescottes]]

Marking as good first bug!

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: expected-filter-close-icon.png

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: 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.

Comment 4

[Deepjyoti Mondal]

Attachment: bug1283072.patch

Changed png icon reference with svg icon reference. Close button icons are now visible.

Comment 5

[Sebastin Santy [:seban]]

I have checked the patch by Deepjyoti, and it has run successfully and is showing the expected SVG close button.

Comment 6

[[:Towkir] Ahmed]

Assigning to Deepjyoti

Comment 7

[Julian Descottes [:jdescottes]]

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!

Comment 8

[Deepjyoti Mondal]

Attachment: bug1283072.patch

updated my previous patch.

Comment 9

[Julian Descottes [:jdescottes]]

Thanks, looks good to me. Tests look good, except for unrelated intermittents. Adding checkin-needed.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/897d2094bec1

Comment 12

[Tim Nguyen :ntim]

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

Comment 13

[Gerry Chang [:gchang]]

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.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5e8bede01474

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/03b0c9698c91

Comment 16

[Khalid Syfullah Zaman [:khalid32]]

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

Comment 17

[Tanvir Rahman]

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]

Comment 18

[Khalid Syfullah Zaman [:khalid32]]

As this bug's fix is verified on Windows 10, 64 bit (comment 17), I am marking this as verified!