Closed
Bug 665196
Opened 13 years ago
Closed 2 years ago
Change in-content plugin crash UI to submit a crash report and reload the page in one step
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
INVALID
Firefox 10
People
(Reporter: verdi, Unassigned)
References
(Depends on 1 open bug)
Details
Attachments
(1 file, 15 obsolete files)
9.94 KB,
patch
|
Dolske
:
review+
|
Details | Diff | Splinter Review |
Currently when, for example, a flash video crashes the in-content plugin crash UI has a link that says, "Submit crash report." It's not until after you submit a crash report that you get a second message and a link to "reload the page to try again." We should change the original message to something like, "Submit crash report and reload the page." If the user is going to use the in-content UI for resolving the issue we might as well do this in one step instead of two. Also it solves the immediate problem of reloading the missing content in the first step instead of the second step.
Comment 1•13 years ago
|
||
We could try to make the UI more like the existing crash reporter client, where "submit" is a checkbox, and "restart" is a button, so we could have "[x] Submit a crash report" and "[ Reload this page]", and clicking reload would submit if the box is checked, as well as reload.
Reporter | ||
Comment 2•13 years ago
|
||
Friendly ping. I would love to get this into Firefox 8.
OS: Mac OS X → All
Reporter | ||
Comment 3•13 years ago
|
||
Flash crashed this morning <http://people.mozilla.org/~mverdi/screenshots/broswing_-_blip.tv_%28since_2005%29-20110830-085354.jpg> and I was reminded that this bug is still open. If we get this in Firefox 9 we'll only be showing this less than optimal message to people for four more months.
Comment 4•13 years ago
|
||
I spoke to Dolske about this and it kind of got lost in the black hole. We have a new intern starting next week and this would be a great project for him. I'll put it on my list to followup in a couple of weeks.
Comment 5•13 years ago
|
||
Ahhh, NUTS. I keep dropping this on the floor. :( So, to recap prior email on this... 0) SUMO is getting ~240K monthly hits on the support page we link to from the Firefox UI, and this is abnormally high compared to other SUMO pages (even those linked to from within Firefox). Suspicion is that this may be due to confused users wondering what to do after a plugin crashes. 1) It would be useful to add telemetry probes to help understand how many plugin crashes happen vs how many are reported (or fail to generate a report, ala bug 554451). Also add a modifier to the help links (in-content UI and notification bar) to help understand which is generating more clicks. 2) Verdi suggests that the in-content UI is confusing because it doesn't offer any immediate solution to the problem -- just links to submit a crash report and a "?" icon... We could improve the steps of "Submit crash report" --> *click* --> "Submitting..." --> "Crash report sent. Reload the page to try again" to something that immediately suggests a helpful action to perform. For example, "Reload page and submit crash report" --> *click* --> reload page + background submission. 3) The suggestion was made in the thread to modify the notification bar, so make better explain that the _page_ was using a plugin, even it it wasn't visible, and to change the 2 buttons to a checkbox + 1 button. old: The Flash plugin crashed. _Learn More…_ (Reload) (Submit crash report) new: (abbreviated here for space): Page was using Flash plugin, it crashed. _Learn More…_ [x] Submit report (Reload) 4) Possibly do a survey to better understand why people click the help link, which might lead to other article/UI changes. (Out of scope for this bug, though)
Comment 6•13 years ago
|
||
Bouncing bug over to Margaret as a placeholder; let's talk about making this an early project for Felix? I meant to note above: all these changes look reasonable to me. I'd like to review the original bug to see if there were any specific reasons we avoided any of this originally, but I think we should proceed. #3 might need a privacy review. The link tweaks in #2 would be nice to make in Aurora (Beta?) so we can make some before/after comparisons.
Assignee: dolske → margaret.leibovic
Comment 7•13 years ago
|
||
Sort of repeating myself from comment 1, but one of the driving design forces behind the crash reporter client UI was "make it so users can get back to what they were doing as quickly and easily as possible". Getting crash data was secondary, but we tried to make it so that it went hand in hand. That's why we made the check-box to submit crash data opt-out by default, so all users have to do is click "Restart Firefox" and they're back up and running (and we get a crash report by default).
Comment 8•13 years ago
|
||
(In reply to Justin Dolske [:Dolske] from comment #6) > I meant to note above: all these changes look reasonable to me. I'd like to > review the original bug to see if there were any specific reasons we avoided > any of this originally, but I think we should proceed. Could you link to that bug and/or set a dependency?
Comment 9•13 years ago
|
||
Summary of Changes: - Made aggregate function submitReportAndReload, does as it says on the tin. - Updated some string - Just re-arranged some display logic so the original reload message doesn't show alongside the new message. Screenshots attached below.
Attachment #559091 -
Flags: review?(margaret.leibovic)
Attachment #559091 -
Flags: review?(dolske)
Comment 10•13 years ago
|
||
Comment 11•13 years ago
|
||
Comment 12•13 years ago
|
||
Comment 13•13 years ago
|
||
Just some style changes
Attachment #559091 -
Attachment is obsolete: true
Attachment #559091 -
Flags: review?(margaret.leibovic)
Attachment #559091 -
Flags: review?(dolske)
Attachment #559095 -
Flags: review?(margaret.leibovic)
Attachment #559095 -
Flags: review?(dolske)
Comment 14•13 years ago
|
||
(In reply to Ted Mielczarek [:ted, :luser] from comment #7) > Sort of repeating myself from comment 1, but one of the driving design > forces behind the crash reporter client UI was "make it so users can get > back to what they were doing as quickly and easily as possible". Getting > crash data was secondary, but we tried to make it so that it went hand in > hand. That's why we made the check-box to submit crash data opt-out by > default, so all users have to do is click "Restart Firefox" and they're back > up and running (and we get a crash report by default). +1 Submitting a crash report should remain opt in. While the user can reload the page without the link, "Reload page and send crash report" makes it look like there's no choice.
Updated•13 years ago
|
Assignee: margaret.leibovic → ffung
Reporter | ||
Comment 15•13 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #14) > > Submitting a crash report should remain opt in. While the user can reload > the page without the link, "Reload page and send crash report" makes it look > like there's no choice. This doesn't change the current behavior, it just makes it one step instead of two. With the current set up you have no choice but to "submit crash report" or reload the page manually.
Comment 16•13 years ago
|
||
(In reply to Verdi from comment #15) > (In reply to Dão Gottwald [:dao] from comment #14) > > Submitting a crash report should remain opt in. While the user can reload > > the page without the link, "Reload page and send crash report" makes it look > > like there's no choice. > > This doesn't change the current behavior, it just makes it one step instead > of two. With the current set up you have no choice but to "submit crash > report" or reload the page manually. The current UI doesn't make much sense, frankly. On top of that, the proposed change doesn't merely maintain the current behavior. The user currently doesn't feel obliged to submit data in order to reload, as we don't tell him beforehand that the option to reload will appear as a second step.
Reporter | ||
Comment 17•13 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #16) > The user > currently doesn't feel obliged to submit data in order to reload, as we > don't tell him beforehand that the option to reload will appear as a second > step. I'm not sure they don't feel obliged to submit data. The only in-content options are to submit a crash report or click the question mark. What's also troubling to me about those two options is that about 60,000 people click the question mark each week (4M total), taking them to a SUMO article which tells them to reload the page https://support.mozilla.com/en-US/kb/Plugin%20crash%20reports That's > 2X the next most clicked in-product sumo article. I think this is because the UI doesn't give people the solution to their problem which is to reload the page.
Comment 18•13 years ago
|
||
(In reply to Verdi from comment #17) > the next most clicked in-product sumo article. I think this is because the > UI doesn't give people the solution to their problem which is to reload the > page. Sure, I agree that the current UI is broken. There's no reason why the user should have to submit data in order to get the plugin working again.
Comment 19•13 years ago
|
||
What about a checkbox alternative?
Comment 20•13 years ago
|
||
Patch reflecting screenshot with checkbox
Attachment #559614 -
Flags: review?(margaret.leibovic)
Comment 21•13 years ago
|
||
Comment on attachment 559587 [details]
Alternative Submit Crash Report Screen
This looks good. We should be populating the checkbox using the default pref, I know we have code for that. (Why is the checkbox on the wrong side of the text in this screenshot?)
Comment 22•13 years ago
|
||
Move checkbox to the correct side of label
Attachment #559614 -
Attachment is obsolete: true
Attachment #559614 -
Flags: review?(margaret.leibovic)
Attachment #559837 -
Flags: review?(margaret.leibovic)
Attachment #559837 -
Flags: review?(dolske)
Comment 23•13 years ago
|
||
First, a bit of history about this UI FTR... The main implementation(s) occurred in bug 538910, then bug 550293, and finally bug 557661. The UX was worked out in those bugs, in person at a work week, and after meeting with Legal to discuss privacy issues. The design was tricky to balance, because it had goals that were difficult to reconcile: 1) Out-of-process-plugins was new and also being backported to 3.6. We very much wanted to push users towards submitting crash reports to help ensure OOPP was stable and its impact was understood. 2) The UI needed to be compact and minimal. Having a plugin crash can be a bit disorienting for a user, and isn't a very good time to be asking them to make decisions. "Just fix it!" 3) Respect the user's privacy. We actually had a early design similar to what we want now (see bug 538910 comment 14), but that started getting tricky to implement well (see bug 538910 comment 24), and so we started exploring opt-out designs as being simpler and better for #1 and #2 above. In retrospect the opt-out design hurt #3 above (leading to the changes in bug 550293 and bug 557661), and I ended up finding a way to address my my technical concern (cmt 24) for another need anyway. tl;dr: we considered this design, moved away for other reasons, but should be OK to do it now. On to the review!
Comment 24•13 years ago
|
||
Comment on attachment 559095 [details] [diff] [review] One-Step 'Submit Crash Report' and 'Reload The Page' Link Let's go with the checkbox version.
Attachment #559095 -
Attachment is obsolete: true
Attachment #559095 -
Flags: review?(margaret.leibovic)
Attachment #559095 -
Flags: review?(dolske)
Updated•13 years ago
|
Attachment #559092 -
Attachment is obsolete: true
Updated•13 years ago
|
Attachment #559093 -
Attachment is obsolete: true
Updated•13 years ago
|
Attachment #559094 -
Attachment is obsolete: true
Comment 25•13 years ago
|
||
Comment on attachment 559837 [details] [diff] [review] Alternative Submit Crash Report Screen With Checkbox Review of attachment 559837 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser.js @@ +7133,1 @@ > Now that we've got a checkbox, the checkbox value should mirror the pref... Add a pref observer, using the "mozCleverClosureHack" from a few lines up to avoid leaks. Oh. Except this isn't a real pref, it's just an attribute on nsICrashReporter. Sigh. Let's just fire a new notification when this attribute is toggled so we can observe it here. Bug 540532 implemented the API, you should just need to add a NotifyObservers (nsIObserverService) in nsXULAppInfo::SetSubmitReports(). Let's do that in a separate bug that depends on this one. ::: toolkit/mozapps/plugins/content/pluginProblem.xml @@ +68,5 @@ > <html:div class="msg msgManagePlugins"><html:a class="managePluginsLink" href="">&managePlugins;</html:a></html:div> > <html:div class="submitStatus"> > <!-- links set at runtime --> > + <html:div class="msg msgPleaseSubmit"> > + <xul:checkbox label="&report.please;" class="pleaseSubmitCheckbox" /> Please use <html:input type="checkbox"> here. Shouldn't need to wrap it in a <div>, but it should be hidden upon submission. Also, let's put this after msgReload, so that the UI looks like... :( The Flash plugin crashed. _Reload_the_page_ to try again. [X] …and send crash report.
Attachment #559837 -
Flags: review?(margaret.leibovic)
Attachment #559837 -
Flags: review?(dolske)
Attachment #559837 -
Flags: review-
Comment 26•13 years ago
|
||
Summary of Changes: Made changes suggested by comment 25 Note: Checkbox is to the right of the text as is more usual and will appear on the left in RTL languages.
Attachment #559587 -
Attachment is obsolete: true
Attachment #559837 -
Attachment is obsolete: true
Attachment #561397 -
Flags: review?(dolske)
Comment 27•13 years ago
|
||
Comment 28•13 years ago
|
||
(In reply to Felix Fung from comment #26) > Note: Checkbox is to the right of the text as is more usual and will appear > on the left in RTL languages. I don't understand your reasoning here. Also, when the checkbox is after the reload link, I think it should probably be checked by default. :/ I don't think we need to be religious about opt-in vs opt-out; if it's opt-out, the message just needs to be very clear. Alternatively, as a backup plan, are we at least going to notice when the plugin crashreport rate suddenly drops on certain channels?
Comment 29•13 years ago
|
||
Changes: comment 28 -> My mistake about the positioning of the checkbox As for opt-in of the checkbox, I believe that should be taken care of in bug 688083
Attachment #561397 -
Attachment is obsolete: true
Attachment #561399 -
Attachment is obsolete: true
Attachment #561397 -
Flags: review?(dolske)
Attachment #561411 -
Flags: review?(dolske)
Comment 30•13 years ago
|
||
Additional Changes: - Comment typo fixed - Submit Checkbox checked based on gCrashReporter 'preference'
Attachment #561411 -
Attachment is obsolete: true
Attachment #561411 -
Flags: review?(dolske)
Attachment #562194 -
Flags: review?(dolske)
Comment 31•13 years ago
|
||
Comment on attachment 562194 [details] [diff] [review] Submit Plugin Crash Report Screen Using Checkbox Review of attachment 562194 [details] [diff] [review]: ----------------------------------------------------------------- Close, just a few more things that should be obvious to fix. Looks like you've split some work off to bug 688083. We'll probably want to land both together... ::: browser/base/content/browser.js @@ +6811,5 @@ > BrowserOpenAddonsMgr("addons://list/plugin"); > }, > > + // When user clicks try, checks if we should also send crash report in bg > + retryPluginPage: function (plugin, pluginDumpID, browserDumpID) { Nit: make this function's args be (browser, checkbox, pluginDumpID, browserDumpID). That avoids the need to refetch browser, and may be slightly less future-cleanup for electrolysis (where this code won't be able to touch content elements). @@ +6829,2 @@ > // The crash reporter wants a DOM element it can append an IFRAME to, > // which it uses to submit a form. Let's just give it gBrowser. Hmm. This comment seems to have become obsolete at some point. Looks like bug 548667 or something before it... Remove it while you're here? Thanks! ::: toolkit/locales/en-US/chrome/mozapps/plugins/plugins.dtd @@ +33,5 @@ > <!ENTITY reloadPlugin.pre ""> > <!ENTITY reloadPlugin.middle "Reload the page"> > <!ENTITY reloadPlugin.post " to try again."> > <!-- LOCALIZATION NOTE (report.please): This and the other report.* strings should be as short as possible, ideally 2-3 words. --> > +<!ENTITY report.please "… and send crash report"> No space after the ellipsis and period at the end, I think? (paging grammar nazis…) Also, when changing localized strings, the entity/property name has to be changed so that our localization tools let localizers know something's different. (For spelling fixes, the name can stay the same though.) ::: toolkit/mozapps/plugins/content/pluginProblem.xml @@ +72,3 @@ > <!-- links set at runtime --> > + <html:input id="pleaseSubmitCheckbox" type="checkbox" class="msg pleaseSubmitCheckbox" /> > + <html:label for="pleaseSubmitCheckbox" dir="&locale.dir;" class="msg pleaseSubmitLabel">&report.please;</html:label> Was the |dir| actually needed here for proper RTL appearance? I'd sorta expect the right thing to just automagically happen... ::: toolkit/mozapps/plugins/content/pluginProblemContent.css @@ +53,5 @@ > > +.submitStatus[status="please"] .pleaseSubmitCheckbox, > +.submitStatus[status="please"] .pleaseSubmitLabel { > + display: inline; > +} Hmm, just put the checkbox and label into a wrapper <div>? Then you just need one rule to control both, and it can remain grouped with the other "display: block" rules right below.
Attachment #562194 -
Flags: review?(dolske) → review-
Comment 32•13 years ago
|
||
Made recommended changes above.
Attachment #562194 -
Attachment is obsolete: true
Attachment #562215 -
Flags: review?(dolske)
Comment 33•13 years ago
|
||
Made additional changes requested above. (Forgot to mark content type as patch)
Attachment #562215 -
Attachment is obsolete: true
Attachment #562215 -
Flags: review?(dolske)
Attachment #562216 -
Flags: review?(dolske)
Comment 34•13 years ago
|
||
Comment on attachment 562216 [details] [diff] [review] Submit Plugin Crash Report Screen Using Checkbox Review of attachment 562216 [details] [diff] [review]: ----------------------------------------------------------------- r- for still needing to change the entity name, but looks good otherwise. ::: toolkit/locales/en-US/chrome/mozapps/plugins/plugins.dtd @@ +33,5 @@ > <!ENTITY reloadPlugin.pre ""> > <!ENTITY reloadPlugin.middle "Reload the page"> > <!ENTITY reloadPlugin.post " to try again."> > <!-- LOCALIZATION NOTE (report.please): This and the other report.* strings should be as short as possible, ideally 2-3 words. --> > +<!ENTITY report.please "…and send crash report"> As noted in prior review, you need to change "report.please" to some other sting to make our localization tools notice this change. "report.checkbox" or something. ::: toolkit/mozapps/plugins/content/pluginProblem.xml @@ +72,1 @@ > <!-- links set at runtime --> Tiniest Nit Evar: This comment no longer applies, since there are no <a> links below it to set. Please remove.
Attachment #562216 -
Flags: review?(dolske) → review-
Comment 35•13 years ago
|
||
Oh, hmm, I just realized there's a couple more things. Sorry. :/ 1) The msgSubmitPlease <div> (aka the checkbox+msg) should be hidden once the crash report is submitted (from any instance, via CSS). Having the UI hang around past then isn't useful. 2) As-is, I think you end up with retryPluginPage() submitting the report again if the user clicks "reload page" in a different plugin instance. That's not good. Maybe have the observer removeEventListener + hook it up to reloadPage()?
Comment 36•13 years ago
|
||
Additional Changes: - used ifdef #MOZ_CRASHREPORTER to determine whether link callback was reloadPage or retryPluginPage - report.please -> report.checkbox - cleaned up some comments - only submit crash reports if we've not already done so - removed status/checkbox ui after a report has been sent. ergo, there is no longer any messages showing whether a submissions has succeeded since presumably you've reloaded the page already and instances elsewhere don't care
Attachment #562216 -
Attachment is obsolete: true
Attachment #564165 -
Flags: review?(dolske)
Comment 37•13 years ago
|
||
Comment on attachment 564165 [details] [diff] [review] Submit Plugin Crash Report Screen Using Checkbox Review of attachment 564165 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser.js @@ +6830,5 @@ > + let submitChk = > + doc.getAnonymousElementByAttribute(plugin, "class", "pleaseSubmitCheckbox"); > + > + // Check status to make sure we haven't submitted already > + if (status != "success" && status != "failed" && submitChk.checked) { This should just be | if (status == "please" && .checked)|. Otherwise you can again submit an already-submitting report. I think you're trying to handle resubmitting a failed report, but I don't think that'll work here (we should tackle that as part of an about:crashes revamp). ::: toolkit/mozapps/plugins/content/pluginProblem.xml @@ -71,5 @@ > - <html:div class="msg msgPleaseSubmit"><html:a class="pleaseSubmitLink" href="">&report.please;</html:a></html:div> > - <html:div class="msg msgSubmitting">&report.submitting;<html:span class="throbber"> </html:span></html:div> > - <html:div class="msg msgSubmitted">&report.submitted;</html:div> > - <html:div class="msg msgNotSubmitted">&report.disabled;</html:div> > - <html:div class="msg msgSubmitFailed">&report.failed;</html:div> Why remove the "submitted" and "failed" states?
Comment 38•13 years ago
|
||
Comment on attachment 564165 [details] [diff] [review] Submit Plugin Crash Report Screen Using Checkbox Oops, forgot to mark this.
Attachment #564165 -
Flags: review?(dolske) → review-
Comment 39•13 years ago
|
||
Addressed above comments
Attachment #564165 -
Attachment is obsolete: true
Attachment #567772 -
Flags: review?(dolske)
Comment 40•13 years ago
|
||
Comment on attachment 567772 [details] [diff] [review] Submit Plugin Crash Report Screen Using Checkbox Review of attachment 567772 [details] [diff] [review]: ----------------------------------------------------------------- \o/
Attachment #567772 -
Flags: review?(dolske) → review+
Comment 41•13 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/5df0d4237670
Whiteboard: [fixed-in-fx-team]
Comment 42•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5df0d4237670
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 10
Reporter | ||
Comment 43•13 years ago
|
||
(In reply to Felix Fung (:felix) from comment #29) > Created attachment 561411 [details] [diff] [review] [diff] [details] [review] > Submit Plugin Crash Report Screen Using Checkbox > > Changes: > comment 28 -> My mistake about the positioning of the checkbox > > As for opt-in of the checkbox, I believe that should be taken care of in bug > 688083 Sorry, I can't follow the code submissions and figure out what the final workflow is. I have a couple of questions for documentation: 1. What is the default behavior? If I see the in-content UI for a plugin crash and I just click the link without reading the message, will I send a crash report or not? 2. What is the final wording of the message?
Comment 44•13 years ago
|
||
Reply to comment 43: 1) As soon as the UI appears, there is also a checkbox. The checkbox is attached to the pref of whether the user wants to submit crash reports or not. I believe this is first set on install and most users should have it set one way or the other already. So the checkbox will reflect reality and whether or not a crash report will be sent. 2) In totality it'll look something like: The Foo Plugin has crashed. Reload the page to try again. x] ... and send crash report.
Reporter | ||
Comment 45•13 years ago
|
||
(In reply to Felix Fung (:felix) from comment #44) Beautiful! Thank you so much for fixing this!
Comment 46•12 years ago
|
||
Aurora is supposed to be string frozen and you broke everything (toolkit=every single product we ship) a week before the move to beta. Is this right?
Comment 47•12 years ago
|
||
(In reply to Francesco Lodolo [:flod] from comment #46) > Aurora is supposed to be string frozen and you broke everything > (toolkit=every single product we ship) a week before the move to beta. Is > this right? Sorry, Ignore this... wrong bug (I checked the revision history on central instead of aurora, shouldn't be doing this at 7am...) Right bug is 716945 and Axel is already on that.
Reporter | ||
Comment 48•12 years ago
|
||
It appears that there was a problem with the implementation that caused crash reports to not be sent so this is being backed out (bug 716945). The initial problem still remains then. We now have more reliable numbers on SUMO (due to new webtrends js) and we get about 120,000 visits to the linked article each week with 99% of traffic coming from the UI. We should implement this in a way that doesn't prevent the crash reports from going through.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 49•3 years ago
|
||
We're in the process of removing support for plugins (bug 1677160). Is this bug still relevant after plugins are gone?
Comment 50•2 years ago
|
||
The bug assignee didn't login in Bugzilla in the last 7 months.
:mossop, could you have a look please?
For more information, please visit auto_nag documentation.
Assignee: felix.the.cheshire.cat → nobody
Flags: needinfo?(dtownsend)
Comment 51•2 years ago
|
||
Plugins are no more.
Status: REOPENED → RESOLVED
Closed: 13 years ago → 2 years ago
Flags: needinfo?(dtownsend)
Resolution: --- → INVALID
You need to log in
before you can comment on or make changes to this bug.
Description
•