Don't run duplicate the eyedropper

VERIFIED FIXED in Firefox 50

Status

()

Firefox
Developer Tools: Inspector
P2
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: magicp, Assigned: pbro)

Tracking

Trunk
Firefox 51
Points:
---

Firefox Tracking Flags

(firefox50 fixed, firefox51 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
str
Created attachment 8780888 [details]
STR-run-duplicate-eyedropper.mp4

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20160813030202

Steps to reproduce:

1. Start Nightly
2. Run the eyedropper from menubar or PanelUI-contents
3. Open DevTools (Ctrl+Shift+I)
4. Check the eyedropper color (The checked status is not reflected)
5. Click the eyedropper icon
6. Check run duplicate the eyedropper


Actual results:

The eyedropper runs duplicate. And the checked status is not reflected if running it from menubar or PanelUI-contents.


Expected results:

Don't run duplicate the eyedropper and the checked status is reflected.
(Reporter)

Updated

2 years ago
Has STR: --- → yes
status-firefox50: --- → affected
status-firefox51: --- → affected
Component: Untriaged → Developer Tools: Inspector
OS: Unspecified → All
Hardware: Unspecified → All
(Assignee)

Comment 1

2 years ago
Inspector bug triage (filter on CLIMBING SHOES).
Priority: -- → P2
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1297015
(Assignee)

Updated

2 years ago
Assignee: nobody → pbrosset
Blocks: 1262439
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8783479 [details]
Bug 1295008 - Hide the previous eyedropper when we request to show a new one;

https://reviewboard.mozilla.org/r/73260/#review71646
Attachment #8783479 - Flags: review?(mratcliffe) → review+

Comment 8

2 years ago
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/9e3157084c5b
Hide the previous eyedropper when we request to show a new one; r=miker
(Assignee)

Comment 9

2 years ago
Comment on attachment 8783479 [details]
Bug 1295008 - Hide the previous eyedropper when we request to show a new one;

Approval Request Comment
[Feature/regressing bug #]: bug 1262439
[User impact if declined]: The eyedropper can be open from a few different locations in devtools: the command line, the inspector, the color picker, the devtools browser menu. Opening the eyedropper from the inspector, and then from the command line for example, will end up displaying 2 eyedroppers in the page if we decline this uplift request.
[Describe test coverage new/current, TreeHerder]: All current eyedropper tests still pass. No new test was added. This was only manually tested.
[Risks and why]: This is a devtools JS-only change. The risk is rather low. The code changes are located in the eyedropper itself, so if there were new bugs introduced there, only the devtools eyedropper would suffer from them. There are also a few code changes in the inspector which, in itself, could be a bit more risky, but these changes are only in the functions that show the eyedropper, so the inspector itself would still load and function properly even if these changes were wrong.
In any case, there are no impacts to the browser UI itself.
[String/UUID change made/needed]: None
Attachment #8783479 - Flags: approval-mozilla-aurora?

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9e3157084c5b
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Hello magicp, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(magicp.jp)
(Reporter)

Comment 12

2 years ago
Hello Ritu, I have confirmed this in 51 (Build ID: 20160826030226). Run duplicate was fixed, but the checked status is not reflected.
Flags: needinfo?(magicp.jp)
Hey Patrick, based on the verification in comment 12, we have only partially fixed the problem. Are you able to repro this on your end as well? I can wait for a patch-2 and we can uplift both to Aurora50 next week.
Flags: needinfo?(pbrosset)
(Assignee)

Comment 14

2 years ago
(In reply to magicp from comment #12)
> Hello Ritu, I have confirmed this in 51 (Build ID: 20160826030226). Run
> duplicate was fixed, but the checked status is not reflected.
Right, I missed this in the original STR in comment 0.

(In reply to Ritu Kothari (:ritu) from comment #13)
> Hey Patrick, based on the verification in comment 12, we have only partially
> fixed the problem. Are you able to repro this on your end as well? I can
> wait for a patch-2 and we can uplift both to Aurora50 next week.
Yes I can repro, I missed this detail when working on the fix.
I prefer not to add a second patch here. I'll file a separate bug for the remaining issue, and will ask uplift there if needed.
Flags: needinfo?(pbrosset)
(In reply to Patrick Brosset <:pbro> from comment #14)
> I prefer not to add a second patch here. I'll file a separate bug for the
> remaining issue, and will ask uplift there if needed.

Ok. I am fine with that. You or magicp can file another bug to track that. I'll approve this one for uplift then.
Flags: needinfo?(magicp.jp)

Updated

2 years ago
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
Comment on attachment 8783479 [details]
Bug 1295008 - Hide the previous eyedropper when we request to show a new one;

Fix was verified on Nightly, Aurora50+
Attachment #8783479 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 17

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/1bc4cc827105
status-firefox50: affected → fixed
(Reporter)

Comment 18

2 years ago
(In reply to Ritu Kothari (:ritu) from comment #15)

> Ok. I am fine with that. You or magicp can file another bug to track that.
> I'll approve this one for uplift then.

Hello Patrick, Thanks for filing Bug 1298769.
Flags: needinfo?(magicp.jp)
You need to log in before you can comment on or make changes to this bug.