Closed Bug 1444149 Opened 2 years ago Closed 2 years ago

add extension name to newTab override notification

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla61
Tracking Status
firefox60 + verified
firefox61 --- verified

People

(Reporter: designakt, Assigned: mstriemer)

References

Details

Attachments

(4 files)

Reviewing this message with Kev and with Meridel, and also using it in other contexts (Tab Hiding) made it clear that having the extension name in the message will improve clarity of the message.

Please add the extensions name to the dialog as Mark had already proposed in https://bugzilla.mozilla.org/show_bug.cgi?id=1390158#c28
https://bugzilla.mozilla.org/attachment.cgi?id=8927070
Adding Meridel to ensure the copy is right.
Flags: needinfo?(mwalkington)
Comment on attachment 8957712 [details]
Bug 1444149 - Include addon name and icon in New Tab doorhanger

https://reviewboard.mozilla.org/r/226646/#review232590

just optional nits

::: browser/locales/en-US/chrome/browser/browser.dtd:976
(Diff revision 3)
>  <!ENTITY updateRestart.acceptButton.accesskey "R">
>  <!ENTITY updateRestart.cancelButton.label "Not Now">
>  <!ENTITY updateRestart.cancelButton.accesskey "N">
>  <!ENTITY updateRestart.panelUI.label2 "Restart to update &brandShorterName;">
>  
> -<!ENTITY newTabControlled.message "An extension has changed the page you see when you open a New Tab. You can restore your settings if you do not want this change.">
> +<!-- newTabControlled.message is in toolkit/locales/en-US/chrome/global/extensions.properties -->

I don't think this is necessary

::: browser/locales/en-US/chrome/browser/browser.dtd:980
(Diff revision 3)
>  
> -<!ENTITY newTabControlled.message "An extension has changed the page you see when you open a New Tab. You can restore your settings if you do not want this change.">
> -<!ENTITY newTabControlled.header.message "Your New Tab has changed.">
> +<!-- newTabControlled.message is in toolkit/locales/en-US/chrome/global/extensions.properties -->
> +<!ENTITY newTabControlled.header.message2 "Your New Tab changed.">
>  <!ENTITY newTabControlled.keepButton.label "Keep Changes">
>  <!ENTITY newTabControlled.keepButton.accesskey "K">
> -<!ENTITY newTabControlled.restoreButton.label "Restore Settings">
> +<!ENTITY newTabControlled.restoreButton.label2 "Disable Extension">

since we're changing the name here, we could just do restore -> disable and drop the 2
Attachment #8957712 - Flags: review?(aswan) → review+
Keywords: checkin-needed
Blocks: 1414029
Pushed by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2dd9a0fea03e
Include addon name and icon in New Tab doorhanger r=aswan
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2dd9a0fea03e
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment on attachment 8957712 [details]
Bug 1444149 - Include addon name and icon in New Tab doorhanger

https://reviewboard.mozilla.org/r/226646/#review233462

::: browser/locales/en-US/chrome/browser/browser.dtd:976
(Diff revision 4)
>  <!ENTITY updateRestart.acceptButton.accesskey "R">
>  <!ENTITY updateRestart.cancelButton.label "Not Now">
>  <!ENTITY updateRestart.cancelButton.accesskey "N">
>  <!ENTITY updateRestart.panelUI.label2 "Restart to update &brandShorterName;">
>  
> -<!ENTITY newTabControlled.message "An extension has changed the page you see when you open a New Tab. You can restore your settings if you do not want this change.">
> +<!ENTITY newTabControlled.header.message2 "Your New Tab has changed.">

This change was unnecessary, the string is the same.

But… https://bug1444149.bmoattachments.org/attachment.cgi?id=8957725 has "changed" instead of "has changed".

Is the string in the patch correct?
@Mark
Can you answer the comment above? ^^
Flags: needinfo?(mstriemer)
Sorry, we removed the "has" and then it got added back in. The name change is not needed. Should I create a patch to change it back?

Copy was confirmed on slack, so clearing Meridel's NI.
Flags: needinfo?(mwalkington)
Flags: needinfo?(mstriemer)
(In reply to Mark Striemer [:mstriemer] from comment #12)
> Sorry, we removed the "has" and then it got added back in. The name change
> is not needed. Should I create a patch to change it back?
> 
> Copy was confirmed on slack, so clearing Meridel's NI.

If you can land a patch quickly, that would work. 

I can wait to expose new strings to localization tools until then.
The name of this string changed but it didn't end up needing to change. Reverting that so we don't need to translate it again.
Attachment #8958934 - Flags: review?(aswan)
Comment on attachment 8958934 [details] [diff] [review]
revert-name-change.patch

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

I think I can still r? for this
Attachment #8958934 - Flags: review?(aswan) → review+
(In reply to Francesco Lodolo [:flod] from comment #15)
> Comment on attachment 8958934 [details] [diff] [review]
> revert-name-change.patch
> 
> Review of attachment 8958934 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think I can still r? for this

steal, even.
Adding checkin-needed for the follow-up patch. If you can land it flod that might be easier (I do not currently have commit access).
Keywords: checkin-needed
Is this ripe for uplift to beta?
Comment on attachment 8957712 [details]
Bug 1444149 - Include addon name and icon in New Tab doorhanger

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: It might not be clear what extension is causing this doorhanger to appear, and the results of clicking "Restore Settings" are ambiguous.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]:
  1. Install an extension that controls the New Tab, ex: Tabby Cat or Tabliss
  2. Open a new tab, verify the extension name and icon are shown and the secondary action is "Disable Extension"
[List of other uplifts needed for the feature/fix]: Both patches in this bug
[Is the change risky?]: No
[Why is the change risky/not risky?]: String changes with simple substitution
[String changes made/needed]: Yes
Attachment #8957712 - Flags: approval-mozilla-beta?
I have manually verified this on Nightly.
Comment on attachment 8957712 [details]
Bug 1444149 - Include addon name and icon in New Tab doorhanger

more information in new tab doorhanger, l10n changes acked by flod, beta60+
Attachment #8957712 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8958934 [details] [diff] [review]
revert-name-change.patch

Same as the other commit, but these both should land in 60.
Attachment #8958934 - Flags: approval-mozilla-beta?
Attachment #8958934 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attached image my pic.jpg
Tested and reproduced in Firefox 60.0a1 (20180308100121) on Windows 10 64Bit.
Retested and verified in Firefox 61.0a1 (20180403100105) and Firefox 60.0b9 (20180402175344) on Windows 10 64Bit and MacOS 10.13.3.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.