Closed Bug 1310938 Opened 8 years ago Closed 8 years ago

Disable toolbar eyedropper icon in non HTML documents

Categories

(DevTools :: Inspector, defect)

defect
Not set
normal

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, 5 obsolete files)

Follow up to Bug 1305365. 

In non HTML Documents we are now disabling the eyedropper icon displayed in the color picker tooltip. 

To be consistent we should do the same for the eyedropper icon located in the inspector toolbar. At the moment, the icon is simply hidden. The same title as in Bug 1305365 should also be used.

The icon is currently set to display:none in client/inspector/inspector.js [1].
The test updated in Bug 1305365 will also have to be updated here [2].

[1] http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/devtools/client/inspector/inspector.js#652

[2] http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/devtools/client/inspector/test/browser_inspector_highlighter-eyedropper-xul.js
Deepjyoti: would you like to take this bug as well since you already tackled Bug 1305365?
Flags: needinfo?(djmdeveloper060796)
Yeah sure!! I would like to work on this bug.
Flags: needinfo?(djmdeveloper060796)
Awesome, assigning the bug to you :)
I already gave some pointers in the summary, ping me if you need anything else to get started!
Assignee: nobody → djmdeveloper060796
Status: NEW → ASSIGNED
Attached patch bug1310938.patch (obsolete) — Splinter Review
I have disabled the eyedropper in the toolbar for non-html documents and updated the test file.
Attachment #8802220 - Flags: review?(jdescottes)
Comment on attachment 8802220 [details] [diff] [review]
bug1310938.patch

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

Thanks for the patch! On the right track, needs a slightly different approach from the previous patch though.
I'll let you have a look at my comments, ping me if you have any question!

::: devtools/client/inspector/inspector.js
@@ +649,4 @@
>          this.eyeDropperButton.addEventListener("click", this.onEyeDropperButtonClicked);
>        }, e => console.error(e));
>      } else {
> +      	let eyeDropperButton = this.panelDoc.getElementById("inspector-eyedropper-toggle");

nit: we indent using spaces. Syntax in devtools is validated using eslint. More information about this at https://wiki.mozilla.org/DevTools/CodingStandards

@@ +649,5 @@
>          this.eyeDropperButton.addEventListener("click", this.onEyeDropperButtonClicked);
>        }, e => console.error(e));
>      } else {
> +      	let eyeDropperButton = this.panelDoc.getElementById("inspector-eyedropper-toggle");
> +      	eyeDropperButton.style.display = "initial";

Toggling the display property is no longer needed. We actually always want the button to be displayed now.

This means:
- get rid of the display: none set in CSS on the eyedropper button
- remove all the code that updates the style of this button from inspector.js

@@ +652,5 @@
> +      	let eyeDropperButton = this.panelDoc.getElementById("inspector-eyedropper-toggle");
> +      	eyeDropperButton.style.display = "initial";
> +      	/* pointerEvents for eyeDropperButton has to be set auto to display tooltip when
> +     	 * eyeDropperButton is disabled in non-HTML documents.
> +     	 */

nit: remove this newline from the comment

@@ +654,5 @@
> +      	/* pointerEvents for eyeDropperButton has to be set auto to display tooltip when
> +     	 * eyeDropperButton is disabled in non-HTML documents.
> +     	 */
> +      	eyeDropperButton.style.pointerEvents = "auto";
> +      	eyeDropperButton.disabled = true;

The same instance of the inspector is reused when navigating from one page to another. This means that if you were on about:home (xul document) and go to http://mozilla.org (html document) the same inspector panel is actually reused.

onNewRoot() gets called, which calls setupToolbar().

In the if(){} block of this if/else statement, setupToolbar should be able to reenable the eyedropper button. This includes:
- setting disabled to false
- updating the title attribute to show the default "Grab a color from the page" (a localized string already exists for this text)

For now the title value is set in the markup, in inspector.xhtml. I'll let you check the file to see what needs to be removed there.

If you want to exercise this, just play the following STRs:
- go to about:home
- open the inspector
- (here the eyedropper should be disabled, its tooltip should be the disabled tooltip)
- navigate to http://mozilla.org
- (here the eyedropper should be enabled, its tooltip should be "Grab a color from the page")

::: devtools/client/inspector/test/browser_inspector_highlighter-eyedropper-xul.js
@@ +13,4 @@
>  
>    info("Check the inspector toolbar");
>    let button = inspector.panelDoc.querySelector("#inspector-eyedropper-toggle");
> +  ok(isDisabled(button), "The button is disabled in the toolbar");

Related to my comment about the implementation, it would be nice to extend the test in order to test the scenario where we navigate from a XUL document to a HTML document, but we can see that later!
Attachment #8802220 - Flags: review?(jdescottes)
Attached patch bug1310938.patch (obsolete) — Splinter Review
I have removed display:none from the css file and removed all code that updated style of the eyedropper button except the code where pointer-events is set to auto. Should I move it to css file?

In the if part of the setupToolbar function I have set disabled to false and title to the default one and removed the corresponding code from inspector.xhtml.
Attachment #8802220 - Attachment is obsolete: true
Attachment #8802417 - Flags: review?(jdescottes)
Comment on attachment 8802417 [details] [diff] [review]
bug1310938.patch

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

Still some issues when importing this patch. 
inspector.js: was the indentation of inspector.js changes manually updated in the patch file?
inspector.xhtml: rebase needed

This seems to work fine now, I would like us to try and update the test to cover the use case I mentioned. 
Pointers in the comments below.

::: devtools/client/inspector/inspector.js
@@ +652,5 @@
>      } else {
> +        let eyeDropperButton = this.panelDoc.getElementById("inspector-eyedropper-toggle");
> +        // PointerEvents for eyeDropperButton has to be set auto to display tooltip when
> +        // eyeDropperButton is disabled in non-HTML documents.
> +        eyeDropperButton.style.pointerEvents = "auto";

As you suggested, let's move this to the stylesheet!

::: devtools/client/inspector/test/browser_inspector_highlighter-eyedropper-xul.js
@@ +33,1 @@
>    ok(isDisabled(button), "The button is disabled in the color picker");

As I mentioned in my previous review, let's try to enhance this test.

What we should add here:
- navigate to another page (with a HTML document this time)
- get the eyedropper button from the toolbar
- check if it is enabled now

The tricky part here is to handle the navigation properly, so here is a code snippet that should work:

> info("Navigating to different URL.");
> let navigated = toolbox.target.once("navigate");
> let markuploaded = inspector.once("markuploaded");
> navigateTo(toolbox, TEST_URL_2);
> yield navigated;
> yield markuploaded; 

"toolbox" is in the object returned by openInspectorForURL(), we simply don't use it in this test for now. And TEST_URL_2 needs to be created. It can be data-uri, look at the ones used in the other eyedropper test for reference.
Attachment #8802417 - Flags: review?(jdescottes)
An additional note, right now we are disabling the eyedropper button `if (actor has eyedropper highlighter) and (document is not html)`. We enable the button `if (actor has eyedropper highlighter) and (document is html)`. But we don't do anything when the inspector is missing the eyedropper highlighter :) Ideally we should disable the eyedropper button also if the actor is missing the eyedropper highlighter.

We should fix that in a follow up bug. 

FYI: the actors are the part of devtools that allow us to interact with the content page. When debugging Firefox, the typical case, we can expect to have all the APIs we need. But when debugging various remote targets (other browsers, devices etc...) we might not be able to have the eyedropper feature, regardless of the nature of the document being inspected. We probably should update the disabled localized string to be more generic, or have an alternative one displayed only if the actor is missing the eyedropper highlighter.
Attached patch bug1310938_test.diff (obsolete) — Splinter Review
I have updated the test file to navigate to a html document and test the status of the eyedropper button in toolbar. But I feel that its not right, somewhere I am making mistake in the test file. I have attached the diff file containing the changes I have made to the test file.Do have a look.
Flags: needinfo?(jdescottes)
Comment on attachment 8803227 [details] [diff] [review]
bug1310938_test.diff

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

This looks good! You addressed all my comments and the test seems OK to me and runs fine locally. 

Any reason you feel unsure about it? Did you manage to run it yourself? 
The command should be ./mach test devtools/client/inspector/test/browser_inspector_highlighter-eyedropper-xul.js for this particular test.

Just got a few nits in the test file, but I'll r+ a properly formatted patch based on this diff!
In the meantime I pushed your diff to try https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f07ef73dd022c878f6b3a2440b43e144e0d518c

Thanks!

::: devtools/client/inspector/test/browser_inspector_highlighter-eyedropper-xul.js
@@ +7,4 @@
>  // when the page isn't an HTML one.
>  
>  const TEST_URL = URL_ROOT + "doc_inspector_highlighter_xbl.xul";
> +const TEST_URL_2 = "data:text/html;charset=utf-8,<h1>Add node</h1>";

nit: "Add node" is not relevant to this test, use something like "HTML test page" or whatever.

@@ +11,3 @@
>  
>  add_task(function* () {
> +  let {inspector,toolbox} = yield openInspectorForURL(TEST_URL);

nit: eslint failure here, add a space after "inspector," -> {inspector, toolbox}

@@ +32,5 @@
>  
>    button = cPicker.tooltip.doc.querySelector("#eyedropper-button");
>    ok(isDisabled(button), "The button is disabled in the color picker");
> +
> +  info("Navigating to a different URL");

nit: "Navigate to a HTML document"
ni? to you to upload your diff as a patch.
Flags: needinfo?(jdescottes) → needinfo?(djmdeveloper060796)
Attached patch bug1310938.patch (obsolete) — Splinter Review
Updated my previous patch.

runtests.py | Running tests: end.
TEST-INFO | checking window state
Browser Chrome Test Summary
	Passed: 0
	Failed: 0
	Todo: 0
	Mode: e10s
*** End BrowserChrome Test Results ***
SUITE-END | took 589s

Actually I am getting the above output after running the test on my machine. Seeing this (Passed:0 , failed:0 ) I thought perhaps its not right.
Attachment #8802417 - Attachment is obsolete: true
Attachment #8803227 - Attachment is obsolete: true
Flags: needinfo?(djmdeveloper060796)
Attachment #8803420 - Flags: review?(jdescottes)
The problem which we faced here that is on navigating from a xul document to a HTML document the toolbar eyedropper was remaining disabled, this same problem is also present in case of the colorpicker eyedropper. We should also take care of this issue.
Flags: needinfo?(jdescottes)
(In reply to Deepjyoti Mondal from comment #13)
> The problem which we faced here that is on navigating from a xul document to
> a HTML document the toolbar eyedropper was remaining disabled, this same
> problem is also present in case of the colorpicker eyedropper. We should
> also take care of this issue.

That's a great point! Thanks for catching this, for some reason I thought the tooltip was recreated every time ... 
Would you mind fixing this in this current bug as well? You can also extend the test to cover this use case. If you decide to do so, it would be nice to start mutualizing a bit the test code.

> Seeing this (Passed:0 , failed:0 ) I thought perhaps its not right. 

That is weird. You should see Passed: 3 here. Is the browser starting at all during the test? If you attach your full test log as an attachment I can maybe understand what's wrong.
Flags: needinfo?(jdescottes)
Comment on attachment 8803420 [details] [diff] [review]
bug1310938.patch

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

The patch is great, thanks! Clearing my review flag, let's address the issue you raised.

::: devtools/client/inspector/inspector.js
@@ +651,5 @@
>        }, e => console.error(e));
>      } else {
> +      let eyeDropperButton = this.panelDoc.getElementById("inspector-eyedropper-toggle");
> +      eyeDropperButton.disabled = true;
> +      eyeDropperButton.title = INSPECTOR_L10N.getStr("eyedropper.disabled.title");

As said above, it would be great if you could apply the same approach in SwatchColorPickerTooltip.js.
Attachment #8803420 - Flags: review?(jdescottes)
Attached patch bug1310938.patch (obsolete) — Splinter Review
I have updated SwatchColorPickerTooltip.js and added corresponding test.
Attachment #8803420 - Attachment is obsolete: true
Attachment #8803609 - Flags: review?(jdescottes)
I have ran the test on my machine after making the latest updates. Here is the log.
Flags: needinfo?(jdescottes)
Comment on attachment 8803609 [details] [diff] [review]
bug1310938.patch

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

Thanks for taking care of the colorpicker tooltip as well!

I looked at your test log, and I can see the test is timing out, not sure what's happening.
Do you see a Firefox window opening? The test should take a few seconds at most (with a non debug build).

The test is not working as it is. The #scale element is not available in the HTML document we navigate to. Instead we need to add a 'color' property on the h1 node and select it to make sure we will have a swatch element we can click in the ruleview. But since you can't run the tests for now, I can only imagine it's not easy to work on this :) I will fix the test and push an amended version of your patch.

(also: you should rebase on a more recent version of mozilla central. The test file has recently been cleaned up to only use linux-style linebreaks and your patch still has a mix of windows and unix linebreaks)

Thanks again, great job!

::: devtools/client/inspector/test/browser_inspector_highlighter-eyedropper-xul.js
@@ +7,4 @@
>  // when the page isn't an HTML one.
>  
>  const TEST_URL = URL_ROOT + "doc_inspector_highlighter_xbl.xul";
> +const TEST_URL_2 = "data:text/html;charset=utf-8,<h1>HTML test page</h1>";

replace this with

> const TEST_URL_2 =
>   "data:text/html;charset=utf-8,<h1 style='color:red'>HTML test page</h1>";

@@ +44,5 @@
> +  button = inspector.panelDoc.querySelector("#inspector-eyedropper-toggle");
> +  ok(!isDisabled(button), "The button is enabled in the toolbar");
> +
> +  info("Check the color picker in HTML document");
> +  yield selectNode("#scale", inspector);

replace this with 

> yield selectNode("h1", inspector);

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

Alternatively you could set the same title as in inspector.js
Attachment #8803609 - Flags: review?(jdescottes) → review+
Thanks for amending my patch :) . I will be updating my repo soon. I am currently running a debug build.How can I fix the time out problem ?. Should I disable the debug option?

I would love to work on the issue about which you mentioned in comment 8 :). Do let me know when you create the follow up bug
Flags: needinfo?(jdescottes)
Try looks green let's land this! 

(In reply to Deepjyoti Mondal from comment #20)
> Thanks for amending my patch :) . I will be updating my repo soon. I am
> currently running a debug build.How can I fix the time out problem ?. Should
> I disable the debug option?

You shouldn't have to. Do you see a firefox window starting when running the tests? Otherwise try to get some help in #developers on IRC (I am mostly away until nov 4, so can't help you much in the meantime sorry).

> 
> I would love to work on the issue about which you mentioned in comment 8 :).
> Do let me know when you create the follow up bug

Thanks :) I will log one later today and ping you!

Thanks again for your work here.
Flags: needinfo?(jdescottes)
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bf51de60223
disabled eyedropper in toolbar for non-HTML documents and added a tooltip;r=jdescottes
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4bf51de60223
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
See Also: → 1316613
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: