Closed
Bug 1235902
Opened 7 years ago
Closed 7 years ago
Support actions from links in hosted feedback page
Categories
(Firefox for Android Graveyard :: General, defect)
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”
Assignee | ||
Comment 1•7 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/29243/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/29243/
Attachment #8703002 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 2•7 years ago
|
||
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.
Updated•7 years ago
|
Attachment #8703002 -
Flags: review?(mark.finkle)
Comment 3•7 years ago
|
||
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
Assignee | ||
Comment 4•7 years ago
|
||
(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.
Comment 5•7 years ago
|
||
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".
Assignee | ||
Comment 6•7 years ago
|
||
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)
Assignee | ||
Comment 7•7 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/29317/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/29317/
Attachment #8703202 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 8•7 years ago
|
||
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 9•7 years ago
|
||
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 10•7 years ago
|
||
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+
Assignee | ||
Comment 11•7 years ago
|
||
(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.
Assignee | ||
Comment 12•7 years ago
|
||
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
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f2c54cd55ed5 https://hg.mozilla.org/mozilla-central/rev/deaf711a7bc8
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Updated•2 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•