1147805 - Add Learn More link to the add-on install confirmation doorhanger

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Dão Gottwald [:dao]]

As soon as we have the URL, this should be as simple as setting learnMoreURL on the options object we pass to PopupNotifications.show.

Comment 1

[Markus Jaritz [:designakt] (UX)]

Dao, please link to this URL: https://support.mozilla.org/kb/find-and-install-add-ons-add-features-to-firefox

Comment 2

[Dão Gottwald [:dao]]

Attachment: patch

Comment 3

[Dave Townsend [:mossop]]

Comment on attachment 8590669 [details] [diff] [review]
patch

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

Sounds like putting this in a pref is the normal way to go so lets move it there

Comment 4

[Dão Gottwald [:dao]]

(In reply to Dave Townsend [:mossop] from comment #3)
> Comment on attachment 8590669 [details] [diff] [review]
> patch
> 
> Review of attachment 8590669 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sounds like putting this in a pref is the normal way to go so lets move it
> there

It's not; the normal way is to use Services.urlFormatter.formatURLPref("app.support.baseURL") + id, but that needs server-side magic to recognize that id.

Comment 5

[Dave Townsend [:mossop]]

From IRC:

<Mossop> When we embed links in the UI do we generally do so by putting the url directly in the code or is it better to use a pref or a locale or something?
<margaret> Mossop: i think we usually put them in a pref
<gavin> yeah pref is better
<gavin> if only to consolidate external depenencies in firefox.js rather than across the rest of the code base
<gavin> makes it easier to audit what we depend on

Gavin's reasoning seems sound here and just because it is in a pref doesn't mean it has to be formatted or anything beyond a simple string of the url. Feel free to see if gavin wants to review this instead though if you think I'm missing something.

Comment 6

[Dão Gottwald [:dao]]

(In reply to Dave Townsend [:mossop] from comment #5)
> From IRC:
> 
> <Mossop> When we embed links in the UI do we generally do so by putting the
> url directly in the code or is it better to use a pref or a locale or
> something?
> <margaret> Mossop: i think we usually put them in a pref
> <gavin> yeah pref is better
> <gavin> if only to consolidate external depenencies in firefox.js rather
> than across the rest of the code base
> <gavin> makes it easier to audit what we depend on
> 
> Gavin's reasoning seems sound here and just because it is in a pref doesn't
> mean it has to be formatted or anything beyond a simple string of the url.

You seem to be missing my point... the normal way is to /not/ add a pref and /instead/ use app.support.baseURL. But this has a server-side dependency which I guess we'll need to file a new bug for somewhere.

Comment 7

[Dave Townsend [:mossop]]

(In reply to Dão Gottwald [:dao] from comment #6)
> (In reply to Dave Townsend [:mossop] from comment #5)
> You seem to be missing my point... the normal way is to /not/ add a pref and
> /instead/ use app.support.baseURL. But this has a server-side dependency
> which I guess we'll need to file a new bug for somewhere.

We have other code in tree that hardcodes a URL from a pref instead of using app.support.baseURL so I'm not sure why this is a requirement.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

We should use app.support.baseURL for all SUMO links.

We can get the SUMO-side stuff added pretty easily by reaching out to someone who has those rights. Joni, can you get us an in-product link for https://support.mozilla.org/kb/find-and-install-add-ons-add-features-to-firefox?

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Oh, I missed that Dao filed bug 1154666 for that.

Comment 10

[Dão Gottwald [:dao]]

(In reply to Dave Townsend [:mossop] from comment #7)
> We have other code in tree that hardcodes a URL from a pref instead of using
> app.support.baseURL so I'm not sure why this is a requirement.

We also have various other code in the tree hardcoding support.mozilla.org URLs directly without prefs.

If we want to do it the right way, using app.support.baseURL is the way to go. If we want to take a shortcut and land something before we can do it the right way (bug 1154666), I'd like to keep it simple and not add a pref.

Comment 11

[Dave Townsend [:mossop]]

Ok thanks for clarification on that. Let's do this the right way.

Comment 12

[Joni Chan]

I see that mythmon already took care of this so I'm clearing my N.I. 

Gavin is right - just contact (or N.I.) me if you need more in-product links.

Comment 13

[Dão Gottwald [:dao]]

Attachment: patch v2

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/32ec4b157844

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/32ec4b157844

Comment 16

[Vasilica Tamas, QA [:vtamas]]

Verified fixed on Firefox 40.0a1 (2015-04-20) using Windows 7 64-bit and Ubuntu 14.04 32-bit.

During my testing I have noticed 2 issues: 
- "Learn More" link should be positioned on the bottom of the doorhanger or should be accompanied by an explanatory text because in the current situation it may be confusing, the user could think that is a page that offers more information about the selected add-on (e.g. http://i.imgur.com/Ucuu7CP.jpg )

- Left click and scroll click on link do not open the page in a new tab. The "Learn more" Sync link from the confirmation doorhanger does not have the same behavior, it opens in a new tab on left click and scroll click commands.

Should I file new bugs for these issues?

Comment 17

[Dave Townsend [:mossop]]

Yes please, mark them blocking this one

Comment 18

[Vasilica Tamas, QA [:vtamas]]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #16)

> - Left click and scroll click on link do not open the page in a new tab. The
> "Learn more" Sync link from the confirmation doorhanger does not have the
> same behavior, it opens in a new tab on left click and scroll click commands.

Sorry I made a mistake. I meant to say *Right* click instead of *Left* click.