Closed Bug 1305365 Opened 3 years ago Closed 3 years ago

Color picker tooltip is too big in XUL documents (no eyedropper icon)

Categories

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

defect

Tracking

(firefox52 fixed)

RESOLVED FIXED
Firefox 52
Tracking Status
firefox52 --- fixed

People

(Reporter: jdescottes, Assigned: djmdeveloper060796, Mentored)

References

Details

(Keywords: good-first-bug)

Attachments

(2 files, 8 obsolete files)

The eyedropper icon is hidden in XUL Documents (see Bug 1289543).

In such documents, the color picker tooltip is too big because it has a fixed height which still accomodates for the eyedropper icon. 

STRs:
- go to about:home
- open inspector
- open a color picker tooltip on any color property

ER: The tooltip should be properly sized for its content
AR: The tooltip has a big whitespace gap at the bottom

We can either adapt the tooltip's height for XUL documents or actually have a disabled icon, explaining the feature is not available in XUL documents.
The disabled icon sounds like a good idea, maybe with a tooltip that explains why the icon is disabled.
Mentor: jdescottes
Keywords: good-first-bug
Priority: -- → P3
Attached patch bug1305365.patch (obsolete) — Splinter Review
I have disabled the eyedropper button for xul documents instead of making it invisible.
Attachment #8797686 - Flags: review?(jdescottes)
Assignee: nobody → djmdeveloper060796
Status: NEW → ASSIGNED
Comment on attachment 8797686 [details] [diff] [review]
bug1305365.patch

Review of attachment 8797686 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for your interest in this bug and for submitting a patch! I have assigned the bug to you.

This is on the right track. As Patrick mentioned in an earlier comment, we would like to also display a tooltip explaining why the icon is disabled.

To do so, you will need to add a new localized string in inspector.properties. Check the rest of the codebase to see how to use the LocalizationHelper located in devtools/shared/l10n.js (for instance http://searchfox.org/mozilla-central/rev/c1e745733c84630821ef53754b627f2c0b0b5202/devtools/client/shared/widgets/tooltip/ImageTooltipHelper.js#128).

Then, if it is disabled we should set this new string as the "title" attribute of the eyeButton. This brings 2 new issues :)
#1 By default title attributes are not displayed for disabled elements. You can workaround this by setting 'pointer-events: auto' on the element, I will check with the team if we usually use a different pattern though. Feel free to use this or any other workaround in the meantime
#2 HTML title attributes are not working in the colorpicker tooltip. A small modification in devtools/client/shared/widgets/HTMLTooltip is needed to enable them. I will attach a patch here that you can reuse to fix this.

Last few words: 
- the contributor documentation for devtools can be found at https://wiki.mozilla.org/DevTools/Hacking . If you haven't already, give it a read!
- Bug 1307193 just landed on our integration branch fx-team (not yet on mozilla-central though). It splits the Tooltip.js file in several files, so to avoid painful rebasing, try to get update your local repository to get this changeset as soon as you can.

Below, my comments on your patch:

::: devtools/client/shared/widgets/Tooltip.js
@@ +746,4 @@
>        let tooltipDoc = this.tooltip.doc;
>        let eyeButton = tooltipDoc.querySelector("#eyedropper-button");
>        if (value && this.inspector.selection.nodeFront.isInHTMLDocument) {
> +        eyeButton.disabled=false;

No need to set 'disabled' to false here, we can keep the default value.

@@ +750,3 @@
>          eyeButton.addEventListener("click", this._openEyeDropper);
>        } else {
> +        eyeButton.disabled=true;

nit: formatting: add a space before and after '='. We use ESLint to validate our syntax. See https://wiki.mozilla.org/DevTools/CodingStandards#JS_linting_with_ESLint . 

This will allow you to see if your code change contains any syntax issue.
Attachment #8797686 - Flags: review?(jdescottes) → feedback+
As mentioned in my previous comment here is a simple one liner to support title tooltips in your use case.

Feel free to simply take the code and include it in your next patch!
Thanks for the above patch.I will submit my next patch soon.
Comment on attachment 8797961 [details]
Bug 1305365 - support HTML title tooltips in HTMLTooltips using XUL wrapper

My bad, didn't mean to push this here.
Attachment #8797961 - Attachment is obsolete: true
Attached patch bug1305365.patch (obsolete) — Splinter Review
I have added a new string to the inspector.properties and I have set it as the title of the eyedropper when it is diabled for XUL documents.
Attachment #8797686 - Attachment is obsolete: true
Attachment #8799100 - Flags: review?(jdescottes)
Comment on attachment 8799100 [details] [diff] [review]
bug1305365.patch

Review of attachment 8799100 [details] [diff] [review]:
-----------------------------------------------------------------

This is great, works perfectly thank you! 
Just one small nit about the property name and text, and some comments regarding the patch's format.

Your commit message should be formatted slightly differently:
Bug 1305365 - disabled eyedropper for XUL documents and added a tooltip;r=jdescottes

(r=jdescottes simply means I'm the one reviewing your patch)

Your attachment is missing the author information, did you configure hg/git properly with your username and email? 
Or maybe you just exported a diff?

In any case look at the documentation [1] to see how to export a patch properly here.
https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Clearing the review flag waiting for a properly formatted patch.

In the meantime I have pushed your changeset to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1787d0f64e2115336c389342664dc03c177e01c 
This will run our test suite on your patch for an array of different platforms, so that we can make sure there are no unexpected regressions coming from it.

::: devtools/client/locales/en-US/inspector.properties
@@ +38,5 @@
>  previewTooltip.image.brokenImage=Could not load the image
>  
> +# LOCALIZATION NOTE: Used in color picker tooltip when the eyedropper is disabled for
> +# XUL documents
> +tooltip.eyedropper.title=Not available for XUL documents

Two small things: 

#1: I think the string name should indicate that this is useful when the eyedropper is unavailable. Also tooltip & title are redundant here.

    eyedropper.disabled.title maybe ?

#2: The eyedropper might be disabled in other documents than XUL documents, for instance SVG documents. So the message should not be too XUL specific. "Unavailable in non-HTML documents" or "Only available in HTML documents", or something really generic such as "Not available for the current document".
Attachment #8799100 - Flags: review?(jdescottes)
Attached patch bug1305365.patch (obsolete) — Splinter Review
I have updated my previous patch. "Unavailable in non-HTML documents" sounds good to me. I have used it as the message.
Attachment #8799344 - Flags: review?(jdescottes)
Comment on attachment 8799344 [details] [diff] [review]
bug1305365.patch

Review of attachment 8799344 [details] [diff] [review]:
-----------------------------------------------------------------

Patch looks great, thanks! 

As you can see on the try push (https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1787d0f64e2115336c389342664dc03c177e01c) the test devtools/client/inspector/test/browser_inspector_highlighter-eyedropper-xul.js is failing when applying your patch.

You can run this test locally by doing "./mach test browser_inspector_highlighter-eyedropper-xul.js" (more info at https://wiki.mozilla.org/DevTools/Hacking#DevTools_Mochitests)
Indeed this test checks that the eyedropper button is not displayed (as in has no dimensions at all) in the toolbar or in the the colorpicker tooltip.
For now in the colorpicker case let's simply check that the button is disabled instead of hidden. For the first check in the toolbar though, the isVisible() check remains valid.

This brings me to mention the toolbar: I will be in any case creating a follow up so that we disable the eyedropper icon in the toolbar instead of hiding it. I'll ping you when doing so in case you want to take this as well :) (should be pretty similar to the fix you worked on here).

Let's go for another round of review to update the test, ping me if you need any help with this!
Attachment #8799344 - Flags: review?(jdescottes)
Attached patch bug1305365.patch (obsolete) — Splinter Review
I have updated the test file to check whether the eye-dropper button is disabled or not.
Attachment #8799100 - Attachment is obsolete: true
Attachment #8799344 - Attachment is obsolete: true
Attachment #8800134 - Flags: review?(jdescottes)
Comment on attachment 8800134 [details] [diff] [review]
bug1305365.patch

Review of attachment 8800134 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, thanks a lot for fixing the test! R+ with just 2 small nits to address. 

Can you address my comments and push a new version of the patch? You can directly set the r+ flag on it instead of asking for review since it already got r+. 
Also, the patch did not apply cleanly on my repository, can you rebase before submitting a new patch (again ping me if you need any help).

In the meantime I pushed to try again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec453226050064abdba14f2e9c4530b0f66d46f5.
We'll add the checkin-needed keyword on your new patch if try is green, so that a sheriff can pick up your patch and land it on our integration branch.

Thanks again for working on this!

::: devtools/client/inspector/test/browser_inspector_highlighter-eyedropper-xul.js
@@ +30,4 @@
>    yield onColorPickerReady;
>  
>    button = cPicker.tooltip.doc.querySelector("#eyedropper-button");
> +  ok(isDisabled(button), "The button is hidden in the color picker");

Please update the message to "The button is disabled in the color picker"

::: devtools/client/shared/widgets/tooltip/SwatchColorPickerTooltip.js
@@ +56,4 @@
>      let eyedropper = doc.createElementNS(XHTML_NS, "button");
>      eyedropper.id = "eyedropper-button";
>      eyedropper.className = "devtools-button";
> +    eyedropper.style.pointerEvents = "auto";

Since this is only mandatory for disabled elements displayed in a XUL document (maybe not even all XUL documents?), could you add a comment here explaining why this is needed?
Attachment #8800134 - Flags: review?(jdescottes) → review+
Attached patch bug1305365.patch (obsolete) — Splinter Review
updated my previous patch.
Attachment #8800134 - Attachment is obsolete: true
Attachment #8800529 - Flags: review+
Attachment #8797724 - Attachment is obsolete: true
Your patch still doesn't apply cleanly. I think this is because browser_inspector_highlighter-eyedropper-xul.js is using windows-style line endings (I know...) but your patch file is missing them. Maybe you edited your patch file manually and the line endings got automatically translated by your editor?

I'll upload an alternate version of your patch that can be applied on central and we'll move on with this one.
Attached patch bug1305365.amend.patch (obsolete) — Splinter Review
Attachment #8800529 - Attachment is obsolete: true
Attachment #8801657 - Flags: review+
Adding checkin needed, this patch should apply cleanly on central.
Keywords: checkin-needed
See Also: → 1310615
has problems to apply:

Hunk #1 FAILED at 567
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/shared/widgets/tooltip/HTMLTooltip.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug1305365.working.patch
Flags: needinfo?(djmdeveloper060796)
Keywords: checkin-needed
Attachment #8801657 - Attachment is obsolete: true
Flags: needinfo?(djmdeveloper060796)
Attachment #8801677 - Flags: review+
Rebased on top of the latest central
Keywords: checkin-needed
Yeah I editted my patch manually and I used vim to do so.
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/61b5592b7df7
disabled eyedropper for non-HTML documents and added a tooltip;r=jdescottes
Keywords: checkin-needed
See Also: → 1310938
https://hg.mozilla.org/mozilla-central/rev/61b5592b7df7
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
I have reproduced this bug with Nightly 52.0a1 (2016-09-26) (64-bit) on Windows 7 , 64 Bit !

This bug's is now verified in Latest Nightly 52.0a1

Build ID    :  20161025030205
User Agent  :  Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:52.0) Gecko/20100101 Firefox/52.0

[bugday-20161026]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.