1018721 - What is a "fullpage"?

Status: VERIFIED FIXED

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)

Description

[Victor Porof [:vporof][:vp]]

The new screenshot button has an entry in the options panel that reads "Take a fullpage screenshot". A "fullpage" isn't a word.

How about "Take a screenshot of the entire page"?

Comment 1

[Athira S]

Hi I could not take screenshot by that button.why is it so?

Comment 2

[Adrian Aichner [:anaran]]

Were do these screenshots go?

I click to camera but there is no indication of whether anything happened in response.

Comment 3

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

(In reply to adrian.aichner from comment #2)
> Were do these screenshots go?

They go to your Downloads folder.

> I click to camera but there is no indication of whether anything happened in
> response.

That's true, see bug 1018619.  It does work though. :)

Comment 4

[Patrick Brosset <:pbro>]

Moving this to the GCLI component as this is actually an issue with the localization file of the screenshot gcli command: toolkit/locales/en-US/chrome/global/devtools/gclicommands.properties
Marking as good first bug too since this seems simple enough.
As per [1] I don't think a new string ID is required here, we can just change the English translation because this is a minor change in the phrasing, the meaning doesn't change.

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

Comment 5

[malu.t90]

Hi,

I went through the above comments and did the following to understand the issue. 

1) In browser, right click and select "Inspect Element". This will open the developer tools page at the bottom.
2) Then select "Toolbox Options" on the top right of developer tools
3) Under "Available toolbox options" then check the "Take a full page screenshot" check box. The camera icon that appears on the developer tools. The tooltip will show the same text.

And the patch is supposed to change the tooltip and the text under "Toolbox options". Is that right?

I made the change in the following file, 
./browser/locales/en-US/chrome/browser/devtools/gclicommands.properties

The change to the string solves the problem I mentioned above. Can somebody guide me if there is anything else needs to be done?

Thanks,
Malathi

Comment 6

[Victor Porof [:vporof][:vp]]

Sounds good!

Comment 7

[malu.t90]

Attachment: bug_1018721.patch

Patch for the issue mentioned added

Comment 8

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

Comment on attachment 8637742 [details] [diff] [review]
bug_1018721.patch

Thank you for the patch.  This is almost enough!

However, due to the way our localization works, when we change the string text, we also need to change the string ID so that localizers notice it has changed[1].

So, for this case, please change "screenshotTooltip" to "screenshotTooltip2", or change the naming slightly so the ID is different.  You'll also need to update code that referred to "screenshotTooltip" to use the new ID.

When you submit your updated patch, it's best to flag someone for review / feedback or else we may miss your patch.

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

Comment 9

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

Oh, hmm...  I see Patrick already mentioned the localization rules in comment 4.

I think it would best to change the ID in this case.  We have changed the meaning slightly, so we should give other languages a chance to update too.

The wiki page says "If your changes are relevant only for English — for example, to correct a typographical error or to make letter case consistent — then there is generally no need to update the entity name."  In this case, it seems like we've gone further than that, so we *would* need to change the string ID here.

Comment 10

[malu.t90]

(In reply to malu.t90 from comment #7)
> Created attachment 8637742 [details] [diff] [review]
> bug_1018721.patch
> 
> Patch for the issue mentioned added

Can someone please help review the changes? Also please advise if a new string needs to be created.

Comment 11

[Victor Porof [:vporof][:vp]]

Comment on attachment 8637742 [details] [diff] [review]
bug_1018721.patch

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

::: ./browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
@@ +130,4 @@
>  screenshotCopied=Copied to clipboard.
>  
>  # LOCALIZATION NOTE (screenshotTooltip) Text displayed as tooltip for screenshot button in devtools ToolBox.
> +screenshotTooltip=Take a screenshot of the entire page 

There's trailing whitespace here. You will also need to update the localization id string, to something like screenshotTooltip2 since this is probably a significant enough change that we want to alert the localizers.

Comment 12

[u556493]

Attachment: Changed wording for screenshot button tooltip

Please let me know if the patch format is OK, I'm not that familiar with Mercurial.

Is there a guide on how I could submit directly to MozReview ?

If there's anything else let me know.

Thanks.

Comment 13

[u556493]

Attachment: Changed wording for screenshot button tooltip

Re-uploaded patch due to typo.

Sorry.

Comment 14

[Kartikey Agrawal[:kartikey0303]]

I am new to open source community. I would like to take this as my first bug.

Comment 15

[Anup Kumar [:reznord]]

(In reply to Kartikey Agrawal from comment #14)
> I am new to open source community. I would like to take this as my first bug.

Hey Kartikey, 

Go ahead and work on this bug, it looks like it is untouched for a while. Once after you submit a patch to this bug, I can assign the bug to you :)

Comment 16

[Anup Kumar [:reznord]]

Assigning the bug to Raul Glogovetan, since he had a patch reviewed.

Comment 17

[Prakhar Gupta]

Attachment: Preliminary patch for Bug 1018721 to refer for review.

Comment 18

[Prakhar Gupta]

Attachment: Patch for Bug 1018721 Please review it and let me know if anything needs to be added

Corrected a mistake in previous patch.

Comment 19

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

Hmm, actually it looks like Raul had an already reviewed patch that is ready to land.  Let's try to move forward with it.  prakharguptaiitd, please check out other bugs available at http://firefox-dev.tools.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/60b8c047de4c
Changed wording for screenshot button tooltip. r=vporof

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/60b8c047de4c

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/60b8c047de4c

Comment 23

[Khalid Syfullah Zaman [:khalid32]]

I have reproduced this bug on Nightly 32.0a1 (2014-06-01) on ubuntu 16.04 LTS, 64 bit!

The bug's fix is now verified on Latest Aurora 49.0a2!

Build ID: 20160622004014
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0

Comment 24

[Nawaf Chowdhury Akash]

I have reproduced this bug with nightly 32.0a1 (2014-06-01) on windows 10, 32 bit.

The bug’s fix is now verified on Latest Nightly 50.0a1 and Latest Aurora 49.0a2.

Build ID:20160709030233.
User Agent:Mozilla/5.0 (Windows NT 10.0; rv:50.0) Gecko/20100101 Firefox/50.0.

Build ID:20160710004020.
User Agent:Mozilla/5.0 (Windows NT 10.0; rv:49.0) Gecko/20100101 Firefox/49.0.

Comment 25

[Khalid Syfullah Zaman [:khalid32]]

As this bug's fix is also verified on Windows 10, 32 bit (comment 24), I am marking this as verified!

Comment 26

[Sebastian Zartner [:sebo]]

Documented the change at https://developer.mozilla.org/en-US/docs/Tools/Tools_Toolbox#Extra_tools.

Sebastian