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)
DevTools
Inspector
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)
4.70 KB,
patch
|
djmdeveloper060796
:
review+
|
Details | Diff | Splinter Review |
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•8 years 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•8 years ago
|
||
Yup, I would like to work on this bug.
Reporter | ||
Comment 4•8 years 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•8 years ago
|
||
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•8 years 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•8 years 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•8 years ago
|
||
Thanks a lot for the pointers, I will submit my next patch soon.
Updated•8 years ago
|
Priority: -- → P2
Assignee | ||
Comment 9•8 years ago
|
||
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•8 years 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•8 years 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)
Assignee | ||
Comment 12•8 years ago
|
||
I have removed extra spaces and split the long lines.
Attachment #8812576 -
Attachment is obsolete: true
Attachment #8812767 -
Flags: review?(jdescottes)
Assignee | ||
Comment 13•8 years 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•8 years 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•8 years 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•8 years ago
|
||
cool :) I will be sending my next patch soon
Reporter | ||
Comment 17•7 years 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•7 years ago
|
||
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•7 years 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•7 years ago
|
||
Sure I will do it soon :)
Assignee | ||
Comment 21•7 years ago
|
||
Attachment #8813396 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8813866 -
Flags: review+
Reporter | ||
Comment 22•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3b322d58be09
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•