Closed Bug 1306937 Opened 8 years ago Closed 8 years ago

inspector-eyedropper-toggle should be enabled in an image file opened

Categories

(DevTools :: Inspector, defect, P2)

Product:
DevTools
Component:
Inspector
Type:
defect

Tracking

(firefox49 unaffected, firefox50 unaffected, firefox51 wontfix, firefox52 wontfix, firefox53 fixed)

Status:
VERIFIED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox49 --- unaffected
firefox50 --- unaffected
firefox51 --- wontfix
firefox52 --- wontfix
firefox53 --- fixed

People

(Reporter: magicp.jp, Assigned: jdescottes)

References

Details

Attachments

(2 files)

Bug 1306937 - enable inspector eyedropper when viewing images;
(deleted), text/x-review-board-request
pbro
: review+
Details
Bug 1306937 - use XHTML NS by default when creating highlighter nodes;
(deleted), text/x-review-board-request
pbro
: review+
Details
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0 Build ID: 20161001030430 Steps to reproduce: 1. Start Nightly 2. File > Open File... any image files (e.g. SVG, PNG) or View image in any sites 3. Open DevTools > Inspector 4. Check inspector-eyedropper-toggle Actual results: inspector-eyedropper-toggle is not displayed. Regression range: https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=b7e8b15d90da87ca0491b9515ca8640f97ef132e&tochange=f2b99a4da3cf6afb8083eb8ae6ae7df75993bf93 Expected results: inspector-eyedropper-toggle should be enabled in an image file opened.
Blocks: 1289543
Has Regression Range: --- → yes
Has STR: --- → yes
status-firefox49: --- → unaffected
status-firefox50: --- → unaffected
status-firefox51: --- → affected
status-firefox52: --- → affected
Component: Untriaged → Developer Tools: Inspector
OS: Unspecified → All
Hardware: Unspecified → All
Since bug 1262437, the eyedropper is actually a "highlighter", which means that it can only work in HTML documents. In fact, in devtools/client/inspector/inspector-panel.js we check if the document is HTML to decide whether the icon should be shown or hidden. Now, in case the HTTP response is an image (e.g. content-type "image/png"), then Firefox does something special where it injects an HTML page to present the image nicely rather than just simply showing the image. So, the document is indeed HTML, but the content-type is "image/png", and since our code uses content-type, then we just assume the document isn't HTML, and the icon is hidden. http://searchfox.org/mozilla-central/rev/d1276b5b84e6cf7991c8e640b5e0ffffd54575a6/devtools/server/actors/inspector.js#279-280
Inspector bug triage (filter on CLIMBING SHOES).
Priority: -- → P3
I hit this bug with the following use case: I got a Design from our Layouter. Wanted to quickly get the color (e.g. #000000) of some piece in there. Thought, to just quickly use the Evesdropper for this, since firefox was open anyway. So opened the file (png) in Firefox, but then: No eyedropper :(
(In reply to Jens from comment #5) > I hit this bug with the following use case: > > I got a Design from our Layouter. Wanted to quickly get the color (e.g. > #000000) of some piece in there. Thought, to just quickly use the > Evesdropper for this, since firefox was open anyway. So opened the file > (png) in Firefox, but then: No eyedropper :( This sounds like a very common use case that devtools should help with. The eyedropper really ought to work on image files. Especially considering that the page is HTML, this should be a rather easy fix.
Priority: P3 → P2
The eyedropper is toggled here: http://searchfox.org/mozilla-central/rev/886d5186f0598ab2f8a9953eb5a4dab9750ef834/devtools/client/inspector/inspector.js#635-649 If we detect that the current node isn't inside an HTML document, we hide the eyedropper icon. The isInHTMLDocument property is set on the server side here: http://searchfox.org/mozilla-central/rev/886d5186f0598ab2f8a9953eb5a4dab9750ef834/devtools/server/actors/inspector.js#285-286 So, using this piece of code: node.ownerDocument && node.ownerDocument.contentType === "text/html" In the case of an ImageDocument, Firefox does generate an HTML document, but keeps the contentType as "image/png" (or similar), so this test evaluates to false. So, just changing this condition should fix the problem. Perhaps checking that node.ownerDocument.documentElement is indeed an <html> node would be better than checking the contentType? Julian: what do you think about this?
Flags: needinfo?(jdescottes)
Works for me! The only other place where isInHTMLDocument is used is to check if we should show closing tags or not. No reason to treat the HTML document generated for the image viewer any differently for this feature either. Attached a quick patch+test.
Flags: needinfo?(jdescottes)
Comment on attachment 8810375 [details] Bug 1306937 - enable inspector eyedropper when viewing images; https://reviewboard.mozilla.org/r/92730/#review92696 Works for me. Thanks for the quick patch!
Attachment #8810375 - Flags: review?(pbrosset) → review+
Thanks for the review Patrick, try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b8eaee714beaee9e2672b6e15dd02a05c0c6109
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Of course this means we are now treating XHTML documents the same way as HTML documents (and browser_markup_void_elements_xhtml.js consequently fails) ... I'll think a bit more about this and submit another patch for review. Sorry about this.
In the end we cannot rely on the same logic to check if the eyedropper can be used and if we should hide/show the self closing tags. I decided to leave the isInHTMLDocument property of the NodeActor untouched here (even though I think it would make more sense to simply return the content-type and let the client test the content-type directly). I added a new method on the inspector actor "supportsAnonymousContent" which simply checks if the main document of the current tabActor is not XUL. I have a try run ongoing at https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f3354c6bb519dc39dee62ba5699d95e40bf785a&selectedJob=31129942 and will re-flag for review after that, but feel free to let me know what you think about this alternative approach.
Flags: needinfo?(pbrosset)
Comment on attachment 8810375 [details] Bug 1306937 - enable inspector eyedropper when viewing images; A few intermittents, but I've seen them on other unrelated try pushes so I don't think they are related to my patch. Flagging for re-review.
Flags: needinfo?(pbrosset)
Attachment #8810375 - Flags: review+ → review?(pbrosset)
Comment on attachment 8810375 [details] Bug 1306937 - enable inspector eyedropper when viewing images; https://reviewboard.mozilla.org/r/92730/#review93760 Thanks for the patch, and sorry about the delay. ::: devtools/client/inspector/inspector.js:634 (Diff revision 4) > // Setup the add-node button. > this.addNode = this.addNode.bind(this); > this.addNodeButton = this.panelDoc.getElementById("inspector-element-add-button"); > this.addNodeButton.addEventListener("click", this.addNode); > > - // Setup the eye-dropper icon if we're in an HTML document and we have actor support. > + // Setup the eye-dropper icon if we can use anonmous content and have actor support. s/anonmous/anonymous Also, you're just calling `supportsEyeDropper()` here, so let's maybe remove that comment here. The jsdoc above that function should be enough, and there are chances it's implementation can change in the future and us forgetting to update this line comment. ::: devtools/client/inspector/inspector.js:662 (Diff revision 4) > + * Returns a promise that resolves true if the eyedropper highlighter can be used on the > + * current target, false otherwise. > + */ > + supportsEyeDropper: Task.async(function* () { > + try { > + let supportsAnonymousContent = yield this.inspector.supportsAnonymousContent(); We need backward compat here too. You're calling a new method that might not exist depending on the server you connect to. So, in fact, if you connect to an old server that does not have this method, then we should still use `isInHTMLDocument`, right? So the eyedropper is still displayed in those cases. ::: devtools/server/actors/inspector.js:2840 (Diff revision 4) > } > }, > > + /** > + * Check if native anonymous content can be used with the current document. Native > + * anonymous content is not available if the main document is a XUL document. But also if the document is SVG. It would be easier to test if document.insertAnonymousContent existed, but as it turns out, it seems to always be defined unfortunately. Maybe we could call `let content = doc.insertAnonymousContent(node)` with some test div and then remove it again with `doc.removeAnonymousContent(content)` and do this within a try/catch to detect documents that don't support it. ::: devtools/server/actors/inspector.js:2842 (Diff revision 4) > > + /** > + * Check if native anonymous content can be used with the current document. Native > + * anonymous content is not available if the main document is a XUL document. > + */ > + supportsAnonymousContent: function () { I fear the name might be a bit misleading. Anonymous content could mean other things than what we imply here. What we really want to know is whether the current document has the insertAnonymousContent API. So maybe `supportsAnonymousContentInsertion` ?
Attachment #8810375 - Flags: review?(pbrosset)
Thanks for the review! For the time being I am blocking this bug on Bug 1316613 which will start introducing Tasks in the same way as this bug. It should land soon so I'll resume my work on this one right after.
Depends on: 1316613
Comment on attachment 8810375 [details] Bug 1306937 - enable inspector eyedropper when viewing images; https://reviewboard.mozilla.org/r/92730/#review93760 > But also if the document is SVG. > It would be easier to test if document.insertAnonymousContent existed, but as it turns out, it seems to always be defined unfortunately. > Maybe we could call `let content = doc.insertAnonymousContent(node)` with some test div and then remove it again with `doc.removeAnonymousContent(content)` and do this within a try/catch to detect documents that don't support it. I actually reused the logic that was used to decided between regular highlighters and simple outline highlighters. As you can see in my new implementation, the test actually needs to check if a canvas context can be used here. We *could* reuse this to decide between regular and simple-outline highlighters and consequently get a selection highlighter when inspecting SVG documents. > I fear the name might be a bit misleading. Anonymous content could mean other things than what we imply here. > What we really want to know is whether the current document has the insertAnonymousContent API. > So maybe `supportsAnonymousContentInsertion` ? What about supportsHighlighters (since we are testing more than just insertion it's hard to find a great name here. Maybe supportsCanvasHighlighters?
try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=0860e38e4a60d7ea9cb82274970370fa29424743 (This patch is based on the patch from Bug 1316613 which is only on inbound at the moment)
As discussed on IRC: we have two document types that do not support highlighters at the moment: - XUL (can't use insertAnonymousContent) - SVG (canvasFrame not rendered) I couldn't find a real way to detect the SVG case, so I ended up simply checking for the document namespace. The second changeset makes our createNode utility use createElementNS + XHTML_NS by default. This avoids errors when inserting highlighters in non HTML documents (for instance the eyedropper now works when viewing XML documents, even though I doubt it's super useful).
Comment on attachment 8810375 [details] Bug 1306937 - enable inspector eyedropper when viewing images; https://reviewboard.mozilla.org/r/92730/#review96754 ::: devtools/server/actors/inspector.js:2812 (Diff revision 6) > events.off(this.tabActor, "will-navigate", this.destroyEyeDropper); > } > }, > > + /** > + * Check if the current document supports canvas based highlighters. nit: so what do you mean by `canvas` in this comment? HTML <canvas> element, or canvasFrame? It might be worth adding more details in here to summarize what we discussed on IRC a little bit.
Attachment #8810375 - Flags: review?(pbrosset) → review+
Comment on attachment 8815680 [details] Bug 1306937 - use XHTML NS by default when creating highlighter nodes; https://reviewboard.mozilla.org/r/96540/#review96756
Attachment #8815680 - Flags: review?(pbrosset) → review+
Thanks for the review! > what do you mean by `canvas` in this comment? HTML <canvas> element, or canvasFrame? I updated the comment, hopefully that's clear enough now. Try at : https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f0ad5ed0ecd0491a63e01aaf73bbd30f8292bca Looks green, landing.
Pushed by gszorc@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f0cbd37fec30 enable inspector eyedropper when viewing images;r=pbro https://hg.mozilla.org/integration/autoland/rev/db231dc73bf4 use XHTML NS by default when creating highlighter nodes;r=pbro
https://hg.mozilla.org/mozilla-central/rev/f0cbd37fec30 https://hg.mozilla.org/mozilla-central/rev/db231dc73bf4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
I have reproduced this bug with Nightly 52.0a1 (2016-10-01) on Windows 8.1 , 64 Bit ! Build ID 20161001030430 User Agent Mozilla/5.0 (Windows NT 6.3; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0 This bug's fix is verified with latest Nightly ! Build ID 20161204030210 User Agent Mozilla/5.0 (Windows NT 6.3; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0 [bugday-20161130]
Status: RESOLVED → VERIFIED
Too late for 51. Mark 51 won't fix.
status-firefox51: affected → wontfix
I guess we want this to ride the train with 53.
status-firefox52: affected → wontfix
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: