If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Disable eyedropper icon if the eyedropper highlighter is unavailable

RESOLVED FIXED in Firefox 53

Status

()

Firefox
Developer Tools: Inspector
P2
normal
RESOLVED FIXED
10 months ago
8 months ago

People

(Reporter: jdescottes, Assigned: Deepjyoti Mondal, Mentored)

Tracking

Trunk
Firefox 53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

10 months ago
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.
(Reporter)

Comment 1

10 months ago
Deepjyoti: Would you be interested in taking this one? (sorry I took so much time to open it).
Flags: needinfo?(djmdeveloper060796)
(Assignee)

Comment 2

10 months ago
Yup, I would like to work on this bug.
(Assignee)

Comment 3

10 months ago
clearing NI
Flags: needinfo?(djmdeveloper060796)
(Reporter)

Comment 4

10 months ago
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
(Assignee)

Comment 5

10 months ago
Created attachment 8810365 [details] [diff] [review]
bug1316613.patch

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)
(Reporter)

Comment 6

10 months ago
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)
(Reporter)

Comment 7

10 months ago
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)
(Assignee)

Comment 8

10 months ago
Thanks a lot for the pointers, I will submit my next patch soon.

Updated

10 months ago
Priority: -- → P2
(Assignee)

Comment 9

10 months ago
Created attachment 8812576 [details] [diff] [review]
bug1316613.patch

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)
(Reporter)

Comment 10

10 months ago
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+
(Reporter)

Comment 11

10 months ago
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)
(Reporter)

Updated

10 months ago
Blocks: 1306937
(Assignee)

Comment 12

10 months ago
Created attachment 8812767 [details] [diff] [review]
bug1316613.patch

I have removed extra spaces and split the long lines.
Attachment #8812576 - Attachment is obsolete: true
Attachment #8812767 - Flags: review?(jdescottes)
(Assignee)

Comment 13

10 months ago
 > 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)
(Reporter)

Comment 14

10 months ago
(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)
(Reporter)

Comment 15

10 months ago
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+
(Assignee)

Comment 16

10 months ago
cool :) I will be sending my next patch soon
(Reporter)

Comment 17

10 months ago
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.
(Assignee)

Comment 18

10 months ago
Created attachment 8813396 [details] [diff] [review]
bug1316613.patch

Added supporstEyeDropper function to check whether the document is HTML or not
Attachment #8812767 - Attachment is obsolete: true
Attachment #8813396 - Flags: review?(jdescottes)
(Reporter)

Comment 19

10 months ago
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+
(Assignee)

Comment 20

10 months ago
Sure I will do it soon :)
(Assignee)

Comment 21

10 months ago
Created attachment 8813866 [details] [diff] [review]
bug1316613.patch
Attachment #8813396 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8813866 - Flags: review+
(Reporter)

Comment 22

10 months ago
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

Comment 23

10 months ago
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
(Reporter)

Comment 24

10 months ago
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)
(Reporter)

Comment 25

10 months ago
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)
(Assignee)

Comment 26

10 months ago
> 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)

Comment 27

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3b322d58be09
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
I guess this will ride the train from 53
status-firefox52: affected → ---
You need to log in before you can comment on or make changes to this bug.