Closed Bug 1316613 Opened 8 years ago Closed 7 years ago

Disable eyedropper icon if the eyedropper highlighter is unavailable

Categories

(DevTools :: Inspector, defect, P2)

defect

Tracking

(firefox53 fixed)

RESOLVED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed

People

(Reporter: jdescottes, Assigned: djmdeveloper060796, Mentored)

References

Details

Attachments

(1 file, 4 obsolete files)

In Bug 1310938, the eyedropper icon was disabled on non-HTML documents.

When remote debugging a target for which the eyedropper highlighter is unavailable, the icon should also be disabled (see Bug 1310938 comment #8).

This logic should be modified both in SwatchColorPickerTooltip::show and in Inspector::setupToolbar.
Deepjyoti: Would you be interested in taking this one? (sorry I took so much time to open it).
Flags: needinfo?(djmdeveloper060796)
Yup, I would like to work on this bug.
clearing NI
Flags: needinfo?(djmdeveloper060796)
Great, thank you Deepjyoti! Assigning the bug to you.

Some STRs to help you get started. Here the issue occurs when trying to debug a remote target. For instance you can try to remote debug a Google Chrome instance by following the guide here: https://developer.mozilla.org/en-US/docs/Tools/Remote_Debugging/Chrome_Desktop.

If you try to debug a Chrome tab, you will see that the eyedropper icon is disabled in the toolbar but not in the color picker tooltip.

The code in the show() method of the color picker tooltip actually crashes (!) when checking for the existence of the pickColorFromPage method. For the inspector toolbar, we happen to go in the else statement here because this.selection.nodeFront.inInHTMLDocument is undefined.

If you compare the two implementations you will see that the inspector first checks for "isInHTMLDocument" and then checks if the pickColorFromPage method is available ; the color picker tooltip does it the other way around. Both implementations should be changed to be similar and to ensure the icon is disabled if any of the following happens:
- isInHTMLDocument is false
- pickColorFromPage is not available

I will investigate why target.actorHasMethod("inspector", "pickColorFromPage") is crashing in my scenario to see if we should handle it. 

One additional note: if you feel like using Tasks [1] for this, it could make the job easier (the show method in the tooltip is already wrapped in a Task so it should be easy to experiment with it there). 

Let me know if you have any question!

[1] https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Task.jsm
Assignee: nobody → djmdeveloper060796
Status: NEW → ASSIGNED
Attached patch bug1316613.patch (obsolete) — Splinter Review
I updated the show function, Now the eyedropper icon in the color picker is also disabled along with the icon in the inspector toolbar.

In the previous comment you have mentioned that the icon should be disabled only when :
isInHTMLDocument is false and pickColorFromPage is not available

so in the else part in the show function i had used the following code :

 else {
      target.actorHasMethod("inspector", "pickColorFromPage").then(value => {
        if (!value) {
          let tooltipDoc = this.tooltip.doc;
          let eyeButton = tooltipDoc.querySelector("#eyedropper-button");
          eyeButton.disabled = true;
          eyeButton.title = L10N.getStr("eyedropper.disabled.title");
        }
      }, e => console.log(e))
 }  

   
    

However it did not work out. The icon did not get disabled in the color picker tooltip.Is there any other way to check whether pickColorFromPage is available or not.
Flags: needinfo?(jdescottes)
Attachment #8810365 - Flags: review?(jdescottes)
Comment on attachment 8810365 [details] [diff] [review]
bug1316613.patch

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

Thanks for the patch! Please have a look at my comment below. Not sure which scenario you use to test here but remember that actorHasMethod is throwing errors when connecting to Chrome at the moment, so it can impact your testing.

::: devtools/client/shared/widgets/tooltip/SwatchColorPickerTooltip.js
@@ +98,5 @@
>      let {target} = this.inspector;
> +    if (this.inspector.selection.nodeFront.isInHTMLDocument) {
> +      target.actorHasMethod("inspector", "pickColorFromPage").then(value => {
> +        if (!value) {
> +      	  return;

If we hit this code path, we will not disable the icon. The goal is to make sure that we always either enable or disable the eyedropper. 

If you rewrite this method by taking advantage of the Task wrapper you can do the following:

> let isInHTMLDocument = this.inspector.selection.nodeFront.isInHTMLDocument;
> let pickColorAvailable = yield target.actorHasMethod("inspector", "pickColorFromPage");
> if (isInHTMLDocument && pickColorAvailable) {
>   // enable the eyedropper icon
> } else {
>   // disable the eyedropper icon
> }

I think this makes it easy to understand. The only issue is that in some cases, target.actorHasMethod(...) will crash with the current nightly. So for now we would have to do something a bit uglier such as :

> let pickColorAvailable = false;
> try {
>   pickColorAvailable = yield target.actorHasMethod("inspector", "pickColorFromPage");
> } catch (e) {
>   // Temporary workaround because actorHasMethod crashes with some clients.
> }

I would like to investigate a bit more the reasons that make actorHasMethod throw an error here, but in the meantime I think this can be acceptable.

Note that in inspector.js, the method you need to modify is not wrapped in a Task so you can't use this approach straight away. You first need to modify the method to use Task.async (see [1]). Other methods in inspector.js already use Task.async, so you should be able to use them as a reference.
 
[1] https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Task.jsm#async()

@@ +106,5 @@
> +      	let eyeButton = tooltipDoc.querySelector("#eyedropper-button");
> +      	eyeButton.disabled = false;
> +      	eyeButton.removeAttribute("title");
> +      	eyeButton.addEventListener("click", this._openEyeDropper);
> +      }, e => console.log(e))	

eslint: add missing semicolon, remove extra spaces.

@@ +112,4 @@
>        let tooltipDoc = this.tooltip.doc;
>        let eyeButton = tooltipDoc.querySelector("#eyedropper-button");
> +      eyeButton.disabled = true;
> +      eyeButton.title = L10N.getStr("eyedropper.disabled.title");	

eslint: extra spaces.
Attachment #8810365 - Flags: review?(jdescottes)
Clearing the needinfo (no need to add a needinfo when you ask for a review, the review request already acts as a notification :) )
Flags: needinfo?(jdescottes)
Thanks a lot for the pointers, I will submit my next patch soon.
Priority: -- → P2
Attached patch bug1316613.patch (obsolete) — Splinter Review
I have updated my previous patch, I have wrapped the setupToolbar in Task.async.
If this is alright then I will upload a formated patch.
Attachment #8810365 - Attachment is obsolete: true
Attachment #8812576 - Flags: review?(jdescottes)
Comment on attachment 8812576 [details] [diff] [review]
bug1316613.patch

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

Thanks for the new patch, looks good to me! 
Giving a r+ here, but if you want to try and implement my suggestion from the comments, feel free to re-flag for review.

I pushed your patch to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c43d652509486303f150ae8dbf517f03d8e7f4b
Let's see if the tests are still green with your changes!

::: devtools/client/inspector/inspector.js
@@ +639,5 @@
> +    let pickColorAvailable = false;
> +    try {
> +      pickColorAvailable = yield this.target.actorHasMethod("inspector", "pickColorFromPage");
> +    } catch (e) {
> +      console.log(e);

To be consistent with the previous implementation, `console.error(e)`

@@ +640,5 @@
> +    try {
> +      pickColorAvailable = yield this.target.actorHasMethod("inspector", "pickColorFromPage");
> +    } catch (e) {
> +      console.log(e);
> +    }

If you feel like it, we could we add a new method on the inspector class to mutualize this logic between inspector.js and SwatchColorPickerTooltip.js.

Something along the lines of 
> supportsEyeDropper: Task.async(function* () {
>   // check selection is in HTML doc and pickColorFromPage is available
> }),

Up to you! (otherwise I'll probably do it in Bug 1306937)

@@ +649,4 @@
>                                      .getElementById("inspector-eyedropper-toggle");
> +      this.eyeDropperButton.disabled = false;
> +      this.eyeDropperButton.title = INSPECTOR_L10N.getStr("inspector.eyedropper.label");
> +      this.eyeDropperButton.addEventListener("click", this.onEyeDropperButtonClicked);      

nit: extra space to remove

@@ +653,4 @@
>      } else {
>        let eyeDropperButton = this.panelDoc.getElementById("inspector-eyedropper-toggle");
>        eyeDropperButton.disabled = true;
> +      eyeDropperButton.title = INSPECTOR_L10N.getStr("eyedropper.disabled.title");      

nit: extra space to remove

::: devtools/client/shared/widgets/tooltip/SwatchColorPickerTooltip.js
@@ +102,5 @@
> +    let pickColorAvailable = false;
> +    try {
> +      pickColorAvailable = yield target.actorHasMethod("inspector", "pickColorFromPage");
> +    } catch (e) {
> +      console.log(e);

console.error(e)
Attachment #8812576 - Flags: review?(jdescottes) → review+
Comment on attachment 8812576 [details] [diff] [review]
bug1316613.patch

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

Some additional eslint failures detected on try.

::: devtools/client/inspector/inspector.js
@@ +635,4 @@
>      this.addNodeButton.addEventListener("click", this.addNode);
>  
>      // Setup the eye-dropper icon if we're in an HTML document and we have actor support.
> +    let isInHTMLDocument = this.selection.nodeFront && this.selection.nodeFront.isInHTMLDocument;

nit: line too long (90 chars max)

@@ +637,5 @@
>      // Setup the eye-dropper icon if we're in an HTML document and we have actor support.
> +    let isInHTMLDocument = this.selection.nodeFront && this.selection.nodeFront.isInHTMLDocument;
> +    let pickColorAvailable = false;
> +    try {
> +      pickColorAvailable = yield this.target.actorHasMethod("inspector", "pickColorFromPage");

nit: line too long (90 chars max)
Blocks: 1306937
Attached patch bug1316613.patch (obsolete) — Splinter Review
I have removed extra spaces and split the long lines.
Attachment #8812576 - Attachment is obsolete: true
Attachment #8812767 - Flags: review?(jdescottes)
 > If you feel like it, we could we add a new method on the inspector class to
> mutualize this logic between inspector.js and SwatchColorPickerTooltip.js.
> 
> Something along the lines of 
> > supportsEyeDropper: Task.async(function* () {
> >   // check selection is in HTML doc and pickColorFromPage is available
> > }),

So you are suggesting to do something like this :

supportsEyeDropper: Task.async(function* () {
    let isInHTMLDocument = this.selection.nodeFront &&
                            this.selection.nodeFront.isInHTMLDocument;
    let pickColorAvailable = false;
    try {
      pickColorAvailable = yield this.target
                                      .actorHasMethod("inspector", "pickColorFromPage");
    } catch (e) {
      console.error(e);
    }
    if (isInHTMLDocument && pickColorAvailable) {
      return true;
    } else {
      return false;
    }

 }),

create this function in inspector.js and SwatchColorPickerTooltip.js and then use this function to check whether the document is html or not
Flags: needinfo?(jdescottes)
(In reply to Deepjyoti Mondal from comment #13)
>  > If you feel like it, we could we add a new method on the inspector class
> to
> > mutualize this logic between inspector.js and SwatchColorPickerTooltip.js.
> > 
> > Something along the lines of 
> > > supportsEyeDropper: Task.async(function* () {
> > >   // check selection is in HTML doc and pickColorFromPage is available
> > > }),
> 
> So you are suggesting to do something like this :
> 
> supportsEyeDropper: Task.async(function* () {
>     let isInHTMLDocument = this.selection.nodeFront &&
>                             this.selection.nodeFront.isInHTMLDocument;
>     let pickColorAvailable = false;
>     try {
>       pickColorAvailable = yield this.target
>                                       .actorHasMethod("inspector",
> "pickColorFromPage");
>     } catch (e) {
>       console.error(e);
>     }
>     if (isInHTMLDocument && pickColorAvailable) {
>       return true;
>     } else {
>       return false;
>     }
> 
>  }),
> 
> create this function in inspector.js and SwatchColorPickerTooltip.js and
> then use this function to check whether the document is html or not

The idea would be to create this method only in inspector.js and reuse it in SwatchColorPickerTooltip.js. We already grab a reference to the inspector panel in the tooltip (this.inspector).

>     if (isInHTMLDocument && pickColorAvailable) {
>       return true;
>     } else {
>       return false;
>     }

Can be simplified as

> return isInHTMLDocument && pickColorAvailable;
Flags: needinfo?(jdescottes)
Comment on attachment 8812767 [details] [diff] [review]
bug1316613.patch

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

Looks good, thanks! 
Flag me for review again if you want to implement what we discussed on the side.
Attachment #8812767 - Flags: review?(jdescottes) → review+
cool :) I will be sending my next patch soon
Comment on attachment 8812767 [details] [diff] [review]
bug1316613.patch

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

Keeping r+ , make sure to address this last comment in your next patch and sorry for missing it in my earlier review!

::: devtools/client/shared/widgets/tooltip/SwatchColorPickerTooltip.js
@@ +111,5 @@
> +      eyeButton.addEventListener("click", this._openEyeDropper);
> +    } else {
> +      eyeButton.disabled = true;
> +      eyeButton.title = L10N.getStr("eyedropper.disabled.title");
> +    }

Missing this.emit("ready"); at the end of this method.
Attached patch bug1316613.patch (obsolete) — Splinter Review
Added supporstEyeDropper function to check whether the document is HTML or not
Attachment #8812767 - Attachment is obsolete: true
Attachment #8813396 - Flags: review?(jdescottes)
Comment on attachment 8813396 [details] [diff] [review]
bug1316613.patch

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

Looks good, thank you for the patch! 
Just a small nit, feel free to directly set r+ on your new patch.

Pushed your changeset to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5cd661c3b96d333fa0b890753ced7415a1820d3

::: devtools/client/inspector/inspector.js
@@ +612,4 @@
>      this.sidebar.addTab(id, title, panel, selected);
>    },
>  
> +  supportsEyeDropper: Task.async(function* () {

nit: can you add a small JS doc for this new method? Thanks!
Attachment #8813396 - Flags: review?(jdescottes) → review+
Sure I will do it soon :)
Attached patch bug1316613.patchSplinter Review
Attachment #8813396 - Attachment is obsolete: true
Attachment #8813866 - Flags: review+
Sorry Deepjyoti, I forgot to follow up on this one! 

I pushed to try your last patch, I'll add checkin needed if try is green. 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=68e24492be47973526329914022726cce646735d
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b322d58be09
Disables eyedropper icon if the eyedropper highlighter is unavailable.r=jdescottes
Try looks good. I pushed your changeset on inbound directly because I needed to slightly amend the commit message (missing a space between bug & the bug number)
Thanks for working on this bug Deepjyoti!

You've already done a few contributions, would you like to request for commit access level 1 ? 
It would allow you to push your changesets to try and to use mozreview for a more streamlined review process.

If you are interested, have a look at the doc at https://www.mozilla.org/en-US/about/governance/policies/commit/ .
Basically to generate a SSH key and file a bug to get access, provided someone can vouch for you (I'd be happy to do that).

You can also have a look at the mozreview documentation at : http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview.html
Flags: needinfo?(djmdeveloper060796)
> You've already done a few contributions, would you like to request for
> commit access level 1 ? 
> It would allow you to push your changesets to try and to use mozreview for a
> more streamlined review process.
>
Thanks a lot for asking :) Yeah I am interested, I will soon go through the documentations you mentioned above.
Flags: needinfo?(djmdeveloper060796)
https://hg.mozilla.org/mozilla-central/rev/3b322d58be09
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
I guess this will ride the train from 53
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: