Closed Bug 1295008 Opened 3 years ago Closed 3 years ago

Don't run duplicate the eyedropper

Categories

(DevTools :: Inspector, defect, P2)

defect

Tracking

(firefox50 fixed, firefox51 verified)

VERIFIED FIXED
Firefox 51
Tracking Status
firefox50 --- fixed
firefox51 --- verified

People

(Reporter: magicp.jp, Assigned: pbro)

References

Details

Attachments

(2 files)

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.
Has STR: --- → yes
Component: Untriaged → Developer Tools: Inspector
OS: Unspecified → All
Hardware: Unspecified → All
Inspector bug triage (filter on CLIMBING SHOES).
Priority: -- → P2
Duplicate of this bug: 1297015
Assignee: nobody → pbrosset
Blocks: 1262439
Status: NEW → ASSIGNED
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+
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
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?
https://hg.mozilla.org/mozilla-central/rev/9e3157084c5b
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
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)
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)
(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)
Status: RESOLVED → 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+
(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)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.