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

VERIFIED FIXED in Firefox 40

Status

()

Firefox
General
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

Trunk
Firefox 40
Points:
1
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox40 verified)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
As soon as we have the URL, this should be as simple as setting learnMoreURL on the options object we pass to PopupNotifications.show.
Points: --- → 1
Flags: qe-verify+
Flags: firefox-backlog+
(Assignee)

Comment 2

3 years ago
Created attachment 8590669 [details] [diff] [review]
patch
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #8590669 - Flags: review?(dtownsend)
(Assignee)

Updated

3 years ago
Iteration: --- → 40.1 - 13 Apr
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
Attachment #8590669 - Flags: review?(dtownsend) → review-
(Assignee)

Comment 4

3 years ago
(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.
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.
(Assignee)

Comment 6

3 years ago
(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.

Updated

3 years ago
Iteration: 40.1 - 13 Apr → 40.2 - 27 Apr
(Assignee)

Updated

3 years ago
Depends on: 1154666
(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.
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?
Flags: needinfo?(jsavage)
Oh, I missed that Dao filed bug 1154666 for that.
(Assignee)

Comment 10

3 years ago
(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.
Ok thanks for clarification on that. Let's do this the right way.
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.
Flags: needinfo?(jsavage)
(Assignee)

Comment 13

3 years ago
Created attachment 8593208 [details] [diff] [review]
patch v2
Attachment #8590669 - Attachment is obsolete: true
Attachment #8593208 - Flags: review?(dtownsend)
Attachment #8593208 - Flags: review?(dtownsend) → review+
https://hg.mozilla.org/mozilla-central/rev/32ec4b157844
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
QA Contact: vasilica.mihasca
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?
Status: RESOLVED → VERIFIED
status-firefox40: fixed → verified
Flags: needinfo?(dtownsend)
Yes please, mark them blocking this one
Flags: needinfo?(dtownsend)
(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.
(Assignee)

Updated

3 years ago
No longer depends on: 1157144
You need to log in before you can comment on or make changes to this bug.