Taking screenshot of node with multi-part selector fails

RESOLVED FIXED in Firefox 41

Status

RESOLVED FIXED
4 years ago
9 months ago

People

(Reporter: Pike, Assigned: dbryant, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 42

Firefox Tracking Flags

(firefox40 unaffected, firefox41 fixed, firefox42 fixed)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

4 years ago
When trying to take a screenshot of an svg node via the context menu, things break.

The first thing in the error console is

Error: Zu viele Argumente
Stack-Trace:
Requisition.prototype.exec@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/cli.js:2062:14
Requisition.prototype.updateExec/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/cli.js:2112:12
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:920:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:799:7
this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:738:39
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:738:7
this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:762:7
Promise.prototype.then@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:445:5
CommandUtils.createRequisition@resource:///modules/devtools/DeveloperToolbar.jsm:73:12
InspectorPanel.prototype.screenshotNode@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/inspector/inspector-panel.js:1100:5
oncommand@chrome://browser/content/devtools/inspector/inspector.xul:1:1
 cli.js:2044

Taking a screenshot of the html parent node works OK.
(In reply to Axel Hecht [:Pike] from comment #0)

> Taking a screenshot of the html parent node works OK.

Can you provide a url and ideally the node you tried to screenshot? Really helps us quickly reproduce the issue.
Flags: needinfo?(l10n)
(Reporter)

Comment 2

4 years ago
The svg node in http://people.mozilla.org/~axel/l10n-updates/?locale=sl.

Sadly, I can't really promise that that is stable content.
Flags: needinfo?(l10n)
I was able to reproduce the issue.

The screenshot command is here:
https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/gcli/commands/screenshot.js#267
In particular, the line I linked to is where the size and position of the node is determined.

But the problems seems to be coming from when the inspector-panel (where the contextual menu is) calls that command. For some reason, gcli complains that "too many arguments" are passed.
This is where this happens:
https://dxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/inspector-panel.js#1132

My guess is that the unique selector we get for that svg node contains spaces and since we don't enclose the --selector argument in quotes, gcli thinks there are several arguments.
Mentor: pbrosset
Whiteboard: [good first bug][lang=js]
Component: Developer Tools → Developer Tools: Inspector
(Assignee)

Comment 4

4 years ago
This is my first patch, which came about through the Mozilla Weekend event in Berlin (with exceptional guidance from :pike)
Assignee: nobody → dbryant
Status: NEW → ASSIGNED
Attachment #8632532 - Flags: review?(pbrosset)
Taking over for Patrick, since he's out for a bit.
Mentor: pbrosset → jryans
Attachment #8632532 - Flags: review?(pbrosset) → review?(jryans)
Comment on attachment 8632532 [details] [diff] [review]
Screenshot parameter as single string

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

Great, this appears to fix the issue!

However, I think your patch may not be formatted quite right, as I wasn't apply it as a commit ("patch -p1 < file" worked for testing though).

See the FAQ[1] for some help with this.  There are tips for both Mercurial and Git users.

Also, see the commit message conventions[2].  For the summary line of the commit, something like:

Bug 1180314 - Screenshot parameter as single string. r=jryans

would work here.

Once you've fixed these issues, flag me for review again, and I'll run it on Try assuming everything looks good.

[1]: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
[2]: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#Commit_Message_Conventions

::: browser/devtools/inspector/inspector-panel.js
@@ +1129,4 @@
>      CommandUtils.createRequisition(this._target, {
>        environment: CommandUtils.createEnvironment(this, '_target')
>      }).then(requisition => {
> +      // Bug #1180314: CssSelector might contain white space so need to make sure it is

Super nit: I think it's more common to omit the "#", but not really a big deal.
Attachment #8632532 - Flags: review?(jryans) → feedback+
(Reporter)

Comment 7

4 years ago
Ryan, David and I were wondering about the use of "'" without changing the given selector, too.

Our hope is that the "create unique selector" doesn't create attribute value selectors, and thus we'd be fine.

Can you confirm?

David, the docs suggest to do

git format-patch -k -p ...

I think you want to specify 'master' for ..., but I'm not sure. https://github.com/mozilla/gecko-dev/branches/active indicates that fx-team is one of the popular branches there, and we patched against master.
Flags: needinfo?(jryans)
(In reply to Axel Hecht [:Pike] from comment #7)
> Ryan, David and I were wondering about the use of "'" without changing the
> given selector, too.
> 
> Our hope is that the "create unique selector" doesn't create attribute value
> selectors, and thus we'd be fine.
> 
> Can you confirm?

It seems we don't use attribute selectors currently to make unique selectors[1], so I believe the quotes are safe.

Selectors currently used:

* #id
* .class
* div
* :nth-child(0)

[1]: https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/styleinspector/css-logic.js#947
Flags: needinfo?(jryans)
If we wanted to be future-proof and extra safe, we could probably make use of CSS.escape[1] here, but not sure if the screenshot command correctly handles the escaped version.

[1]: https://developer.mozilla.org/en-US/docs/Web/API/CSS/escape
(Assignee)

Comment 10

4 years ago
Revised patch created for HG using moz-git-tools and helpful explanations in various comments on this bug.
Attachment #8632532 - Attachment is obsolete: true
Attachment #8633812 - Flags: review?(jryans)
(Assignee)

Comment 11

4 years ago
Thanks for all the great help & coaching.  I've modified the commit message to follow our style guide, and generated an HG-compatible patch via 'git format-patch -k -p origin' and the git-patch-to-hg-patch tool from moz-git-tools.  The new patch is attached, with the original (mangled) one obsoleted.

As Axel pointed out, we contemplated the completeness of the "'" solution given W3C specified syntax for CSS selectors but weren't sure what mechanism to use to properly and fully escape selectors so as to guarantee a single parameter string that would work with the screenshot command.  CSS.escape seems good on the generating end but would need to dig into the screenshot command to see how it works and perhaps make modifications there.  Perhaps a separate bug for that??  LMK.

Comment 12

4 years ago
Comment on attachment 8633812 [details] [diff] [review]
Bug 1180314 - Screenshot parameter as single string

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

::: browser/devtools/inspector/inspector-panel.js
@@ +1129,4 @@
>      CommandUtils.createRequisition(this._target, {
>        environment: CommandUtils.createEnvironment(this, '_target')
>      }).then(requisition => {
> +      // Bug #1180314: CssSelector might contain white space so need to make sure it is

nit : Please use Bug XXX instead of #XXX (as mentioned before)
(Assignee)

Comment 13

4 years ago
Ooops -- sorry I didn't catch the change needed in the comment when I modified the patch proper.  Out of curiosity, do we have tools that scan for references to bug numbers in comments in code/patches?  Or is this a convention we have for human readers/reviewers?  It's good for me to understand all the pieces that come into place to get a code change through and into the system.  Thanks!

And should I make that change here and resubmit the patch + attachment?

Comment 14

4 years ago
(In reply to David Bryant [:dbryant] from comment #13)
> Ooops -- sorry I didn't catch the change needed in the comment when I
> modified the patch proper.  Out of curiosity, do we have tools that scan for
> references to bug numbers in comments in code/patches?  Or is this a
> convention we have for human readers/reviewers?  It's good for me to
> understand all the pieces that come into place to get a code change through
> and into the system.  Thanks!
Not really, it's just to be consistent with other bug references in our source.

> And should I make that change here and resubmit the patch + attachment?
Yep, you should do that.
(Reporter)

Comment 15

4 years ago
Apparently dxr does, https://dxr.mozilla.org/mozilla-central/source/accessible/base/FocusManager.cpp#59. Not sure what specific pattern it uses, though. Didn't find any #123456 references ad-hoc, so might be good to change it.
Comment on attachment 8633812 [details] [diff] [review]
Bug 1180314 - Screenshot parameter as single string

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

Great, the patch applies correctly now!

As ntim said, let's revise it one more time to fix the nit on the bug number.  I believe the main automated tool is DXR, like Axel mentioned.  Our Mercurial history applies similar rules to the commit messages as well, but that one's correct already.

I think it's safe to stick with just quotes for now given the selector strings we generate at the moment.
Attachment #8633812 - Flags: review?(jryans) → feedback+
(Assignee)

Comment 17

4 years ago
Updating the code and patch to remove the offending, non-standard '#' character I used in the comment identifying the code change as being associated with this particular bug.  (Kill # with fire :-)
Attachment #8633812 - Attachment is obsolete: true
Attachment #8634505 - Flags: review?(jryans)

Comment 18

4 years ago
(In reply to David Bryant [:dbryant] from comment #17)
> Created attachment 8634505 [details] [diff] [review]
> Bug 1180314 - Screenshot parameter as single string
> 
> Updating the code and patch to remove the offending, non-standard '#'
> character I used in the comment identifying the code change as being
> associated with this particular bug.  (Kill # with fire :-)

Looks like you have failed to do so ;) I recommend checking your patch after uploading it, I sometimes end up uploading a blank/older patch.
Comment on attachment 8634505 [details] [diff] [review]
Bug 1180314 - Screenshot parameter as single string

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

Right, like ntim says, this appears to be same as the last one.
Attachment #8634505 - Flags: review?(jryans)
(Assignee)

Comment 20

4 years ago
Revised^2 patch attached (which I'll check after attaching).  Third time's the charm.
Attachment #8634505 - Attachment is obsolete: true
Attachment #8634873 - Flags: review?(jryans)
Comment on attachment 8634873 [details] [diff] [review]
Bug 1180314 - Screenshot parameter as single string

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

Great, looks good to me!

I've pushed this to Try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6ddc5702ae9

Assuming the tests pass, we can mark the bug for landing with the keyword checkin-needed.
Attachment #8634873 - Flags: review?(jryans) → review+
The tests look good, let's mark this for landing.

Thanks again for working on it!
Keywords: checkin-needed
Maybe it's worth uplifting this to DE as well?  Jeff, any thoughts?
Flags: needinfo?(jgriffiths)
Summary: Taking screenshot of SVG node is fails. → Taking screenshot of node with multi-part selector fails
https://hg.mozilla.org/mozilla-central/rev/ea364b2e14d5
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42

Comment 26

4 years ago
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #24)
> Maybe it's worth uplifting this to DE as well?  Jeff, any thoughts?

It's definitively worth it as we don't want to ship a broken feature, also, this patch isn't too risky to uplift. I got caught by this bug a few times already (even on HTML content).
Comment on attachment 8634873 [details] [diff] [review]
Bug 1180314 - Screenshot parameter as single string

Approval Request Comment
[Feature/regressing bug #]: New screenshot node feature (bug 1163332) in 41
[User impact if declined]: Screenshot fails for some elements
[Describe test coverage new/current, TreeHerder]: On m-c
[Risks and why]: Low, DevTools inspector only
[String/UUID change made/needed]: None
Flags: needinfo?(jgriffiths)
Attachment #8634873 - Flags: approval-mozilla-aurora?
status-firefox40: --- → unaffected
status-firefox41: --- → affected
Comment on attachment 8634873 [details] [diff] [review]
Bug 1180314 - Screenshot parameter as single string

Seems like a safe and simple patch. Let's land it on Aurora channel.
Attachment #8634873 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

9 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.