Closed
Bug 1289553
Opened 9 years ago
Closed 8 years ago
Don't hiding color value of the new eyedropper
Categories
(DevTools :: Inspector, defect, P2)
DevTools
Inspector
Tracking
(firefox50 fixed, firefox51 verified)
VERIFIED
FIXED
Firefox 51
People
(Reporter: magicp.jp, Assigned: pbro)
References
Details
Attachments
(3 files)
54.08 KB,
image/png
|
Details | |
58 bytes,
text/x-review-board-request
|
jdescottes
:
review+
ritu
:
approval-mozilla-aurora+
|
Details |
13.45 KB,
patch
|
Details | Diff | Splinter Review |
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
Blocks: 1262439
Has Regression Range: --- → yes
Has STR: --- → yes
status-firefox50:
--- → affected
Component: Untriaged → Developer Tools: Inspector
OS: Unspecified → All
Hardware: Unspecified → All
Comment 1•9 years ago
|
||
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.
status-firefox51:
--- → affected
Comment 2•9 years ago
|
||
Recent regression from eyedropper migration, non blocking -> P2.
Inspector bug triage, filter on CLIMBING SHOES.
Priority: -- → P2
Assignee | ||
Comment 3•8 years 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•8 years ago
|
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•8 years ago
|
||
Comment 6•8 years 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+
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•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Comment 10•8 years 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•8 years 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?
Status: RESOLVED → 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+
Comment 16•8 years ago
|
||
bugherder uplift |
Flags: in-testsuite+
Updated•7 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•