1289553 - Don't hiding color value of the new eyedropper

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P2)

Description

[magicp]

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

Comment 1

[Brian Grinstead [:bgrins]]

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.

Comment 2

[Julian Descottes [:jdescottes]]

Recent regression from eyedropper migration, non blocking -> P2.

Inspector bug triage, filter on CLIMBING SHOES.

Comment 3

[Patrick Brosset <:pbro>]

(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

Comment 4

[Patrick Brosset <:pbro>]

Attachment: Bug 1289553 - Move the eyedropper label so it's always visible;

This adds a few new CSS selectors that are used to move the label to the
top and/or left/right of the eyedropper canvas.
The CSS rules use transform and a quick transition.
The eyedropper highlighter then just makes use of this by adding top, left,
right attributes to the DOM depending on its position.

This also adds a test for this, and while testing, I discovered a bug in
shared/layout/utils.js that I fixed here too. Sometimes, the node passed is
actually a DOCUMENT_NODE and so we must account for this in a couple of places
in this file to avoid JS errors.

Review commit: https://reviewboard.mozilla.org/r/72546/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72546/

Comment 5

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f9edb106f76&group_state=expanded

Comment 6

[Julian Descottes [:jdescottes]]

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.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/8d0277ea4db2
Move the eyedropper label so it's always visible; r=jdescottes

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8d0277ea4db2

Comment 9

[magicp]

Excellent work!! Thanks all.

Comment 10

[Tanvir Rahman]

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]

Comment 11

[Patrick Brosset <:pbro>]

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

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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+

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for Aurora.

Comment 15

[Patrick Brosset <:pbro>]

Attachment: patch-rebased-for-aurora.diff

Here's the rebased patch for Aurora.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e851620e760b