1282462 - Add toggle for --fullpage screenshots to Options Panel

Status: RESOLVED INVALID

(DevTools :: Framework, enhancement, P3)

Description

[Helen V. Holmes (:helenvholmes) (:✨)]

We currently have two screenshotting modes: full page, and the current viewport. It would be nice to have the ability to toggle what kind of screenshotting you want from the options panel where our other screenshotting options currently are.

Comment 1

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1282462.patch

Comment 2

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

Comment on attachment 8767215 [details] [diff] [review]
1282462.patch

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

It seems reasonable other than the prefs, but I want to double check the next round.

::: devtools/client/framework/toolbox-options.xhtml
@@ +116,5 @@
>          </label>
> +        <label title="&options.screenshot.fullpage.tooltip;">
> +          <input type="checkbox"
> +                 id="devtools-screenshot-fullpage"
> +                 data-pref="devtools.screenshot-fullpage.enabled"/>

Can we fix these pref names to be:

devtools.screenshot.clipboard.enabled
devtools.screenshot.audio.enabled

I mentioned this in bug 1257913 comment 18 as well.  Since bug 1257913 is still open, I mentioned it again over there.

Comment 3

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1282462.patch [1.0]

Thanks for pointing out the pref naming thing. I missed it in my previous patch. I updated it in the other bug also.

Comment 4

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1282462.m4v

Comment 5

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

Comment on attachment 8767329 [details] [diff] [review]
1282462.patch [1.0]

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

Aside from the string ID it seems fine, but it would be nice to try locally...  but it seems to depend on an updated version of the patch for bug 1257913 which is not yet attached to that bug.

::: devtools/shared/locales/en-US/gclicommands.properties
@@ +142,5 @@
>  # LOCALIZATION NOTE (screenshotCopied) Text displayed to user when the
>  # screenshot is successfully copied to the clipboard.
>  screenshotCopied=Copied to clipboard.
>  
>  # LOCALIZATION NOTE (screenshotTooltipPage) Text displayed as tooltip for screenshot button in devtools ToolBox.

Nit: DevTools toolbox

@@ +143,5 @@
>  # screenshot is successfully copied to the clipboard.
>  screenshotCopied=Copied to clipboard.
>  
>  # LOCALIZATION NOTE (screenshotTooltipPage) Text displayed as tooltip for screenshot button in devtools ToolBox.
> +screenshotTooltipPage=Take a screenshot

When changing existing strings, you have use to a new string ID.[1]

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings

Comment 6

[Jaideep Bhoosreddy [:jbhoosreddy]]

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

Comment 7

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1282462.patch [2.0]

Hello Patrick, I was wondering if you could help me with this issue. Maybe a fresh look by someone help me figure out what the problem could be with this. The try push for this patch is failing similar to Bug 1257913. Maybe if I can figure this out, I can fix the screenshot clipboard issue too.

Comment 8

[Jaideep Bhoosreddy [:jbhoosreddy]]

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

Comment 9

[Patrick Brosset <:pbro>]

Comment on attachment 8794708 [details] [diff] [review]
1282462.patch [2.0]

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

Not sure if you're asking for a review or help figuring out the failures on try.

I've tried to run the failing test (browser_toolbox_options.js) and, indeed, it fails for me locally too. In fact, it times out.

This happens in testOptions. The test just loops through all checkboxes that have a data-pref attribute, and tries to click on them and detect the pref change.

The very first one it tries is devtools.webconsole.persistlog and for some reason, after having scrolled the checkbox into view, it can't click it.
EventUtils.synthesizeMouseAtCenter(node, {}, panelWin); seems to have no effect, so the test just hangs. If I click on the checkbox manually while the test waits, then it continues on and passes successfully. So something in your patch is breaking this for some reason.

If I try to set display:none; CSS on the fieldset you add, then the test continues further. So it seems that just adding the new checkboxes changed the layout in a way that EventUtils can't simulate the click on the first checkbox.
Looks like this is something wrong with the test itself, not with your change.

Comment 10

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1282462.patch [3.0]

Submitting an updated patch for this bug again, now that it is not blocked.

Comment 11

[Tim Nguyen :ntim]

Comment on attachment 8841137 [details] [diff] [review]
1282462.patch [3.0]

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

I would be really nice to have automated testing the various screenshot settings (just a test that would check if the right GCLI command is picked).

Also UX wise, I wonder if it would make more sense as a select box?

Capture: [ Full page      v]
         | Full page       |
         | Visible portion |

Looks good otherwise.

::: devtools/client/locales/en-US/toolbox.dtd
@@ +180,2 @@
>  <!ENTITY options.screenshot.clipboard.label      "Screenshot to clipboard">
>  <!ENTITY options.screenshot.clipboard.tooltip    "Saves to the screenshot directly to the clipboard">

Hmm, unrelated to this bug, but I noticed a typo here:

"Saves to the screenshot directly to the clipboard".

The first "to" shouldn't be there.

@@ +188,5 @@
> +
> +<!-- LOCALIZATION NOTE (options.screenshot.fullpage.label): This is the
> +   - label for the checkbox that toggles the fullpage mode in screenshot tool -->
> +<!ENTITY options.screenshot.fullpage.label      "Take screenshot of the entire page">
> +<!ENTITY options.screenshot.fullpage.tooltip    "Enables screenshots to capture the entire page instead of only the visible content">

I would prefer something like: 
"Captures the entire page instead of only the visible portion"

Comment 12

[Patrick Brosset <:pbro>]

This fell off my radar, but I'd really like to have this in sooner rather than later. Also, I think we should extend this functionality to also allow changing the dpi, just like the screenshot gcli command allows.
I think this should block removing gcli, which we are planning on doing in 61.

Comment 13

[Soledad Penades [:sole] [:spenades]]

I have filed two bugs to disambiguate the current screenshot behaviour after we've removed the GCLI UI.

* One makes the 'entire page' screenshot button actually take a full page screenshot: Bug 1463129.
* The other one adds a new button to take a partial screenshot (which is the current, wrong, and confusing behaviour): Bug 1463131

With these additions, this bug and its patch become slightly redundant. I'm sorry Jaideep, but I also thank you for your contributions!

Comment 14

[Soledad Penades [:sole] [:spenades]]

Furthermore, the option to choose between screenshot button behaviour has been removed, so this bug/patch are no longer valid. I'm going to close it as it's more confusing than helping us. Feel free to follow the progress in Bug 1463129 and Bug 1463131 :-)

Thank you everyone who contributed!