Closed Bug 1235902 Opened 5 years ago Closed 5 years ago

Support actions from links in hosted feedback page

Categories

(Firefox for Android Graveyard :: General, defect)

35 Branch
defect
Not set
normal

Tracking

(firefox46 fixed)

RESOLVED FIXED
Firefox 46
Tracking Status
firefox46 --- fixed

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

Attachments

(2 files)

We specifically need something to happen when…

a) Someone clicks on “Maybe later”
b) Someone clicks on “No thanks”
c) Someone clicks on “Go to Play Store”
This works with my test page here:
http://people.mozilla.org/~mleibovic/feedback.html

This patch isn't great because it adds these event listeners to the browser deck no matter what... maybe I should try to be lazier about it.

I tried adding the listeners in the old "Feedback:Show" observer, but the problem with that is that in that case the links would only work when we prompt the user for feedback, vs. them going to the feedback page directly.

I'll also want to write a follow-up patch to read from this URL pref instead of loading about:feedback directly from the "Feedback" item in settings.
Attachment #8703002 - Flags: review?(mark.finkle)
Comment on attachment 8703002 [details]
MozReview Request: Bug 1235902 - Support actions from links in hosted feedback page. r=mfinkle

https://reviewboard.mozilla.org/r/29243/#review26043

::: mobile/android/chrome/content/browser.js:3272
(Diff revision 1)
> +var Feedback = {

Why not continue to use Feedback.js and lazy load it? We want less in browser.js, right?

::: mobile/android/chrome/content/browser.js:3282
(Diff revision 1)
> +    BrowserApp.deck.addEventListener("FeedbackClose", this, false, true);

You could add these directly to the tab.browser when adding it, and remove them when closing it.

::: mobile/android/chrome/content/browser.js:3288
(Diff revision 1)
> +    if (aTopic !== "Feedback:Show")

\{ \}

::: mobile/android/chrome/content/browser.js:3298
(Diff revision 1)
> +    BrowserApp.addTab(url, { selected: true, parentId: BrowserApp.selectedTab.id }).browser;

selectOrAddTab might be better, just in case

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#6003
(In reply to Mark Finkle (:mfinkle) from comment #3)
> Comment on attachment 8703002 [details]
> MozReview Request: Bug 1235902 - Support actions from links in hosted
> feedback page. r=mfinkle
> 
> https://reviewboard.mozilla.org/r/29243/#review26043
> 
> ::: mobile/android/chrome/content/browser.js:3272
> (Diff revision 1)
> > +var Feedback = {
> 
> Why not continue to use Feedback.js and lazy load it? We want less in
> browser.js, right?

There's not much point to lazy-loading it if we need to add listeners on startup.

> ::: mobile/android/chrome/content/browser.js:3282
> (Diff revision 1)
> > +    BrowserApp.deck.addEventListener("FeedbackClose", this, false, true);
> 
> You could add these directly to the tab.browser when adding it, and remove
> them when closing it.

So this is what I mentioned in my last comment - if we do this, these links will only work when we prompt them directly here. It mainly depends on how we expect people to be getting at this feedback page. If this is the case, we should probably use a URL parameter to only show these links when the user is prompted.

Alternately, we could try adding this logic in content.js, where we already listen for the "AboutReaderContentLoaded" event, and create a similar "FeedbackContentLoaded" event that adds the additional listeners. But then we're trading off one listener on the deck vs. a different listener on every content window.

I'll keep thinking about this.
So, the current plan is that the form that Justin is building on Input will become *the* Firefox for Android feedback form. 

People come from various places to leave feedback currently, like the release notes page, peoples' blog posts, etc. If a user was coming from the releasenotes page, it seems weird to be greeted with a form that has "Maybe later" and "No thanks".

I think a querystring param is probably the easiest way to control whether those links show where "easiest" is defined as "easy to implement and easy to test the various possibilities".
Comment on attachment 8703002 [details]
MozReview Request: Bug 1235902 - Support actions from links in hosted feedback page. r=mfinkle

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29243/diff/1-2/
Attachment #8703002 - Flags: review?(mark.finkle)
I updated the patch to only add the listeners in the case where we open the tab directly ourselves.

In order to test this with a development page, you'll need to use the remote console to trigger the browser to open the page directly. To do this, in the main process, run the following code:

Services.obs.notifyObservers(null, "Feedback:Show", null);
Comment on attachment 8703002 [details]
MozReview Request: Bug 1235902 - Support actions from links in hosted feedback page. r=mfinkle

https://reviewboard.mozilla.org/r/29243/#review26637
Attachment #8703002 - Flags: review?(mark.finkle) → review+
Comment on attachment 8703202 [details]
MozReview Request: Bug 1235902 - (Part 2) Update feedback settings item to read URL from gecko prefs. r=mfinkle

https://reviewboard.mozilla.org/r/29317/#review26639

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java
(Diff revision 1)
> -    private static final String PREFS_ADVANCED = NON_PREF_PREFIX + "advanced.enabled";

Did you want to remove these?

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java
(Diff revision 1)
> -    private static final String PREFS_CATEGORY_HOMEPAGE = NON_PREF_PREFIX + "category_homepage";

And this one
Attachment #8703202 - Flags: review?(mark.finkle) → review+
(In reply to Mark Finkle (:mfinkle) from comment #10)
> Comment on attachment 8703202 [details]
> MozReview Request: Bug 1235902 - (Part 2) Update feedback settings item to
> read URL from gecko prefs. r=mfinkle
> 
> https://reviewboard.mozilla.org/r/29317/#review26639
> 
> :::
> mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java
> (Diff revision 1)
> > -    private static final String PREFS_ADVANCED = NON_PREF_PREFIX + "advanced.enabled";
> 
> Did you want to remove these?
> 
> :::
> mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java
> (Diff revision 1)
> > -    private static final String PREFS_CATEGORY_HOMEPAGE = NON_PREF_PREFIX + "category_homepage";
> 
> And this one

They're unused, so I was just doing some clean-up while I was in here.
https://hg.mozilla.org/integration/fx-team/rev/f2c54cd55ed5ee6baecfe6b195292ad682167f5f
Bug 1235902 - Support actions from links in hosted feedback page. r=mfinkle

https://hg.mozilla.org/integration/fx-team/rev/deaf711a7bc8a8cb604666b0e4e7a91edc90018e
Bug 1235902 - (Part 2) Update feedback settings item to read URL from gecko prefs. r=mfinkle
https://hg.mozilla.org/mozilla-central/rev/f2c54cd55ed5
https://hg.mozilla.org/mozilla-central/rev/deaf711a7bc8
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.