Change in-content plugin crash UI to submit a crash report and reload the page in one step

Categories

(Firefox :: General, defect)

For bugs in Firefox Desktop, the Mozilla Foundation's web browser. For Firefox user interface issues in menus, bookmarks, location bar, and preferences. Many Firefox bugs will either be filed here or in the Core product. Bugs for developer tools (F12) should be filed in the DevTools product. (more info)

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.

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.

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.

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)

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.

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).

(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?

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.

(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.

(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.

(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.

(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.

(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 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?)

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 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.

(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 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.

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()?

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

(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?

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.

(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.

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 → ---

You need to log in
before you can comment on or make changes to this bug.