1286128 - Don't overflow eyedropper button in the color picker

Status: VERIFIED FIXED

(DevTools :: Inspector: Rules, defect, P3)

Description

[magicp]

Attachment: do-not-overflow-eyedropper-button-in-the-spectrum.png

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

Steps to reproduce:

1. Start Nightly
2. Go to any sites (e.g. about:home)
3. Open DevTools > Inspector > Rules tab
4. Click to open color picker


Actual results:

The eyedropper button overflows to bottom in the spectrum frame.


Expected results:

The eyedropper button does not overflow.

Comment 1

[magicp]

Regression range:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=e3a487def5b169e7ebfc7b8c41bba1f541931037&tochange=6aff4aa8f3dee8b902434e572b65b4eff40d00dc

Comment 2

[Patrick Brosset <:pbro>]

Inspector bug triage (filter on CLIMBING SHOES).

Comment 3

[[:Towkir] Ahmed]

Hi patrick,
I am interested to work on this. can I take this ? 
If yes, please reply with the first step to be done here. I already tried to 'click to select element' on browser toolbox, but how do I find a spectrum frame ?
Thanks

Comment 4

[Patrick Brosset <:pbro>]

The color picker is a popup that goes away as soon as you switch to the browser toolbox, so it makes it hard to inspect. We have a special feature just for this. In the browser toolbox, go to the options panel (the cog, top right corner), and in the "Available Toolbox Buttons" section, make sure you check the "Disable popup auto hide" checkbox.
Now, you should have a new icon in the toolbar of the browser toolbox. Whenever you click that icon, any popup will stay on screen forever, it won't close if you click outside or switch windows. 
So you should enable that, and then use the browser toolbox to select the spectrum frame elements and investigate the problem.
Make sure you toggle off the popup auto-hide toolbar button at the end though, or it may cause surprising things to happen if you forget about it :)

Thanks for working on this. I'll assign the bug to you.

When you're ready with a patch you'd like someone to review, then please ask :jdescottes for this. He's been working on popups quite a lot lately, and would be a better reviewer than me for this.

Comment 5

[[:Towkir] Ahmed]

Hi Patrick, 
Is it a good idea to use negative values in css? 
I can generate a result like this http://prnt.sc/bvk024

Comment 6

[[:Towkir] Ahmed]

Hi Julian, what is your suggestion on comment 5 ?

Comment 7

[Julian Descottes [:jdescottes]]

I am not a big fan of compensating with negative margins here, as it feels a bit hacky. 

On OSX, there is a 2px vertical gap between the #spectrum element and the #eyedropper-button. On windows, the gap is 4px. Part of this might come from whitespace/inline-block issues [1], and it looks like the #eyedropper-button element is using display: inline-block. Maybe we could try changing the display of this element to block. 

Keeping the ni? for :pbro so that he can share his opinion on this.

[1] https://css-tricks.com/fighting-the-space-between-inline-block-elements/

Please note that if you are testing on the latest version of the code you will see another unrelated issue: the padding around the widget has been removed, so the colorpicker icon is really close to the edge of the tooltip. This is a regression from Bug 1267414. I will attach a patch here to fix that, so that you can work on a tooltip that looks the same as the one from the screenshots.

Comment 8

[Julian Descottes [:jdescottes]]

Attachment: bug1286128.padding.patch

This patch just fixes a padding issue, to make it easier to test the issue investigated here on the latest code level.

Comment 9

[Patrick Brosset <:pbro>]

I agree with Julian, negative margins here feel hacky.
I never looked at the details of the layout here, and always assumed that the colorpicker container was just not tall enough. Maybe we could just make it a tiny bit bigger to accommodate the icon? Or as Julian suggests, investigate changing the display of the element.

In any case, I would suggest not spending too much effort on this since we're in the process of re-designing the color picker in bug 1277352 (see mockup attachment 8758865 [details]).

So we should fix this, for sure, but anything that requires a big amount of changes, new tests, etc... is probably not worth it.

Comment 10

[Julian Descottes [:jdescottes]]

(In reply to Julian Descottes [:jdescottes] from comment #8)
> Created attachment 8773207 [details] [diff] [review]
> bug1286128.padding.patch
> 
> This patch just fixes a padding issue, to make it easier to test the issue
> investigated here on the latest code level.

Just logged Bug 1288358 to fix padding issues across all ruleview tooltips after the HTMLTooltip migration.

Comment 11

[[:Towkir] Ahmed]

Attachment: eyedropper_overflown_fixing.patch

Thanks guys for helping me out with this micro patch ;)

Comment 12

[Julian Descottes [:jdescottes]]

Thanks for your contribution Ahmed! Patch applies just fine on the top of fx-team and fixes the issue for me on Windows. 

Adding checkin-needed.

(try push in case: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7454f6c9e084 but this is a very small CSS change and I don't expect any regression here)

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/7436d69f220b
Prevent eyedropper button from being overflown in the color picker. r=jdescottes

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7436d69f220b

Comment 15

[Azmina]

I have reproduced this bug with Nightly 50.0a1 (2016-07-11) on Windows 8.1 , 64 bit!

This Bug's fix is verified on Latest Nightly 50.0a1.

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

[bugday-20160727]