102.62 KB, image/png
147.83 KB, image/png
831 bytes, patch
Deepjyoti Mondal: review+
|Details | Diff | Splinter Review|
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://email@example.com no longer exists and any remaining occurrence should be replaced by chrome://devtools/skin/images/close.
Marking as good first bug!
Whiteboard: [good first bug]
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.
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.
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+
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.
Attachment #8766459 - Attachment is obsolete: true
Pushed by firstname.lastname@example.org: 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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
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
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.
status-firefox49: affected → fixed
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]
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.