Closed Bug 1286128 Opened 6 years ago Closed 5 years ago

Don't overflow eyedropper button in the color picker

Categories

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

defect

Tracking

(firefox48 affected, firefox49 affected, firefox50 verified)

VERIFIED FIXED
Firefox 50
Tracking Status
firefox48 --- affected
firefox49 --- affected
firefox50 --- verified

People

(Reporter: magicp.jp, Assigned: Towkir, Mentored)

References

Details

(Whiteboard: [good first bug][lang=css])

Attachments

(2 files, 1 obsolete file)

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.
Has STR: --- → yes
Component: Untriaged → Developer Tools: CSS Rules Inspector
OS: Unspecified → All
Hardware: Unspecified → All
Inspector bug triage (filter on CLIMBING SHOES).
Mentor: pbrosset
Priority: -- → P3
Whiteboard: [good first bug][lang=css]
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
Flags: needinfo?(pbrosset)
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.
Assignee: nobody → 3ugzilla
Status: NEW → ASSIGNED
Flags: needinfo?(pbrosset)
Hi Patrick, 
Is it a good idea to use negative values in css? 
I can generate a result like this http://prnt.sc/bvk024
Flags: needinfo?(pbrosset)
Hi Julian, what is your suggestion on comment 5 ?
Flags: needinfo?(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.
Flags: needinfo?(jdescottes)
Attached patch bug1286128.padding.patch (obsolete) — Splinter Review
This patch just fixes a padding issue, to make it easier to test the issue investigated here on the latest code level.
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.
Flags: needinfo?(pbrosset)
(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.
See Also: → 1288358
Thanks guys for helping me out with this micro patch ;)
Attachment #8773207 - Attachment is obsolete: true
Attachment #8773214 - Flags: review?(jdescottes)
Attachment #8773214 - Flags: review?(jdescottes) → review+
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)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/7436d69f220b
Prevent eyedropper button from being overflown in the color picker. r=jdescottes
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7436d69f220b
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
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]
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.