1273630 - about:tabcrashed should hide extra fields by default and let it be opened with a toggle

Status: NEW

(Firefox :: General, enhancement, P5)

Description

[:Felipe Gomes (needinfo for replies!)]

The form asking the user to provide extra information (email, comments, etc) in about:tabcrashed is tied to the same toggle (the checkbox) that lets the user decide whether to submit the report at all or not.

In order to not let that discourage users from submitting the report, the two options should be untied, and the form asking for extra info should only be shown if the user chooses to.

I made an initial mockup and will try to drive this forward.

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Attachment: Current page

Comment 2

[:Felipe Gomes (needinfo for replies!)]

Attachment: Proposal

If this UI is accepted, there are two things to solve:

- we autocomplete the user's email from previous submissions. If the form is toggled off, we have to decide whether that pre-filled information should be submitted or not. I imagine not.  The same thing goes for if the user did fill in extra comments but then toggled off the form.

- decide on the final string for the toggle link

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Shorlander, can you take a look at this bug and give your thoughts on it?

Comment 4

[:Felipe Gomes (needinfo for replies!)]

(removing needinfo so this shows up on triage)

Comment 5

[Stephen Horlander [:shorlander]]

(In reply to :Felipe Gomes (needinfo me!) from comment #2)
> Created attachment 8753513 [details]
> Proposal
> 
> If this UI is accepted, there are two things to solve:
> 
> - we autocomplete the user's email from previous submissions. If the form is
> toggled off, we have to decide whether that pre-filled information should be
> submitted or not. I imagine not.  The same thing goes for if the user did
> fill in extra comments but then toggled off the form.

I agree we shouldn't submit things if they can't see them.

> - decide on the final string for the toggle link

Will have to think about this.

Overall this approach looks good to me.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1273630 - [WIP] Hide about:tabcrashed extra details fields by default, and connect to their own toggle.

Review commit: https://reviewboard.mozilla.org/r/59040/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59040/

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8762183 [details]
Bug 1273630 - [WIP] Hide about:tabcrashed extra details fields by default, and connect to their own toggle.

Some screenshots here:

https://reviewboard.mozilla.org/r/59040/

I opted to go with a checkbox instead of a twisty, since that presented some RTL complexities that I didn't want to deal with. If we really want a twisty, we can go into it.

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8762183 [details]
Bug 1273630 - [WIP] Hide about:tabcrashed extra details fields by default, and connect to their own toggle.

Looks good to me!

Comment 9

[mheubusch]

Hi - Saw this bug referenced in one that was assigned to me, and want to make sure that I am pulling thru content recommendations to each part of the about : tabcrashed pages.  I have a few questions before I offer copy edits:


1. Would it help developers if we specify the kind of details we are seeking?  (So, instead of "Provide more details, we could ask "What were you trying to do on this tab?" or asking for any other info that is pertinent but not captured by the basic crash report?  

2. Are you asking people to sign up for one email or will you be emailing them every time there is an update on the bug? Or when it's resolved or closed?  Can you send me a sample of what this email looks like? 

3. Why is the Mozilla Crash Reporter headline and gray box below the checkbox for submitting a report?  I understand that it's not exposed unless you agree to provide more details, but it seems to separate it from the main function, which is agreeing to submit the report.  

4. When you say "twisty" do you mean the disclosure triangle?  If you can't do that, maybe it would help if you indented the second box to show the relationship between the two boxes?

thanks in advance!

Comment 10

[Mike Conley (:mconley) (:⚙️)]

ni'ing myself for comment 9

Comment 11

[Mike Conley (:mconley) (:⚙️)]

So some JS is responding to a click event and calling window.open. Bleh.

Hey smaug, here's my plan for this one:

1) Augment AutoHandlingUserInputStatePusher so that it looks at key modifiers, and tells the ESM about it via static void StartHandlingUserInput
2) Add a static EventStateManager::CurrentUserInputModifiers that returns modifier keys if there's currently a user event being handled on the stack
3) Have nsGlobalWindow::OpenJS (or maybe nsGlobalWindow::OpenInternal?) query EventStateManager::CurrentUserInputModifiers, and pass any modifiers along to nsWindowWatcher
4) Have nsWindowWatcher pass those modifiers along to ProvideWindow. If ProvideWindow determines that we're opening a new tab, pass those modifiers along to nsIBrowserDOMWindow's openURI
5) Have the browser front end determine how to deal with any modifiers that are being passed in. From there, the front-end JS can trivially decide to open a new tab in the background if the modifiers seem appropriate.

Does that sound like a reasonable way forward? Or is there a better path that I'm not seeing?

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Uh, whoops, wrong bug.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

There's actually a redesign of the about:tabcrashed page that's occurring in bug 1309316.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information

Comment 15

[Elaine Neoh (:elainen98)]

Hello, can I work on this?

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Hello Elaine! Yes, you can. Do you have a build of Firefox ready to go?

If so, the relevant files are:

browser/base/content/aboutTabCrashed.xhtml
browser/base/content/aboutTabCrashed.js
browser/themes/shared/aboutTabCrashed.css (for style CSS)
browser/base/content/aboutTabCrashed.css (for functional CSS)

Comment 17

[Elaine Neoh (:elainen98)]

Hello, I have sent you an email regarding this.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

I've been communicating with Elaine via email and Matrix. Given the timeline constraints of the course that requires her to work on bugs, this doesn't seem to be the right bug for her to work on at this time. She's focusing on some other bugs instead.

Comment 19

[Ashley]

Hello, could I work on this?

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Hello Ashley! Yes, you can. Do you have a build of Firefox ready to go?

If so, the relevant files are:

browser/base/content/aboutTabCrashed.xhtml
browser/base/content/aboutTabCrashed.js
browser/themes/shared/aboutTabCrashed.css (for style CSS)
browser/base/content/aboutTabCrashed.css (for functional CSS)

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Once I know whether or not you have a build of Firefox ready to work on, I'll guide you forward with a development plan.

Comment 22

[Ashley]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #20)

Hello Ashley! Yes, you can. Do you have a build of Firefox ready to go?

If so, the relevant files are:

browser/base/content/aboutTabCrashed.xhtml
browser/base/content/aboutTabCrashed.js
browser/themes/shared/aboutTabCrashed.css (for style CSS)
browser/base/content/aboutTabCrashed.css (for functional CSS)

Hello! I do have a build ready to go (it's about 2 weeks old). I just checked, and I can access all the files you listed above.

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Hi Ashley,

I'm on holiday tomorrow, and will be back with some concrete guidance on Tuesday. In the meantime, I suggest familiarizing yourself with those files I linked to.

You can test the about:tabcrashed page by running your build with:

mach run --enable-crash-reporter

And then browsing a tab to about:crashcontent to intentionally crash the tab.

Talk to you on Tuesday!

Comment 24

[Ashley]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #23)

Hi Ashley,

I'm on holiday tomorrow, and will be back with some concrete guidance on Tuesday. In the meantime, I suggest familiarizing yourself with those files I linked to.

You can test the about:tabcrashed page by running your build with:

mach run --enable-crash-reporter

And then browsing a tab to about:crashcontent to intentionally crash the tab.

Talk to you on Tuesday!

Perfect, thank you!

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Hi Ashley,

So I think to start here, what we want to do is add a new class or attribute that sets display: none on the <div id="reportBox"> element, and then add some text (let's use "Provide more details" for now) above it, then when clicked, runs some JS to remove that class or attribute to make the box visible, and also hide the link that the user just clicked.

Here's a handy event handler in aboutTabCrashed.js that you can add to: https://searchfox.org/mozilla-central/rev/d9f92154813fbd4a528453c33886dc3a74f27abb/browser/base/content/aboutTabCrashed.js#109

Set an ID on your link element, and then add that ID to the switch statement, and then add your logic to show the report box there.

That's where I'd start, anyhow. Let me know if you want more detailed guidance on where to implement that.

Comment 26

[Ashley]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #25)

Hi Ashley,

So I think to start here, what we want to do is add a new class or attribute that sets display: none on the <div id="reportBox"> element, and then add some text (let's use "Provide more details" for now) above it, then when clicked, runs some JS to remove that class or attribute to make the box visible, and also hide the link that the user just clicked.

Here's a handy event handler in aboutTabCrashed.js that you can add to: https://searchfox.org/mozilla-central/rev/d9f92154813fbd4a528453c33886dc3a74f27abb/browser/base/content/aboutTabCrashed.js#109

Set an ID on your link element, and then add that ID to the switch statement, and then add your logic to show the report box there.

That's where I'd start, anyhow. Let me know if you want more detailed guidance on where to implement that.

Hello! Thank you for the help. I think I know how to proceed code-wise, but I'm having trouble seeing my changes. When I make a change, save, and refresh about:crashcontent, the tab goes blank. Then, when I switch to about:TabCrashed, I see what I used to see in about:crashcontent. I can't see my changes anywhere. I tried closing the tab and running "mach run --enable-crash-reporter" again, but that didn't work either.

I'm looking at the following help doc, but it looks like it doesn't say anything about how to see local changes. Am I missing some mach command?
https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html#firefox-contributors-quick-reference

Comment 27

[Mike Conley (:mconley) (:⚙️)]

Hi Ashley,
(In reply to Ashley from comment #26)

Hello! Thank you for the help. I think I know how to proceed code-wise, but I'm having trouble seeing my changes. When I make a change, save, and refresh about:crashcontent, the tab goes blank.

You're right - there are a few extra steps that you need here to see your change. First, shut down your test instance of Firefox. Then, you need to run ./mach build faster in the terminal in order for your changes to be built into the build that you're running (this is true on Windows, at least. On Linux and macOS, sometimes you can skip this step because fancy symlinks are used - but those aren't easily available on Windows).

After that, re-run ./mach run --enable-crash-reporter, and browse to about:crashcontent again to see your changes manifested.

There's another technique you might try to shorten the feedback loop, but I haven't used it much. If you come from a web development background, it might seem familiar. In a separate terminal, try running ./mach watch. What this will do, is monitor your file system for changes, and do the ./mach build faster step for you. I think this also does some magic to tell the running Firefox instance to reload its assets. So, as a potentially faster alternative to the above, try:

1. In Terminal 1, browse to your source code folder, and run ./mach watch
2. In Terminal 2, run ./mach run --enable-crash-reporter
3. Make changes to aboutTabCrashed.js/.css/.xhtml
4. Browse the tab to about:home to load up a new crashable content process
5. Send the tab to about:crashcontent, and hopefully your changes manifest.

If the above 5 steps don't work, I think you'll have to do the shutdown-buildfaster-run loop manually.

Comment 28

[Ashley]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #27)

Hi Ashley,
(In reply to Ashley from comment #26)

Hello! Thank you for the help. I think I know how to proceed code-wise, but I'm having trouble seeing my changes. When I make a change, save, and refresh about:crashcontent, the tab goes blank.

You're right - there are a few extra steps that you need here to see your change. First, shut down your test instance of Firefox. Then, you need to run ./mach build faster in the terminal in order for your changes to be built into the build that you're running (this is true on Windows, at least. On Linux and macOS, sometimes you can skip this step because fancy symlinks are used - but those aren't easily available on Windows).

After that, re-run ./mach run --enable-crash-reporter, and browse to about:crashcontent again to see your changes manifested.

There's another technique you might try to shorten the feedback loop, but I haven't used it much. If you come from a web development background, it might seem familiar. In a separate terminal, try running ./mach watch. What this will do, is monitor your file system for changes, and do the ./mach build faster step for you. I think this also does some magic to tell the running Firefox instance to reload its assets. So, as a potentially faster alternative to the above, try:

1. In Terminal 1, browse to your source code folder, and run ./mach watch
2. In Terminal 2, run ./mach run --enable-crash-reporter
3. Make changes to aboutTabCrashed.js/.css/.xhtml
4. Browse the tab to about:home to load up a new crashable content process
5. Send the tab to about:crashcontent, and hopefully your changes manifest.

If the above 5 steps don't work, I think you'll have to do the shutdown-buildfaster-run loop manually.

Perfect! Thank you. This should keep me occupied for a few days.

Comment 29

[Ashley]

Hello Mike. I added an input (type=button) element to trigger the display of the reportBox and now I have a few questions.

1. How should I be formatting the "Provide more details" text? It looks like the other text is using a "data-l10n-id=<text to add>" identifier to format text. How does that work?
2. Is this intended to be a click-once-to-open situation or a toggle?
3. Is the intention to hide the entire report box or just everything below the "send report" checkbox? I thought the original request was to hide the extra details.

Also, I tried a <button> element instead of <input type=button> original and it was not triggering the onClick event. Do you know why that would be?

Thank you for your help!

Comment 30

[Mike Conley (:mconley) (:⚙️)]

Hi Ashley,

I apologize, I'm on vacation this week (that's twice now!), and will be able to reply when I return on Monday. If you want feedback sooner than that, you might try needinfo'ing [e-mail], or joining our Matrix instance and asking in the Firefox Desktop Development channel.

Comment 31

[Ashley]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #30)

Hi Ashley,

I apologize, I'm on vacation this week (that's twice now!), and will be able to reply when I return on Monday. If you want feedback sooner than that, you might try needinfo'ing [e-mail], or joining our Matrix instance and asking in the Firefox Desktop Development channel.

Hi Mike - I actually had a very busy week and haven't had a chance to ask another channel. I hope you've enjoyed your vacation!

Comment 32

[Mike Conley (:mconley) (:⚙️)]

Hi Ashley! I'm back.

How should I be formatting the "Provide more details" text? It looks like the other text is using a "data-l10n-id=<text to add>" identifier to format text. How does that work?

The way we do that is by adding the string to something called a "Fluent" file that maps "l10n-id" keys to values. The file you want to add a string to is browser/locales/en-US/browser/aboutTabCrashed.ftl. The format for adding the string is:

<l10n-id> = <string> - you can check out the other strings in that file for examples. If you want to know more about Fluent, here are some docs: https://firefox-source-docs.mozilla.org/l10n/fluent/index.html

Is this intended to be a click-once-to-open situation or a toggle?

For simplicity's sake, let's make this a "click-once-to-open" behaviour for now. That way, we don't have to worry about the case where the user has filled out some details and then re-collapses the details section (ie: should we still submit them? Should we clear them?)

Is the intention to hide the entire report box or just everything below the "send report" checkbox? I thought the original request was to hide the extra details.

The design of about:tabcrashed has changed since the original idea was put forward, but I think the idea still has legs. The intent is to make the page less intimidating so that users feel safer submitting crash reports. What I'd like us to try to do is hide the "Optional comments" textarea and the "Include URLs" checkbox by default. Let's not collapse the "Report background tabs" section, and keep it visible for now, and see how that feels.

Also, I tried a <button> element instead of <input type=button> original and it was not triggering the onClick event. Do you know why that would be?

You might need to give the button an ID, and then add that ID to this list: https://searchfox.org/mozilla-central/rev/62f6cc5d9c829bc0c6f18e25f93203a98681ac97/browser/base/content/aboutTabCrashed.js#22

Comment 33

[Ashley]

Hi Mike - thank you again! Two more questions, but I think it's mostly done!

1. I'm not sure how to trigger the #requestEmail or the #requestAutoSubmit items. I hid the entire #options list, so these will be hidden by the button as well. Is that okay?
2. Only <input type="button"> works without the data-l10n-id, but only <button> works with the data-l10n-id. Do you know why that is? In both cases they did have an id in the list you specified. This is more a curiosity at this point.
   <input type="button" class="button-as-text" id="moreDetails" value="► Provide more details..."/>
   <button class="button-as-text" id="moreDetails" data-l10n-id="more-details"></button>

I will try to add attachments of what I've done so far. I think the button is a little ugly, but otherwise seems like what is intended. What should I do next?

Comment 34

[Ashley]

Attachment: Crash tab "Provide more details" gif of button working

Sorry for the double comment - but I didn't know how attachments work! Here's a gif of the button. I have pictures too if you would like those.

Comment 35

[Ashley]

Attachment: Crashed tab "Provide more details" png after click

Comment 36

[Mike Conley (:mconley) (:⚙️)]

Hi Ashley, sorry again for the delay. Great screenshots!

I'm not sure how to trigger the #requestEmail or the #requestAutoSubmit items. I hid the entire #options list, so these will be hidden by the button as well. Is that okay?

Requesting of the email is preffed off by default - you can turn it on by submitting browser.tabs.crashReporting.requestEmail to true in about:config. Similarly, the #requestAutoSubmit item is hidden unless browser.crashReports.unsubmittedCheck.autoSubmit2 is set to false.

Only <input type="button"> works without the data-l10n-id, but only <button> works with the data-l10n-id. Do you know why that is? In both cases they did have an id in the list you specified. This is more a curiosity at this point.
<input type="button" class="button-as-text" id="moreDetails" value="► Provide more details..."/>
<button class="button-as-text" id="moreDetails" data-l10n-id="more-details"></button>

I think it depends on what the value of the string is that got put into the .ftl file. The way Fluent works, is that if the string needs to be part of an attribute (like the "value" attribute of the <input type="button"/>), then the string should be like this:

more-details =
  .value = ► Provide more details...

Where "value" is the attribute that's being written to. Whereas for a <button>, the text content of the node is what ends up being the button label, so:

more-details = ► Provide more details...

will insert ► Provide more details... into the <button>, which is what will get displayed.

I think the next step here is to take a look at the patch! The best way to submit a patch is via a command-line tool called moz-phab. Usage instructions are here: https://pypi.org/project/MozPhab/

Let me know if you have any difficulty with moz-phab, and I'll take a look. I'm eager to see the patch!

Thanks!

Comment 37

[Ashley]

Hello - just an update, I am still working on how to commit with mercurial and submit with moz-phab. I'm reading through lots of documentation and asking questions in the matrix chat. Hopefully I will have this done this week.

Comment 38

[Kanav]

Hello, I'm Kanav.
I'm a web developer, and I found this issue marked open, and 'good first bug', but a series of changes have occurred here, and I'd like to ask if this issue is still open to to be assigned to another developer. Thank you.

Comment 39

[Ashley]

(In reply to Kanav from comment #38)

Hello, I'm Kanav.
I'm a web developer, and I found this issue marked open, and 'good first bug', but a series of changes have occurred here, and I'd like to ask if this issue is still open to to be assigned to another developer. Thank you.

Hello Kanav - I’m in the middle of submitting a patch for this. I’m planning on submitting this week. I’ll let you know if anything changes!

Comment 40

[Ashley]

Hello Mike,

I am unfortunately having a lot of trouble submitting my patch. The following is what I've learned researching through the documentation. Please let me know if any are incorrect.

1. Mercurial tracks changes through branches and bookmarks - bookmarks are more similar to git branches
2. When I used the "hg clone" command (suggested in the quick start guide), it created a new 'default' branch for me to make my changes
3. Now that the changes are done, I need to "hg add" and "hg commit" my changes before I can submit the patch using moz phab

The problem is, when I used "hg status" and attempted to "hg add" all files that were listed as Modified, I got a "aboutTabCrashed.xhtml already tracked!" message.

Then I tried to commit the changes (that were seemingly being tracked) and I got the following:
MYCOMPUTER /c/mozilla-source/mozilla-central/browser/base/content
$hg commit -m "Bug 1273630 - Added button to hide extra fields on crashed tab."
sh: mach: No such file or directory

(Even though I have modified files outside of this directory, I was in there just to lower the "hg status" output time and see if I could isolate the problem.)

Thank you again for all the help you have provided!

Comment 41

[Mike Conley (:mconley) (:⚙️)]

Hi Ashley,

Now that the changes are done, I need to "hg add" and "hg commit" my changes before I can submit the patch using moz phab

Ah, this is a major difference between Git and Mercurial: Git has a notion of a "staging area" where changed files have to be "added" each time before they can be committed. Mercurial does not have a staging area. You only need to hg add once, when you're adding the file to be tracked for the first time. After that, hg commit is like git commit -a, where all changes listed by hg diff will go into the next commit.

Are you running hg commit from the root mozilla-central folder? I highly recommend running all hg and mach commands from there.

Comment 42

[Mike Conley (:mconley) (:⚙️)]

(I've reached out over Matrix to try to figure this out with Ashley)

Comment 43

[Ashley]

Attachment: patch.diff

Comment 44

[Mike Conley (:mconley) (:⚙️)]

I'm helping Ashley repair a broken local repository. In the meantime, I'll review the patch using Splinter here in Bugzilla.

Comment 45

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 9176777 [details] [diff] [review]
patch.diff

Review of attachment 9176777 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, Ashley! This is a great start. I have some suggestions on how to proceed with this. See below. Let me know if you have any questions about my comments.

::: browser/base/content/aboutTabCrashed.js
@@ +108,5 @@
>  
>    onClick(event) {
>      switch (event.target.id) {
> +      case "moreDetails": {
> +        document.getElementById("options").style.display = "block";

What I recommend instead of manipulating the style on the element, is to use the ".hidden" property that exists on documents like this.

So you should be able to do:

```javascript
document.getElementById("options").hidden = false;
```

In retrospect though, I wonder if instead of having the `moreDetails` be a `<button>`, we make it a `<checkbox>`. That way, it can be a toggle, and we can show or hide the `options` div based on the state of the checkbox.

We can keep the _style_ that we've got here by actually hiding the checkbox in the CSS, but we can use the semantics of a checkbox here.

So in that case, I'd suggest having it be:

```javascript
let showMoreDetails = document.getElementById("moreDetails").checked;
document.getElementById("options").hidden = !showMoreDetails;
break;
```

@@ +109,5 @@
>    onClick(event) {
>      switch (event.target.id) {
> +      case "moreDetails": {
> +        document.getElementById("options").style.display = "block";
> +        document.getElementById("moreDetails").value="▼ Provide more details";

All of our strings have to be in the `.ftl` file, so for this, what we're going to have to do is have Fluent (our localization framework) update the string dynamically using a command. Let me find you an example:

https://searchfox.org/mozilla-central/rev/b58ca45005fe02077c92779483d1b60e9a49687c/browser/base/content/webrtcIndicator.js#402

In this example, this statement `document.l10n.setAttributes(toggleEl, l10nId);` is updating the `data-l10n-id` attribute on an element `toggleEl` to be the value of `l10nId`. You can see the two strings we're flipping between here:

https://searchfox.org/mozilla-central/rev/b58ca45005fe02077c92779483d1b60e9a49687c/browser/locales/en-US/browser/webrtcIndicator.ftl#18-21

(you won't need the `.title` for your patch - but you'll want to use `.label` instead when you switch the <button> to a <checkbox> per my other comment)

@@ +113,5 @@
> +        document.getElementById("moreDetails").value="▼ Provide more details";
> +        break;
> +      }
> +	  
> +	  case "closeTab": {

Small nit here - looks like some whitespace got introduced at the start of this line.

::: browser/base/content/aboutTabCrashed.xhtml
@@ +45,5 @@
> +          <label for="sendReport" data-l10n-id="crashed-send-report"></label>
> +        </div>
> +
> +        <div>
> +          <button class="button-as-text" id="moreDetails" data-l10n-id="more-details"></button>

Let's make this a checkbox instead, so we can use the built-in checked/unchecked state.

::: browser/locales/en-US/browser/aboutTabCrashed.ftl
@@ +21,5 @@
>  crashed-email-me = Email me when more information is available
>  crashed-report-sent = Crash report already submitted; thank you for helping make { -brand-short-name } better!
>  crashed-request-auto-submit-title = Report background tabs
>  crashed-auto-submit-checkbox = Update preferences to automatically submit reports when { -brand-short-name } crashes.
> +more-details = Provide more details...

When we do ellipses like this, we like to use the ellipses character - you can copy and paste it from here: https://searchfox.org/mozilla-central/rev/b58ca45005fe02077c92779483d1b60e9a49687c/third_party/rust/fluent-syntax/benches/contexts/browser/browserContext.ftl#388

::: browser/themes/shared/aboutTabCrashed.css
@@ +48,5 @@
>  #email {
>    margin-top: 14px;
>  }
>  
> +.button-as-text {

Instead of this rule being for a button, per my suggestion, this will now be for a checkbox. We're unlikely to re-use this rule, so I think we can have the selector be `#moreOptions` for now.

Then, to hide the actual checkbox part of the checkbox, we can use something kind of like this: https://searchfox.org/mozilla-central/rev/b58ca45005fe02077c92779483d1b60e9a49687c/browser/themes/shared/webRTC-indicator.inc.css#71-81

You don't want _all_ of those rules. Just the `moz-appearance` one, `border-style` and `margin`, probably.

Comment 46

[Ashley]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #45)

Comment on attachment 9176777 [details] [diff] [review]
patch.diff

Review of attachment 9176777 [details] [diff] [review]:

Thanks, Ashley! This is a great start. I have some suggestions on how to
proceed with this. See below. Let me know if you have any questions about my
comments.

::: browser/base/content/aboutTabCrashed.js
@@ +108,5 @@

onClick(event) {
switch (event.target.id) {

•  case "moreDetails": {
  

•    document.getElementById("options").style.display = "block";
  

What I recommend instead of manipulating the style on the element, is to use
the ".hidden" property that exists on documents like this.

So you should be able to do:

document.getElementById("options").hidden = false;

In retrospect though, I wonder if instead of having the moreDetails be a
<button>, we make it a <checkbox>. That way, it can be a toggle, and we
can show or hide the options div based on the state of the checkbox.

We can keep the style that we've got here by actually hiding the checkbox
in the CSS, but we can use the semantics of a checkbox here.

So in that case, I'd suggest having it be:

let showMoreDetails = document.getElementById("moreDetails").checked;
document.getElementById("options").hidden = !showMoreDetails;
break;

I got this working, but using hidden has revealed that showCrashReportUI already does this - are these two checkboxes supposed to do/hide different things?

@@ +109,5 @@

onClick(event) {
switch (event.target.id) {

•  case "moreDetails": {
  

•    document.getElementById("options").style.display = "block";
  

•    document.getElementById("moreDetails").value="▼ Provide more details";
  

All of our strings have to be in the .ftl file, so for this, what we're
going to have to do is have Fluent (our localization framework) update the
string dynamically using a command. Let me find you an example:

https://searchfox.org/mozilla-central/rev/
b58ca45005fe02077c92779483d1b60e9a49687c/browser/base/content/
webrtcIndicator.js#402

In this example, this statement document.l10n.setAttributes(toggleEl, l10nId); is updating the data-l10n-id attribute on an element toggleEl
to be the value of l10nId. You can see the two strings we're flipping
between here:

https://searchfox.org/mozilla-central/rev/
b58ca45005fe02077c92779483d1b60e9a49687c/browser/locales/en-US/browser/
webrtcIndicator.ftl#18-21

(you won't need the .title for your patch - but you'll want to use
.label instead when you switch the <button> to a <checkbox> per my other
comment)

I'm having trouble getting the label to dynamically change. When I click my checkbox, I look at the console and see
console.warn: "[fluent] Missing translations in en-US: more-details." JavaScript error: , line 0: uncaught exception: undefined

Here is the code I've written

case  "moreDetails": {
   let  showMoreDetails = document.getElementById("moreDetails").checked;
   document.getElementById("options").hidden = !showMoreDetails;

   let  l10nId = "";
   if (showMoreDetails) {
      l10nId = "more-details";
   } else {
      l10nId = "fewer-details";
   }

   document.l10n.setAttributes(document.getElementById("moreDetails"), l10nId);
   break;
}

@@ +113,5 @@

•    document.getElementById("moreDetails").value="▼ Provide more details";
  

•    break;
  

•  }
  

• case "closeTab": {
  

Small nit here - looks like some whitespace got introduced at the start of
this line.

I don't know how to get rid of this. The ftl file has a space before the string like every other variable in that file.

::: browser/base/content/aboutTabCrashed.xhtml
@@ +45,5 @@

•      <label for="sendReport" data-l10n-id="crashed-send-report"></label>
  

•    </div>
  

•    <div>
  

•      <button class="button-as-text" id="moreDetails" data-l10n-id="more-details"></button>
  

Let's make this a checkbox instead, so we can use the built-in
checked/unchecked state.

done

::: browser/locales/en-US/browser/aboutTabCrashed.ftl
@@ +21,5 @@

crashed-email-me = Email me when more information is available
crashed-report-sent = Crash report already submitted; thank you for helping make { -brand-short-name } better!
crashed-request-auto-submit-title = Report background tabs
crashed-auto-submit-checkbox = Update preferences to automatically submit reports when { -brand-short-name } crashes.
+more-details = Provide more details...

When we do ellipses like this, we like to use the ellipses character - you
can copy and paste it from here:
https://searchfox.org/mozilla-central/rev/
b58ca45005fe02077c92779483d1b60e9a49687c/third_party/rust/fluent-syntax/
benches/contexts/browser/browserContext.ftl#388

done

::: browser/themes/shared/aboutTabCrashed.css
@@ +48,5 @@

#email {
margin-top: 14px;
}

+.button-as-text {

Instead of this rule being for a button, per my suggestion, this will now be
for a checkbox. We're unlikely to re-use this rule, so I think we can have
the selector be #moreOptions for now.

Then, to hide the actual checkbox part of the checkbox, we can use something
kind of like this:
https://searchfox.org/mozilla-central/rev/
b58ca45005fe02077c92779483d1b60e9a49687c/browser/themes/shared/webRTC-
indicator.inc.css#71-81

You don't want all of those rules. Just the moz-appearance one,
border-style and margin, probably.

Why do we want to hide the checkbox part? So it still functions like a button? Like here? https://css-tricks.com/the-checkbox-hack/
I updated my checkbox css with the below code but the checkbox still displayed.

#moreDetails {
   -webkit-appearance: none;
   -moz-appearance: none;
   appearance: none;
   border-style: none;
   margin: 0;
}

Comment 47

[BugBot [:suhaib / :marco]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 48

[Karthik Sundar]

Is this bug still open.. or is it done??

Comment 49

[Sam. M]

Attachment: tabcrashed.html

Fixed it.

Comment 50

[Karthik Sundar]

Therefore, this bug shouldn't tagged as open right??

Comment 51

[Manuel Carretero]

Is this bug solved? Or why is it still flagged as unassigned if this thread suggests it is? In case it isn't done I would like to work on it

Comment 52

[Simple]

Hello,
is this bug solved? if yes, why is it still open?
If no, can we proceed to work on it.

Comment 53

[akpjunio4real]

Hello,
My Name is Abraham and I'm an outreachy applicant. I noticed this issue is unassigned. I want to confirm if I can work on this issue?

Comment 54

[Falguni Islam]

Attachment: Bug 1273630 : hiding about:tabcrashed should hide extra fields by default and let it be opened with a toggle

Comment 55

[Mike Conley (:mconley) (:⚙️)]

Hi Falguni Islam! I've left a review on the patch. Let me know if you have any questions on how to proceed. Thanks!

Comment 56

[BugBot [:suhaib / :marco]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 57

[omamazainab00]

Can you assign this to me? I want to work on it.

Comment 58

[Mike Conley (:mconley) (:⚙️)]

Hi omamazainab00, sure - but before I do that, a few things:

1. Do you have a local build set up?
2. There's a pre-existing patch in this patch to start off with. Do you know how to apply that to work off of?

Comment 59

[uwaobayagbona]

Hi Mike, I have a local build set up and I can figure out how to base my work off the existing patch.
Is the bug still available?

Comment 60

[Mike Conley (:mconley) (:⚙️)]

Hi uwaobayagbona,

Sure, sounds like you're all set to work on this. I'll assign the bug to you. Let me know if you have any questions about how to proceed - though I highly recommend examining the conversation history in the original patch to get a sense of where it was heading and where it should go.

Comment 61

[uwaobayagbona]

Hi Mike,

Firefox keeps crashing when I run it from this patch D128102

Comment 62

[Mike Conley (:mconley) (:⚙️)]

Hi uwaobayagbona,

Sorry for the slow reply! Can you tell me more about this crash? Does all of Firefox shut down, or does the tab just crash? Do you see any error messages sent to the shell / console that might help indicate what's going wrong?

I'm mconley on Matrix if you want to reach out and chat with me directly.

Comment 63

[uwaobayagbona]

Hi Mike, I have reached out to you on Matrix with more info.

Comment 64

[BugBot [:suhaib / :marco]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 65

[Favour]

Hello, I'm favour.
I'm a outreachy apllicant, and I found this issue marked open, and 'good first bug', but a series of changes have occurred here, and I'd like to ask if this issue is still open to to be assigned to another developer., if yess please direct me
Thank you.

Comment 66

[Mike Conley (:mconley) (:⚙️)]

Hi all,

I think I'm actually going to pull this from the pool of good-first-bugs. I believe there's an ongoing effort to redesign the about:tabcrashed page, and this bug is blocked on that redesign.