Don't hiding color value of the new eyedropper

VERIFIED FIXED in Firefox 50

Status

()

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

People

(Reporter: magicp, Assigned: pbro)

Tracking

Trunk
Firefox 51
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox50 fixed, firefox51 verified)

Details

MozReview Requests

()

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

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
Created attachment 8774843 [details]
do-not-hiding-color-value.png

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

Steps to reproduce:

1. Start Nightly
2. Open DevTools > Inspector
3. Enable the new eyedropper
4. Move to the left-bottom corner


Actual results:

Cannot see color value.

Regression range:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=e0bc88708ffed39aaab1fbc0ac461d93561195de&tochange=a7a882d122e36defe6c2a102a28ae7dfc16311c5


Expected results:

Can see like old version. Please find attached file
(Reporter)

Updated

2 years ago
Blocks: 1262439
Has Regression Range: --- → yes
Has STR: --- → yes
status-firefox50: --- → affected
Component: Untriaged → Developer Tools: Inspector
OS: Unspecified → All
Hardware: Unspecified → All
Since this is using the highlighter API we don't have any ability as far as overlapping the toolbox and/or going outside of the browser window.  But it should be possible to adapt the highlighter's layout based on the current mouse position, similar to what we do for the box model highlighter on elements near the top of the page where it moves the info tooltip below the element instead of above it.
(Reporter)

Updated

2 years ago
status-firefox51: --- → affected
Recent regression from eyedropper migration, non blocking -> P2.

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

Comment 3

a year ago
(In reply to Brian Grinstead [:bgrins] from comment #1)
> Since this is using the highlighter API we don't have any ability as far as
> overlapping the toolbox and/or going outside of the browser window.  But it
> should be possible to adapt the highlighter's layout based on the current
> mouse position, similar to what we do for the box model highlighter on
> elements near the top of the page where it moves the info tooltip below the
> element instead of above it.
Indeed, extending out of the document was a known limitation when we started to work on the new implementation of the eyedropper.
Because we're not using XUL anymore, we can't use floating XUL panels (which can extend out of the bounds of the browser) and instead the eyedropper is injected as a devtools highlighter [1] into the page, therefore constraining it to the page's bounds.

So, as Brian said, we should move the label so it's always visible to the user.

[1] https://wiki.mozilla.org/DevTools/Highlighter
(Assignee)

Updated

a year ago
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)

Comment 6

a year ago
mozreview-review
Comment on attachment 8782370 [details]
Bug 1289553 - Move the eyedropper label so it's always visible;

https://reviewboard.mozilla.org/r/72546/#review70320

Nice! I love the smooth animation when the label moves around. 
Great patch, a couple of comments nits, just for the sake of commenting.

::: devtools/client/inspector/test/browser_inspector_highlighter-eyedropper-label.js:7
(Diff revision 1)
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +"use strict";
> +
> +// Test the position of the eyedropper label.
> +// It should move around when the eyedropper is close to the viewport height so as to

nit: "is close to the edges of the viewport" ?

::: devtools/server/actors/highlighters.css:482
(Diff revision 1)
> +}
> +:-moz-native-anonymous .eye-dropper-root[left][top] .eye-dropper-color-container {
> +  transform: var(--label-horizontal-left) var(--label-vertical-top);
> +}
> +
> +/* Same on the other side */

extra-nit: let's use a verbose comment here, in case this code moves and is no longer right after the previous one.

::: devtools/shared/layout/utils.js:679
(Diff revision 1)
> +
> +/**
> + * Return the default view for a given node, where node can be:
> + * - a DOM node
> + * - the document node
> + * - the window itself

nit: the whole file is consistent regarding jsdoc, please add it.
Attachment #8782370 - Flags: review?(jdescottes) → review+

Comment 7

a year ago
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/8d0277ea4db2
Move the eyedropper label so it's always visible; r=jdescottes

Comment 8

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8d0277ea4db2
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
(Reporter)

Comment 9

a year ago
Excellent work!! Thanks all.

Comment 10

a year ago
I have reproduced this bug with Nightly 50.0a1(2016-07-26) on Windows 10, 64 bit.

The Bug's fix is now verified on latest Nightly 51.0a1

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

[bugday-20160817]
(Assignee)

Comment 11

a year ago
Comment on attachment 8782370 [details]
Bug 1289553 - Move the eyedropper label so it's always visible;

Approval Request Comment
[Feature/regressing bug #]: bug 1262439
[User impact if declined]: The eyedropper can be moved with the mouse over the viewport, and just below it is a small area that contains the color code for the pixel currently hovered by the mouse. As you move closer to the edges of the viewport, this area may become hidden below the edges. So, if declined, users of the eyedropper won't always see the color code of the pixel they are hovering in the page.
[Describe test coverage new/current, TreeHerder]: New automated mochitest added, also manually tested in nightly.
[Risks and why]: This is a rather minor CSS/JS change in the eyedropper code only. Risk is low. If there were bugs in the changes here, they would only affect the eyedropper, the browser would still function properly, and devtools would also work correctly.
[String/UUID change made/needed]: None
Attachment #8782370 - Flags: approval-mozilla-aurora?
(Reporter)

Updated

a year ago
Duplicate of this bug: 1259001

Updated

a year ago
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
Comment on attachment 8782370 [details]
Bug 1289553 - Move the eyedropper label so it's always visible;

Fix was verified on Nightly, seems isolated to the devtools inspector eyedropper functionality so should we low risk, Aurora50+
Attachment #8782370 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Needs rebasing for Aurora.
Flags: needinfo?(pbrosset)
(Assignee)

Comment 15

a year ago
Created attachment 8784712 [details] [diff] [review]
patch-rebased-for-aurora.diff

Here's the rebased patch for Aurora.
Flags: needinfo?(pbrosset)

Comment 16

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/e851620e760b
status-firefox50: affected → fixed
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.