1444149 - add extension name to newTab override notification

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Markus Jaritz [:designakt] (UX)]

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

Comment 1

[Markus Jaritz [:designakt] (UX)]

Adding Meridel to ensure the copy is right.

Comment 2

[Mark Striemer [:mstriemer]]

Attachment: Bug 1444149 - Include addon name and icon in New Tab doorhanger

Review commit: https://reviewboard.mozilla.org/r/226646/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/226646/

Comment 3

[Mark Striemer [:mstriemer]]

Comment on attachment 8957712 [details]
Bug 1444149 - Include addon name and icon in New Tab doorhanger

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226646/diff/1-2/

Comment 4

[Mark Striemer [:mstriemer]]

Comment on attachment 8957712 [details]
Bug 1444149 - Include addon name and icon in New Tab doorhanger

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226646/diff/2-3/

Comment 5

[Mark Striemer [:mstriemer]]

Attachment: Screenshot 2018-03-09 16.30.52.png

Comment 6

[Andrew Swan [:aswan]]

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

Comment 7

[Mark Striemer [:mstriemer]]

Comment on attachment 8957712 [details]
Bug 1444149 - Include addon name and icon in New Tab doorhanger

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226646/diff/3-4/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2dd9a0fea03e
Include addon name and icon in New Tab doorhanger r=aswan

Comment 9

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/2dd9a0fea03e

Comment 10

[Francesco Lodolo [:flod]]

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?

Comment 11

[Francesco Lodolo [:flod]]

@Mark
Can you answer the comment above? ^^

Comment 12

[Mark Striemer [: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.

Comment 13

[Francesco Lodolo [:flod]]

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

Comment 14

[Mark Striemer [:mstriemer]]

Attachment: revert-name-change.patch

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.

Comment 15

[Francesco Lodolo [:flod]]

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

Comment 16

[Francesco Lodolo [:flod]]

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

Comment 17

[Mark Striemer [:mstriemer]]

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

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c32a186097a2
Revert string name change r=flod

Comment 19

[Stefan Hindli [:stefan_hindli]]

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

Comment 20

[Julien Cristau [:jcristau]]

Is this ripe for uplift to beta?

Comment 21

[Mark Striemer [:mstriemer]]

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

Comment 22

[Mark Striemer [:mstriemer]]

I have manually verified this on Nightly.

Comment 23

[Julien Cristau [:jcristau]]

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+

Comment 24

[Mark Striemer [:mstriemer]]

Comment on attachment 8958934 [details] [diff] [review]
revert-name-change.patch

Same as the other commit, but these both should land in 60.

Comment 25

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0c9152376d52 
https://hg.mozilla.org/releases/mozilla-beta/rev/903344884b55

Comment 26

[marius.santa]

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