1180314 - Taking screenshot of node with multi-part selector fails

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Axel Hecht [:Pike]]

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.

Comment 1

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(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.

Comment 2

[Axel Hecht [:Pike]]

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

Sadly, I can't really promise that that is stable content.

Comment 3

[Patrick Brosset <:pbro>]

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.

Comment 4

[David Bryant [:dbryant]]

Attachment: Screenshot parameter as single string

This is my first patch, which came about through the Mozilla Weekend event in Berlin (with exceptional guidance from :pike)

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Taking over for Patrick, since he's out for a bit.

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 7

[Axel Hecht [:Pike]]

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.

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(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

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 10

[David Bryant [:dbryant]]

Attachment: Bug 1180314 - Screenshot parameter as single string

Revised patch created for HG using moz-git-tools and helpful explanations in various comments on this bug.

Comment 11

[David Bryant [:dbryant]]

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

[Tim Nguyen :ntim]

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)

Comment 13

[David Bryant [:dbryant]]

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

[Tim Nguyen :ntim]

(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.

Comment 15

[Axel Hecht [:Pike]]

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 16

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 17

[David Bryant [:dbryant]]

Attachment: 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 :-)

Comment 18

[Tim Nguyen :ntim]

(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 19

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 20

[David Bryant [:dbryant]]

Attachment: Bug 1180314 - Screenshot parameter as single string

Revised^2 patch attached (which I'll check after attaching).  Third time's the charm.

Comment 21

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 22

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

The tests look good, let's mark this for landing.

Thanks again for working on it!

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ea364b2e14d5

Comment 24

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Maybe it's worth uplifting this to DE as well?  Jeff, any thoughts?

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ea364b2e14d5

Comment 26

[Tim Nguyen :ntim]

(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 27

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 28

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4ffa40cf5c43