404855 - Crash Reporter UI review, round 2

Status: VERIFIED FIXED

(Toolkit :: Crash Reporting, defect, P1)

Description

[(not currently active) Ted Mielczarek]

After living with the crash reporter client redesign for a while, it's obvious it needs a little more work.  I've got a meeting scheduled with beltzner next week (Nov 27th) to talk about this, this bug is to track whatever work we need to do.  Going to mark deps on a few things that are already filed.

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

Attachment: whiteboard image of edited design

ss, luser and I did some whiteboarding and came up with the following mods:

 - move "submit" checkbox closer to request to submit report
 - remove textbox for URL (don't let users edit it, but do let them exclude it)
 - add checkbox and textarea for comments
 - change "View Report..." to a button that loads a secondary dialog
 - add a dynamic label above the close/restart firefox buttons that indicates whether or not a report will be submitted and provides progress indication as it is submitted
 - rename "close" to "quit"
 - have sicorro email the user (if email address provided) with the URL of their crash report once it's been processed

More detailed UI screencaps and such to follow. Text changes for the top paragraphs will be covered in bug 385236.

Comment 2

[(not currently active) Ted Mielczarek]

Attachment: IB mockup

Comment 3

[Reed Loden [:reed]]

"include the URL in the crash report" seems lacking... Maybe "include the URL of the last site in the crash report" or "include the URL of the site you were just viewing in the crash report" or something. The way it currently reads, users might go "what URL?" since it doesn't say what exact URL it is including.

Comment 4

[Adam Kowalczyk]

"URL" is a bit too technical term. Perhaps we should use "location" or "address"?

Comment 5

[Vitorio]

Attachment: Mockups of crash reporter dialog improvements per discussion with luser

I talked with luser (Ted) about this in IRC, and we went back and forth about some potential improvements.

Three states are presented here: the user opting out of submission, the user opting into submission, and the submission in process.

The dialog expands or contracts to present only the relevant options.  An additional bit of text is used to tie the quit/restart buttons to the process described by the rest of the dialog (bug 384608).

Simpler phrasing is used throughout (no novelty text, "page" instead of "URL", "tell" instead of "submit", etc.).  As a typical end user gains no benefit from viewing the report, that option has been removed.

For submission, options are nested under the main checkbox line to show the relationship.  The extraneous email address field and dialog checkbox has been removed in lieu of a single text box with explanatory text within it.  The email address, if provided, can be extracted server-side.

Finally, the entire dialog is grayed out upon submission, and a status field is presented at the very bottom, signaling completion of the top-to-bottom process with an explanation of what is occurring.  If the user had chosen to exit Firefox instead of restarting, it might read, "Thanks.  Sending your report and then quitting..."

Best,
Vitorio Miliano
UX Consultant
Optaros

Comment 6

[John J. Barton]

This looks great! How about:
  1) "Just Quit Firefox"  and "Just Restart Firefox" on the first dialog
This is a three way choice presented as a two buttons and a check.  An extra word will help clarify that it's not a two way choice.
  2) I wonder about the offer to send email.  My parents had a problem on their PC and they called me to complain. They did say yes for "tell Microsoft about this problem" (or whatever Windoze says) multiple times, but Microsoft had never called. I gently suggested that they should not wait any longer. You and I know that if the user gets an email from Firefox "when it is fixed" it would be a miracle and happen six months or a year later. But millions of FF users may expect an email later that day.  A more realistic offer would be "We'll email you a link to a page with technical information on this problem that you can use to monitor our progress in fixing it".  Even that is a stretch. (Correct me if I am being pessimistic here).

Comment 7

[Arie Paap [:wildmyron]]

(In reply to comment #5)
> For submission, options are nested under the main checkbox line to show the
> relationship.  The extraneous email address field and dialog checkbox has been
> removed in lieu of a single text box with explanatory text within it.  The
> email address, if provided, can be extracted server-side.

Would this require entry of an email address for every crash report or would there still be some way for the crash reporter to remember it and prefill the text box with it? It seems to me that doing that would then prevent display of any explanatory text.

Comment 8

[Vitorio]

re: comment #6, I'm not sure it's worth adding additional information to the interface to set those expectations (since it might not be read or understood by a frustrated, skimming user anyway).  An immediate, automated, followup email, thanking the user for their submission, letting them know when they might expect a fix, providing the crash report, etc., might be better, and could serve the additional function of validating the email address.

re: comment #7, luser asked the same question.  I am of the opinion that remembering the email address is not a worthwhile optimization.  Crashes are supposed to happen so infrequently that remembering the email address isn't going to save the user any appreciable time, given how much time the crash itself is wasting.  If they are happening frequently, I suspect the user isn't going to want a slew of emails about what's likely to be the same issue, over and over.

Comment 9

[John J. Barton]

(In reply to comment #8)
> re: comment #6, I'm not sure it's worth adding additional information to the
> interface to set those expectations (since it might not be read or understood
> by a frustrated, skimming user anyway).  An immediate, automated, followup
> email, thanking the user for their submission, letting them know when they
> might expect a fix, providing the crash report, etc., might be better, and
> could serve the additional function of validating the email address.

Ok, so not additional information, but at least accurate information. "Please include your email address if you would like to receive addition information on  this issue; your address will not be included on our web site".

I think you should tell people that comments in this box will be posted publicly.

> 
> re: comment #7, luser asked the same question.  I am of the opinion that
> remembering the email address is not a worthwhile optimization.  Crashes are
> supposed to happen so infrequently that remembering the email address isn't
> going to save the user any appreciable time, given how much time the crash
> itself is wasting.  If they are happening frequently, I suspect the user isn't
> going to want a slew of emails about what's likely to be the same issue, over
> and over.
> 
Please, please remember the info for reuse! Its no unusual for FF to crash on me 10 times in a row. Yes on average crashes are infrequent, but once you create the conditions for FF to crash once, the probability goes way up that you will crash a second and third time.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

While the cleanliness of the design proposed in comment 5 is definitely desired, it fails to meet two of the primary goals, which are:

 - encourage people to submit crash reports
 - encourage people to add comments to aid in crash reports

I'm not naive enough to believe that the majority of people will *want* to add comments (Sam says we get about 20% response rate on comments) but I *do* think we should be making it easier for people who do.

It's also really important to allow users to see the full report being sent to Mozilla for privacy reasons.

The string-work in the design is great, though, and you're right, the fields beneath the "Tell Mozilla about the crash so they can fix it" need to be indented.

Comment 9 is also correct; we should be remembering the email address so that users don't have to enter it repeatedly.

   Regret and restoration

   Sorry, Firefox seems to have crashed. Click below to
   restart with your tabs and windows restored to what you
   were doing.

   [X] Tell Mozilla about this crash to they can fix it
        [Add a comment...                                ]
        [                                                ]
        [X] Include the address of the page I was on
        [ ] Tell Mozilla to email me with more information
            [                                            ]

  
   ( Quit )                          (( Restart Firefox ))    
   

 - the "Add a comment..." should be grey-text in the comment field
 - Xs show default selection

I like the idea of showing the throbber and the "Thanks! Sending your report and restarting Firefox..." after clicking "Restart Firefox".

Comment 11

[Vitorio]

Attachment: Mockups of crash reporter dialog per further discussion

Re: comment #10, this version tightens up the language further, restores the crash report viewing button and restores the separate email address field.

The language is now consistent re: "telling Mozilla."  The technical term "crash report" is not used.  The user is asked for their help.  The same implicit usage for the comment text field is also used for the email address, rather than a separate checkbox to signal that the user wants to be contacted.  The "Or:" and "Then:" text has been replaced with a more explanatory line that works in both the opt-in (default) and opt-out cases.  The crash report viewing button does change tense depending on whether you opt-in or opt-out.

Best,
Vitorio Miliano

Comment 12

[John J. Barton]

(In reply to comment #11)

Looks great Vitorio. (I don't understand what is going on in the last panel of your mockup).

Another wording:
"We'd appreciate your help. If you tell us about this crash, we'll try to fix the problem"

I think the "It's not a trap" will be translated by most humans into "Oh, this is probably a trap".  I think something like "Firefox Quality Improvement" makes more sense.  

Comment 13

[(not currently active) Ted Mielczarek]

I absolutely veto anything that sounds like the old Talkback dialog "Quality Feedback" whatever nonsense.  The window title is staying as "Crash Reporter", because that's what it is.  I don't think "crash" is a technical term.  People know that software crashes.

Comment 14

[(not currently active) Ted Mielczarek]

Going to have to fix this while I'm here:
http://people.mozilla.com/~tmielczarek/crashreporter-mac-pl.png

Comment 15

[(not currently active) Ted Mielczarek]

Ok, here's a Mac build with my crashreporter rework:
http://people.mozilla.com/~tmielczarek/firefox-3.0b3pre.en-US.mac.dmg

I'd appreciate some testing and feedback, especially by heavy Mac users.  I changed the default crash report server to the staging server, so don't worry about submitting junk crash reports.  In fact, you could even install bsmedberg's Memory Corrupter extension to crash a lot (use a new profile!):
http://benjamin.smedbergs.us/tests/memory-corrupter-0.2.xpi

This build should fix everything in the dep list except bug 385236.

Comment 16

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I spent a couple hours today crashing today, so some feedback:

1) Return in the text field submits the crash report; if a reporter wants to start a new line (and they will), this will be aggravating.

2) Copy and Paste don't work in the text field; again, unexpected and aggravating ;)  Possibly enabling the Edit menu will be all it takes to fix this?

2b) Likewise, bug 393991 remains unfixed.

3) No scrollbar appears when you have more than two lines of text.

4) The text field is so small as to discourage more than a word or two of comments (particularly given it's using the Large System Font).  I think we want to provide a few more lines by default, and if possible allow resizing of the field.  

It's very hard to write anything more than a word or two in a very small text field (this one is about equivalent to the default size of the review comments field in bugzilla).  We don't want to encourage huge comments or pastes of usually-irrelevant info, but the current size is very annoying.

5) Since the db has space for 500 characters (characters, or bytes? thinking about the double-byte half of UTF-8 here), we need to tell the text field to stop accepting user input at 500 chars, so they don't write a big long comment with the payoff at the end, only to have the report QA gets to see be cut off in the middle of the good stuff ;)

6) The text in the "details" sheet looks very cramped; perhaps use the Small System Font instead, and/or include the resize widget so that users can expand the sheet width-wise to view all the info without wrapping?

7) On the Mac, "OK" is standard, not "Ok", but maybe "Close" is better? (I'm trying to find an example of another informational sheet to see what it might use.)

8) When the "Tell Mozilla to email me with more information" box is unchecked, the email address field should be disabled (unfocusable, and any remembered email address should switch to disabled text).

9) The initial focus being in the email address field seems a little odd to me; it should always be on the first text field (when Full Keyboard Access is off) or the first control ("Tell Mozilla about this crash…"; when FKA is on).  I understand the rationale for not having it in the comments field with FKA is off, since that clears the hint text, but it still feels odd.  The FKA focus is  wrong however you look at it, though.

10) When you get the error window instead of the chance to send a report, the "Quit" button is not set as default button (it took me 20-30 constant crashes to so corrupt something that I got this; I didn't have FKA on at the time, but I presume the Quit button also wasn't set as initial first responder, either).

11) When I've previously opted not to send a report, then send a report with no comments or email address, then crash again, sending a report is disabled instead of enabled on the next crash.  

12) A couple of times, at some point I was no longer able to get an email address to persist.  This seemed to involve deleting the email address (select, delete), then submitting, and then several subsequent attempts to enter the same or new addresses didn't stick to the crashes after them.  Unfortunately I don't have good STR for this :(

Those were the only non-persistence bugs I found.

13) The "Quit" button looks very odd all the way over to the left; on the Mac, it should be only 12px to the left of the "Restart Firefox" button.

14) Even knowing how this is supposed to work, the button titles strike me as a bit odd each time I see them. Can we maybe change the buttons to read "Submit Report and {Restart|Quit}" when the "Tell Mozilla about this crash" checkbox is enabled?  

(Or maybe I don't understand exactly; is a report only submitted when Firefox restarts when "Tell Moz" is checked?  The "Your crash report will be submitted when you restart" text implies that. If the "Quit" button never submits a report, it should read "Quit and Don’t Send" or something at all times. Otherwise, the explanatory text needs to be changed.)

There are definitely some issues with l10n and Cocoa button sizing, so I realize this is thorny, but it's also still the most confusing part of the whole reporter client right now...just when is a report submitted?

15) Bug 382540 is still a problem, but we're also launching Minefield in such a way that its "You Crashed. Restore Session?" can end up hidden behind other apps' windows.  Can Crash Reporter send -foreground to Firefox when relaunching?

16) I'm running a Minefield build, but Crash Reporter keeps talking about "Firefox".  Is this covered by the "make Crash Reporter suitable for Thunderbird/Songbird" bugs, or is this a new bug where CR really is pulling the wrong appname?

There are a lot of comments here about small things; the good thing is that the big things are mostly OK, and even some of the small details that often get missed ("Quit" being the first responder when "Tell Moz about the crash" is unchecked, persistance of user settings, etc.) are working, too.

When the staging server is accepting reports and has comment-collection enabled, I'll be sure to play with sending some non-Roman and multi-script comments.

Comment 17

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

17) Esc should probably map to "Quit" if "Quit" *doesn't* submit the report.  (If "Quit" does submit a report, then maybe not, since Esc generally means "get me out of here without doing anything".)

Also, some nib nit-picks:

18) Your nib has some cruft in it, as I'm sure you know; there's also an IB backup file shipped in this build ;)

19) A number of the items are closer to the sides of the window than 20px (both in the main window and in the ErrorView).

20) "Tell Mozilla to email me" is only 6px from "Include the address of the page" instead of 8px like everything else.

21) The indented items should probably be indented 17px (at the very least, indented consistently ;) ).

22) You need at least 16px between the buttons and the text above them.

Comment 18

[(not currently active) Ted Mielczarek]

Thanks for the insanely detailed feedback!  I'll read more thoroughly tomorrow and reply, and make it not suck.  Sounds like if I make you happy then everyone should be ok.  :-)

Comment 19

[(not currently active) Ted Mielczarek]

Attachment: the home game

Vito asked if I had a screenshot so he could follow along with Smokey's comments.  Here's a composite of five screenshots showing various states of the crashreporter.

Comment 20

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

The other thing that occurred to me this morning is that we may want to tweak the text field hint text to indicate that comments will be public (i.e., not to submit any private info in the comment).

Comment 21

[(not currently active) Ted Mielczarek]

Just a few comments, if I don't comment on something I'm probably working on it (or planning to).

(In reply to comment #16)
> 4) The text field is so small as to discourage more than a word or two of
> comments (particularly given it's using the Large System Font).  I think we
> want to provide a few more lines by default, and if possible allow resizing of
> the field.  

How big would you like it to be?  I agree that it's sort of small (only two lines), but I also don't want a huge comment field taking up a ton of space on the form.  Any suggestion here?

> 9) The initial focus being in the email address field seems a little odd to me;
> it should always be on the first text field (when Full Keyboard Access is off)
> or the first control ("Tell Mozilla about this crash…"; when FKA is on).  I
> understand the rationale for not having it in the comments field with FKA is
> off, since that clears the hint text, but it still feels odd.  The FKA focus is
>  wrong however you look at it, though.

I have to say I don't even understand why Apple has that as an option, and on by default, no less.  I think it's stupid.  I can easily set the "Tell Mozilla" checkbox as the first responder, but that just means that anyone who hasn't turned on FKA is going to wind up focused in the comment box, which will be confusing.  I'm not even sure what we'd do in the non-FKA case.  The only things that are focusable are the comment or email fields.  I'm open to ideas.


> 14) Even knowing how this is supposed to work, the button titles strike me as a
> bit odd each time I see them. Can we maybe change the buttons to read "Submit
> Report and {Restart|Quit}" when the "Tell Mozilla about this crash" checkbox is
> enabled?  
> 
> (Or maybe I don't understand exactly; is a report only submitted when Firefox
> restarts when "Tell Moz" is checked?  The "Your crash report will be submitted
> when you restart" text implies that. If the "Quit" button never submits a
> report, it should read "Quit and Don’t Send" or something at all times.
> Otherwise, the explanatory text needs to be changed.)

The "Quit" button quits without sending.  I think I'll re-label it to "Quit without sending" to make this clear.  You can only submit when you click "Restart".  This was part of our plan to make the buttons less confusing, apparently it wasn't entirely successful.  Hopefully with renaming the "Quit" button, and the explanatory text, this should be clearer.

> 15) Bug 382540 is still a problem, but we're also launching Minefield in such a
> way that its "You Crashed. Restore Session?" can end up hidden behind other
> apps' windows.  Can Crash Reporter send -foreground to Firefox when
> relaunching?

That's unfortunate.  We could probably jam a -foreground in there if it doesn't already exist, although I'm wary of screwing with the commandline.

> 16) I'm running a Minefield build, but Crash Reporter keeps talking about
> "Firefox".  Is this covered by the "make Crash Reporter suitable for
> Thunderbird/Songbird" bugs, or is this a new bug where CR really is pulling the
> wrong appname?

No, Minefield identifies itself as Firefox in application.ini, so that your profile stays in the same place.  The crash reporter gets these values from application.ini, so it works properly with other apps.  I'm not really worried about it referring to Minefield here, since it's correct with official branding.

(In reply to comment #17)
> 18) Your nib has some cruft in it, as I'm sure you know; there's also an IB
> backup file shipped in this build ;)

The backup file should go away, that's just an artifact of my build environment.  :-)  I'm sure there's cruft in the nib, but since I'm barely capable at Cocoa, I have a hard time identifying what is cruft.  Could you explicitly call out anything you've noticed?
 
> 19) A number of the items are closer to the sides of the window than 20px (both
> in the main window and in the ErrorView).
>
> 20) "Tell Mozilla to email me" is only 6px from "Include the address of the
> page" instead of 8px like everything else.

I've just been using IB's guides here.  Is it wrong, or is something else mucking that up?

Thanks again for the detailed feedback.

Comment 22

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #21)
> (In reply to comment #16)
> > 4) The text field is so small as to discourage more than a word or two of
> 
> How big would you like it to be?  I agree that it's sort of small (only two
> lines), but I also don't want a huge comment field taking up a ton of space on
> the form.  Any suggestion here?

3, or at most 4 lines.  That seems to make a huge improvement without adversely affecting the size of the app.  We could perhaps switch to the Small System Font and gain an additional extra line; I'm trying to find other apps that use NSTextFields for text entry and which use the smaller font, but so far no luck.

> > 9) The initial focus being in the email address field seems a little odd to me;
> > it should always be on the first text field (when Full Keyboard Access is off)
> 
> I have to say I don't even understand why Apple has that as an option, and on
> by default, no less.  I think it's stupid.  I can easily set the "Tell Mozilla"
> checkbox as the first responder, but that just means that anyone who hasn't
> turned on FKA is going to wind up focused in the comment box, which will be
> confusing.  I'm not even sure what we'd do in the non-FKA case.  The only
> things that are focusable are the comment or email fields.  I'm open to ideas.

Actually, I was wrong here; the same thing (in views with text field(s), the first text field) should be the first responder in both cases.  

It's still a little odd to focus the email field instead of the comment field, but given that the placeholder text is acting as hint text and is the only thing that indicates what the comment field is for (which is probably not good UI, looking back on it), I understand the dilemma.  

Perhaps we should add an actual label ("Comments" ?) outside the comments field? 


> > 14) Even knowing how this is supposed to work, the button titles strike me as a
> > bit odd each time I see them. 
> 
> The "Quit" button quits without sending.  I think I'll re-label it to "Quit
> without sending" to make this clear.  You can only submit when you click
> "Restart".  This was part of our plan to make the buttons less confusing,
> apparently it wasn't entirely successful.  Hopefully with renaming the "Quit"
> button, and the explanatory text, this should be clearer.

Sounds good.  In that case, Esc should definitely be bound to "Quit".

> > 18) Your nib has some cruft in it, as I'm sure you know; there's also an IB
> > backup file shipped in this build ;)
> 
> The backup file should go away, that's just an artifact of my build
> environment.  :-)  I'm sure there's cruft in the nib, but since I'm barely
> capable at Cocoa, I have a hard time identifying what is cruft.  Could you
> explicitly call out anything you've noticed?

Sure.  The EnableView and UploadingView views don't seem to be used any more.

> > 19) A number of the items are closer to the sides of the window than 20px (both
> > in the main window and in the ErrorView).
> >
> > 20) "Tell Mozilla to email me" is only 6px from "Include the address of the
> > page" instead of 8px like everything else.
> 
> I've just been using IB's guides here.  Is it wrong, or is something else
> mucking that up?

Tempermental? :P  IB guides like to snap you to both the Aqua guidelines and to align with any nearby object boundary, so if one thing was off, it will blithely snap other objects into alignment there.  

A couple of things (the bold font lines) measure differently between IB 2.x and 3.x, which is annoying.  FWIW, in Camino we've been treating the IB 2.x measurements as canonical and using IB 2.x for all our nib work since it's available on both 10.4 and 10.5.

The spacing between those two adjacent checkboxes is just flat-out wrong, despite IB snapping there (that, or Apple have changed their mind and not updated their spacing guidelines on the web, which say 8px vertically for full-size).

Other than the items I specifically mentioned before, it should be just a matter of selecting each object in the window, holding down Option and moving your mouse outside the object's bounds, and seeing what the distances from the edges of the window are (mostly a bunch of right edges are 21 or 12 or 0px away, instead of twenty, and then some left edges of non-indented controls are 17px instead of 20px), and then resizing appropriately.

You can ping me if you want to go into more detail on the alignment stuff.

> Thanks again for the detailed feedback.

My pleasure.

Comment 23

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #22)
> 3, or at most 4 lines.  That seems to make a huge improvement without adversely
> affecting the size of the app.  We could perhaps switch to the Small System
> Font and gain an additional extra line; I'm trying to find other apps that use
> NSTextFields for text entry and which use the smaller font, but so far no luck.

Comments field in "Get Info" windows in the Finder, and I was told NetNewsWire also does for its "Get Info" on feeds, so we could use that (just switch the text field size to "Small" in IB's Inspector) and not seem out of place.

Comment 24

[(not currently active) Ted Mielczarek]

Ok, I have fixed all but one of Smokey's numbered points (except for a few things I couldn't reproduce).  I believe I have also fixed the handling of longer strings from l10n that I mentioned in comment 14.  Anything not explicitly mentioned here is fixed, I'm only mentioning things with a comment:

4) Fixed (sized up to several lines)
6) Fixed (switched to small system font)
7) Fixed (changed to OK)
9) Fixed, but not happy.  (first responder is the "tell mozilla" checkbox, but if FKA is off it winds up in the comments box, which sucks)
10) Fixed (Quit button set as first responder and default button in error dialog)
11) This WFM
12) Not really sure about this, just using a binding in IB, there's no custom code to do this.
13) Fixed (Put back where it belongs)
14) Fixed (Renamed to "Quit without sending")
16) N/A
17) Fixed (ESC maps to quit)

I didn't fix #15 (Firefox can launch behind crash reporter), can you file a follow-up on it?

I'll have a build up with all these changes shortly.

Comment 25

[(not currently active) Ted Mielczarek]

My patch also fixes bug 401960.

Comment 26

[(not currently active) Ted Mielczarek]

Ok, new Mac build up: http://people.mozilla.com/~tmielczarek/firefox-3.0b3pre.en-US.mac.dmg

Comment 27

[(not currently active) Ted Mielczarek]

Attachment: mac changes + strings

Mac changes, string changes, and 1-line win32/linux changes so as not to break them.

Comment 28

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #24)

> 9) Fixed, but not happy.  (first responder is the "tell mozilla" checkbox, but
> if FKA is off it winds up in the comments box, which sucks)

mento is a good choice of reviewer to hash this out with :)

> I didn't fix #15 (Firefox can launch behind crash reporter), can you file a
> follow-up on it?

Bug 411455
 
> I'll have a build up with all these changes shortly.

I'll try the build in the next couple of days :)

One quick question about the strings: I'm not familiar with toolkit style, but should the elipses be proper typographical elipses (…) instead of three periods (...)?  On Mac OS X, besides it being platform style, Apple's Accessibility docs note that use of the proper typographical elipsis is important for how VoiceOver presents information about those objects to the user.

Comment 29

[Jesse Ruderman]

> The "Quit" button quits without sending.  I think I'll re-label it to "Quit
> without sending" to make this clear.  You can only submit when you click
> "Restart".  This was part of our plan to make the buttons less confusing,
> apparently it wasn't entirely successful.  Hopefully with renaming the "Quit"
> button, and the explanatory text, this should be clearer.

So there's no way to send the crash report without restarting Firefox?  I don't see the point of tying those two things together.  I think the buttons should be "Don't restart" and "Restart", and both should obey the checkbox for whether to send the crash data to Mozilla.

Comment 30

[(not currently active) Ted Mielczarek]

Jesse: you can see some more discussion on this in bug 384608, but it's generally felt that the current behavior is confusing.  If this screws up your workflow somehow, we can add environment variables or other magic to make your life easier.

Comment 31

[Jesse Ruderman]

I imagine combining the two choices interferes with some users' workflow too, not just testers' workflow.  What's the advantage of tying the two choices and adding explanatory text over separating them in the way I proposed?

Comment 32

[(not currently active) Ted Mielczarek]

My user scenarios consist of "get me the hell out of here" (quit) and "get me back to what the hell i was doing" (restart).  submitting the crash report is probably incidental to normal users, so we enable it by default and assume that restarting will be the most common action, in which case it will be submitted.

The advantage of tying the two choices is that it actually lessens the amount of thought required.  Please see bug 384608 for some actual user confusion with the existing buttons.

Comment 33

[Jesse Ruderman]

The reporter of bug 384608 is a Mozilla community member, so I don't think it makes sense to point that that bug as an example of "actual user confusion".  And he seems to be arguing the same thing that I am: that tying the choices increases confusion rather than decreasing it.

If submitting the crash report is incidental, what's wrong with submitting it in the case where the user selects "Don't relaunch" instead of "Relaunch"?

Comment 34

[(not currently active) Ted Mielczarek]

Here's a Windows build to test:
http://people.mozilla.com/~tmielczarek/firefox-3.0b3pre.en-US.win32.zip

Also I wrote an extension to crash easily:
http://ted.mielczarek.org/code/mozilla/crashme.xpi
(Tools -> Crash me now!)

Comment 35

[John J. Barton]

Ted, I tried you 3.0b3pre: this is a huge improvement and overall I think its very nice.

I still think the "show me what is sent" bit is a lie: I know much more than this is sent.  Also the window used for the view of sent stuff is microscopic.

When I selected quit-without-sending the UI was kinda funky and I thought it says thanks for submitting.

I didn't get an email, still waiting....

I didn't need your crashme as the browser never successfully came up in six or seven tries.

Comment 36

[(not currently active) Ted Mielczarek]

FWIW, there's no email yet, that's a server-side thing that we have yet to implement.

Can you be more specific on the "quit-without-sending" thing?  It should just close the window, it shouldn't change anything.

I'll make the "view report" window larger.  I know it's not exhaustive, that's why we added the blurb at the bottom "...contains information about the state of the application..." but the minidump is just binary data, so there's nothing there we can effectively show the user.

Comment 37

[Vitorio]

Awesome.

On Windows, the greytext is a different font (System?) than the entry text.

It took me a moment to realize that you have to check the email box above the email field in order to enable the email field, which isn't enabled by default.  Could clicking on the email text entry field also check the box and enable it, like clicking on the label does?

"Quit without sending" doesn't make sense when "Tell Mozilla about this crash" is unchecked, since then you're obviously not going to send anything.  I still believe "Quit" is sufficient in both scenarios, but if others insist, maybe it could shorten to just "Quit" when the "Tell Mozilla" box is unchecked?

Comment 38

[(not currently active) Ted Mielczarek]

Ok, I made the view report dialog larger, fixed the font in the graytext, and made the email field enable+check the checkbox when you click on it and it's disabled.  I uploaded a new build to the same url from comment 34 if you're interested.

I agree that the button labelling isn't the best, but at least "Quit without Sending" is always true.  Changing the button text based on a checkbox just feels wrong.

Comment 39

[Vitorio]

Attachment: Animation showing the changing of the quit button based on a checkbox

Where is the "include the URL" checkbox?  Does that not show up if I use the CrashMe extension?

The larger View Report dialog is much better.  I notice the text at the bottom of that reads, "This report also contains information about the state of the application when it crashed."  My first thought is, I thought that's why I clicked "See what's included," to see this information.  What additional information is included?  Stack trace?  Should that line be clarified, e.g. "This report also contains technical information...?"

The comment field says, "Note: Comments are publicly visible!"  Crash reports are visible to the general public?  Also, no need to capitalized "Comments."

The Crash Reporter has an icon in my taskbar, but not in the window that appears when I Alt-Tab between applications.  It has the generic Windows Application icon there.

Is there a reason the spinner is to the left of the restart text instead of to the right of it?  Your last action was to click "Restart Firefox," so the result (the spinner) should be near there so you don't miss the resulting indicator that something is now happening.  I did miss it the first time.

Did you actually try out changing the quit button?  I understand that it feels wrong, but I think it works well in practice (animation attached).

Don't forget the top text.

Comment 40

[(not currently active) Ted Mielczarek]

(In reply to comment #39)
> Where is the "include the URL" checkbox?  Does that not show up if I use the
> CrashMe extension?

It doesn't show up if there's no URL in the report.  Maybe you crashed without loading a URL first?

> The larger View Report dialog is much better.  I notice the text at the bottom
> of that reads, "This report also contains information about the state of the
> application when it crashed."  My first thought is, I thought that's why I
> clicked "See what's included," to see this information.  What additional
> information is included?  Stack trace?  Should that line be clarified, e.g.
> "This report also contains technical information...?"

Yeah, it's binary data including the contents of the stack, so it's not human-readable.  We could clarify that somehow.

> The comment field says, "Note: Comments are publicly visible!"  Crash reports
> are visible to the general public?  Also, no need to capitalized "Comments."

Yes, crash reports are publicly visible, as they always have been.  (Although we hide the URL and email address).
 
> The Crash Reporter has an icon in my taskbar, but not in the window that
> appears when I Alt-Tab between applications.  It has the generic Windows
> Application icon there.

Oh, huh, I never noticed that.  I'll have to fix that.

> Is there a reason the spinner is to the left of the restart text instead of to
> the right of it?  Your last action was to click "Restart Firefox," so the
> result (the spinner) should be near there so you don't miss the resulting
> indicator that something is now happening.  I did miss it the first time.

That's just where we figured we'd put it.  I would think the animation would draw your eye to it, but I honestly don't care where it goes.

> Did you actually try out changing the quit button?  I understand that it feels
> wrong, but I think it works well in practice (animation attached).

No, I didn't.  I still remain unconvinced, but maybe I'll try it out and see how I feel about it.

> Don't forget the top text.

Right, we need to nail that down.  I'm thinking we'd go with your suggestion of "We're Sorry" in the bold text, then modify the subtext appropriately.

Comment 41

[Vitorio]

(In reply to comment #40)
> It doesn't show up if there's no URL in the report.  Maybe you crashed without
> loading a URL first?

Oh, nice.  Yes, it shows up when I crash after going to a URL.

Comment 42

[(not currently active) Ted Mielczarek]

Requesting blocking.  Several of the deps here are blockers, they're all getting fixed in this bug.

Windows/Linux patches, and a Linux test build coming up soon.

Comment 43

[(not currently active) Ted Mielczarek]

Attachment: string changes [checked in]

Just the string changes.

Comment 44

[(not currently active) Ted Mielczarek]

Attachment: mac patch

Comment 45

[(not currently active) Ted Mielczarek]

Attachment: win32 patch [checked in]

Comment 46

[(not currently active) Ted Mielczarek]

Attachment: Linux patch

Plus a Linux build to test:
http://people.mozilla.com/~tmielczarek/firefox-3.0b3pre.en-US.linux-i686.tar.bz2

Comment 47

[(not currently active) Ted Mielczarek]

Comment on attachment 297807 [details] [diff] [review]
win32 patch [checked in]

I missed it in this patch, but this also includes the new file toolkit/crashreporter/client/Throbber-small.avi, which is just Throbber-small.gif from winstripe converted to avi form.

Comment 48

[(not currently active) Ted Mielczarek]

Attachment: Linux patch [checked in]

Of course I forgot to add the throbber to the packaging files.  Re-uploaded the Linux build, since the previous one was probably missing the throbber.

Comment 49

[Vitorio]

Speaking of string changes, to fix the voice in the new "We're Sorry" version:

"Firefox hit an unexpected problem and crashed. It will try to restore your tabs and windows when it restarts.

To help Mozilla diagnose and repair this problem, you can send them a crash report."

Comment 50

[Vitorio]

Ryan suggested (and I agreed): "Firefox had a problem and crashed," as "unexpected" is redundant and "hit" is atypical.

Comment 51

[Dave Camp (:dcamp)]

Comment on attachment 297823 [details] [diff] [review]
Linux patch [checked in]

>Index: toolkit/crashreporter/client/crashreporter_linux.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/crashreporter/client/crashreporter_linux.cpp,v
>retrieving revision 1.13
>diff -u -p -5 -r1.13 crashreporter_linux.cpp
>--- toolkit/crashreporter/client/crashreporter_linux.cpp	4 Dec 2007 15:54:20 -0000	1.13
>+++ toolkit/crashreporter/client/crashreporter_linux.cpp	18 Jan 2008 19:07:21 -0000

>+  // and spawn a thread to do the sending
>+  pthread_create (&sendThreadID, NULL, SendThread, NULL);
> }

It's typical to use GThread (g_thread_create()/g_thread_join()), although I can't think of any concrete reason why it would matter.

>+  // Get the throbber image from alongside the executable
>+  char* throbber_path = new char[strlen(gArgv[0])+18];
>+  strcpy(throbber_path, gArgv[0]);
>+  char* dot = strrchr(throbber_path, '/');
>+  dot++;
>+  strcpy(dot, "Throbber-small.gif");

This could be:

char *dir = g_path_get_dirname(gArgv[0]);
char *path = g_build_filename(dir, "Throbber-small.gif");
g_free(dir);

Looks fine other than that.

Comment 52

[Dave Camp (:dcamp)]

Comment on attachment 297807 [details] [diff] [review]
win32 patch [checked in]

>--- toolkit/crashreporter/client/crashreporter_win.cpp	14 Dec 2007 19:47:19 -0000	1.20
>+++ toolkit/crashreporter/client/crashreporter_win.cpp	18 Jan 2008 18:35:36 -0000
>@@ -58,10 +58,11 @@
> #define SUBMIT_REPORT_VALUE  L"SubmitReport"
> #define INCLUDE_URL_VALUE    L"IncludeURL"
> #define EMAIL_ME_VALUE       L"EmailMe"
> #define EMAIL_VALUE          L"Email"
> #define MAX_EMAIL_LENGTH     1024
>+#define MAX_COMMENT_LENGTH   500

Should that constant be shared somewhere?

Comment 53

[Dave Camp (:dcamp)]

Comment on attachment 297804 [details] [diff] [review]
string changes [checked in]

>Index: toolkit/locales/en-US/crashreporter/crashreporter.ini
>===================================================================

>+ViewReportTitle=Report Contents
>+CommentGrayText=Add a comment.  Note: Comments are publicly visible!
>+ExtraReportInfo=This report also contains technical information about the state of the application when it crashed.
>+# LOCALIZATION NOTE (CheckSubmit): The %s is eplaced with the vendor name.

Typo: s/eplaced/replaced/

Comment 54

[Mike Schroepfer]

Yep - want this for b3

Comment 55

[Axel Hecht [:Pike]]

The string changes are calling for key changes to make localizers pick those up, I guess. Sad, but true.

Comment 56

[Mike Connor [:mconnor]]

Comment on attachment 297804 [details] [diff] [review]
string changes [checked in]

some minor kvetches about cross-app strings, but those are in another bug

Comment 57

[(not currently active) Ted Mielczarek]

Comment on attachment 297804 [details] [diff] [review]
string changes [checked in]

Checked in with vito's suggestion from comment 50.  I renamed all the keys that changed materially in content, except for one that I plan to change tomorrow anyway.  I left the "Close" string in there, since I'm not checking in all three platforms at once.  Hopefully I'll remember to nuke it later.

Comment 58

[(not currently active) Ted Mielczarek]

Comment on attachment 297807 [details] [diff] [review]
win32 patch [checked in]

Moved MAX_COMMENT_LENGTH to crashreporter.h, checked in.

Comment 59

[(not currently active) Ted Mielczarek]

Attachment: Linux build bits [checked in]

I left a Makefile change out of the Linux patch, then realized I'd left myself a TODO there since I started linking gthread in the client.  This is the configure change needed to do that right + the makefile change.

Comment 60

[Ondrej Brablc]

I cannot build my Mozilla anymore due to this commit:

bug 404855 - crash reporter UI review, round 2--win32+strings changes.  r=dcamp

It is 1.21 revision of crashreporter_win.cpp

c/usr/src/trunk/mozilla/toolkit/crashreporter/client/crashreporter_win.cpp
crashreporter_win.cpp
c:/usr\src\trunk\mozilla\toolkit\crashreporter\client\crashreporter_win.cpp(521) : error C3861: 'wcsncpy_s': identifier not found, even with argument-dependent lookup
make[2]: *** [crashreporter_win.obj] Error 2
make[2]: Leaving directory `/cygdrive/c/usr/src/trunk/mozilla/_obj-browser-debug/toolkit/crashreporter/client'
make[1]: *** [libs] Error 2
make[1]: Leaving directory `/cygdrive/c/usr/src/trunk/mozilla/_obj-browser-debug/toolkit/crashreporter'
make: *** [all] Error 2

I'm on Windows with Visual C++ 7.1. Do I have to upgrade or can this be fixed?

Comment 61

[Reed Loden [:reed]]

(In reply to comment #60)
> c:/usr\src\trunk\mozilla\toolkit\crashreporter\client\crashreporter_win.cpp(521)
> : error C3861: 'wcsncpy_s': identifier not found, even with argument-dependent
> lookup
...
> I'm on Windows with Visual C++ 7.1. Do I have to upgrade or can this be fixed?

Sounds like that code should be using WindowsStringUtils::safe_wcsncpy(), as per http://mxr.mozilla.org/firefox/source/toolkit/crashreporter/google-breakpad/src/common/windows/string_utils-inl.h#113.

Comment 62

[Ondrej Brablc]

Attachment: Fix compilation problem on VC++ 7.1 [checked in]

This is a fix for patch 297807. I did not test it (have no idea how), just replaced the function call so that it compiles.

Comment 63

[(not currently active) Ted Mielczarek]

Comment on attachment 298432 [details] [diff] [review]
Fix compilation problem on VC++ 7.1 [checked in]

Oops, I don't know how that snuck in there, I should know better.  I'll check this in for you.

Comment 64

[(not currently active) Ted Mielczarek]

Comment on attachment 298432 [details] [diff] [review]
Fix compilation problem on VC++ 7.1 [checked in]

Checked in, except I used MAX_COMMENT_LENGTH+1 for the second argument (length of destination buffer).  Thanks for the patch!

Comment 65

[(not currently active) Ted Mielczarek]

Comment on attachment 297823 [details] [diff] [review]
Linux patch [checked in]

Checked in with dcamp's comments addressed.

Comment 66

[Josh Aas]

from crashreporter_osx.mm

============================
-(void)showReportInfo
{
  NSDictionary* boldAttr = [NSDictionary
                            dictionaryWithObject:[NSFont boldSystemFontOfSize:0]
                                          forKey:NSFontAttributeName];
  NSDictionary* normalAttr = [NSDictionary
                              dictionaryWithObject:[NSFont systemFontOfSize:0]
                                            forKey:NSFontAttributeName];

  [viewReportTextView setString:@""];
  for (StringTable::iterator iter = gQueryParameters.begin();
       iter != gQueryParameters.end();
       iter++) {
    [[viewReportTextView textStorage]
     appendAttributedString: [[NSAttributedString alloc]
                              initWithString:NSSTR(iter->first + ": ")
                                  attributes:boldAttr]];
    [[viewReportTextView textStorage]
     appendAttributedString: [[NSAttributedString alloc]
                              initWithString:NSSTR(iter->second + "\n")
                                  attributes:normalAttr]];
  }

  [[viewReportTextView textStorage]
   appendAttributedString: [[NSAttributedString alloc]
                            initWithString:NSSTR("\n" + gStrings[ST_EXTRAREPORTINFO])
                            attributes:normalAttr]];
}
============================

All of the appends in the code above leak. When you alloc those attributed strings, you get a string with a retain count of 1. It is your responsibility to handle the release. Please explicitly release those strings after using them.

>  mPost = [[HTTPMultipartUpload alloc] initWithURL: url];

This also leaks. Assuming HTTPMultipartUpload follows normal obj-c memory mgmt rules, you allocate it here but nowhere in the code do you release it.

> NSString* r = [[NSString alloc] initWithData: data encoding: encoding];
> reply = [r UTF8String];

This also leaks. Same thing - if you alloc it, it is your responsibility to deal with it.

>   NSMutableDictionary* parameters =
>    [[NSMutableDictionary alloc] initWithCapacity: gQueryParameters.size()];

Also leaks.

This is stuff that is in the tree already, I haven't made it to your patch yet. Please re-post the mac patch with this stuff taken care of.

Obj-c code really needs to get reviewed by an experienced obj-c programmer, it worries me that this made it into the tree in the first place.

Comment 67

[(not currently active) Ted Mielczarek]

Attachment: mac patch (with leak fixes)

Updated patch with leak fixes.

Comment 68

[(not currently active) Ted Mielczarek]

Attachment: mac patch (with nits fixed)

Fix some more nits josh mentioned on irc.  Renamed member variables to start with 'm', and changed from 'id' to specific types on a couple of methods.

Comment 69

[Josh Aas]

Comment on attachment 299271 [details] [diff] [review]
mac patch (with nits fixed)

patch fails to apply, can you please update it?

Comment 70

[(not currently active) Ted Mielczarek]

Attachment: mac patch updated to trunk

Unbitrotted.

Comment 71

[Josh Aas]

Comment on attachment 299843 [details] [diff] [review]
mac patch updated to trunk

+      [mWindow setFrame:windowFrame display: true animate: NO];

Try to be consistent about having a space after the ":" in obj-c arguments. Generally there is no space, so the first arg there is correct and the latter two are not.

"mPlaceHolderString != nil"

No need for "!= nil", logically the same to just say "mPlaceHolderString"

I didn't do much reviewing in terms of application architecture, but this code is a huge improvement.

Comment 72

[(not currently active) Ted Mielczarek]

Checked in with Josh's comments addressed.  woot!

Comment 73

[Marcia Knous [:marcia]]

Mac UI looks good using  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2008012904 Minefield/3.0b3pre.

Comment 74

[Roman R.]

I just had an opportunity to see the crash reporter UI and want to tell you that it has poorly constructed options and a missing one.

1. The "Quit without sending" button is unnecessary because its function is implemented by the "Tell Mozilla about this crash so they can fix it" check box.
2. There's no option to send a report without restarting Firefox, and for somebody with 50 tabs open, this option can very desirable for hopefully obvious reasons.

There should just be one button on the bottom -- OK. And the "Restart Firefox" button should be turned into a check box.

This is in 2008021804.

Comment 75

[(not currently active) Ted Mielczarek]

This has been discussed to death. It's not perfect, but this is what we're shipping with.  I realize there's no way to submit without restarting, but I'm ok with that.

Comment 76

[Roman R.]

Fine, ship with it, but can't this be marked for improvement in the next version? Or have devs gotten too tired to deal with this?

Comment 77

[Samuel Sidler (old account; do not CC)]

This bug is resolved fixed. If you have specific improvements you want to make, please file them as new bugs marked as "enhancement" and they'll get triaged in order of importance.

Comment 78

[(not currently active) Ted Mielczarek]

File a new bug if you have specific suggestions, this one is fixed.  And yeah, I'm tired.  :)

Comment 79

[John J. Barton]

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=419336