Closed Bug 1018721 Opened 5 years ago Closed 3 years ago

What is a "fullpage"?

Categories

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

defect
Not set

Tracking

(firefox49 verified)

VERIFIED FIXED
Firefox 49
Tracking Status
firefox49 --- verified

People

(Reporter: vporof, Assigned: bugzilla-mozilla, Mentored)

Details

(Keywords: dev-doc-complete, Whiteboard: [good first bug])

Attachments

(1 file, 4 obsolete files)

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"?
Hi I could not take screenshot by that button.why is it so?
Were do these screenshots go?

I click to camera but there is no indication of whether anything happened in response.
(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. :)
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
Component: Developer Tools → Developer Tools: Graphic Commandline and Toolbar
Whiteboard: [good first bug]
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
Flags: needinfo?(vporof)
Sounds good!
Flags: needinfo?(vporof)
Attached patch bug_1018721.patch (obsolete) — Splinter Review
Patch for the issue mentioned added
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
Attachment #8637742 - Flags: feedback+
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.
(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.
Flags: needinfo?(vporof)
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.
Flags: needinfo?(vporof)
Mentor: vporof
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.
Attachment #8692003 - Flags: review?(vporof)
Re-uploaded patch due to typo.

Sorry.
Attachment #8692003 - Attachment is obsolete: true
Attachment #8692003 - Flags: review?(vporof)
Attachment #8692055 - Flags: review?(vporof)
Attachment #8692055 - Flags: review?(vporof) → review+
I am new to open source community. I would like to take this as my first bug.
(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 :)
Assigning the bug to Raul Glogovetan, since he had a patch reviewed.
Assignee: nobody → bugzilla-mozilla
Attachment #8759018 - Attachment is obsolete: true
Attachment #8759018 - Attachment is patch: false
Corrected a mistake in previous patch.
Attachment #8759025 - Flags: review?(vporof)
Attachment #8759025 - Flags: review?(allamsetty.anup)
Attachment #8759025 - Attachment description: Patch for Bug 1018721 → Patch for Bug 1018721 Please review it and let me know if anything needs to be added
Assignee: bugzilla-mozilla → prakharguptaiitd
Mentor: jryans
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.
Assignee: prakharguptaiitd → bugzilla-mozilla
Attachment #8759025 - Attachment is obsolete: true
Attachment #8759025 - Flags: review?(vporof)
Attachment #8759025 - Flags: review?(allamsetty.anup)
Attachment #8637742 - Attachment is obsolete: true
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/60b8c047de4c
Changed wording for screenshot button tooltip. r=vporof
https://hg.mozilla.org/mozilla-central/rev/60b8c047de4c
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
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
QA Whiteboard: [bugday-20160622]
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.
As this bug's fix is also verified on Windows 10, 32 bit (comment 24), I am marking this as verified!
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.