use buttons for view and submit actions on about:crashes

NEW
Unassigned

Status

()

--
enhancement
3 years ago
a year ago

People

(Reporter: blassey, Unassigned)

Tracking

({feature})

unspecified
feature
Points:
---

Firefox Tracking Flags

(firefox57 wontfix)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Created attachment 8760971 [details] [diff] [review]
use_buttons.patch
Attachment #8760971 - Flags: ui-review?(shorlander)
Attachment #8760971 - Flags: review?(mconley)
Comment on attachment 8760971 [details] [diff] [review]
use_buttons.patch

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

::: toolkit/crashreporter/content/crashes.js
@@ +38,5 @@
> +      let bundle = Services.strings.createBundle("chrome://global/locale/crashes.properties");
> +      button.firstChild.textContent = bundle.GetStringFromName("view.label");
> +      button.addEventListener("click", navigateToReport, true);
> +      button.disabled = false;
> +      

fixed this ws

::: toolkit/locales/en-US/crashreporter/crashes.dtd
@@ -7,5 @@
>  <!ENTITY date.heading               "Date Submitted">
>  <!ENTITY noReports.label            "No crash reports have been submitted.">
>  <!ENTITY noConfig.label             "This application has not been configured to display crash reports. The preference <code>breakpad.reportURL</code> must be set.">
>  <!ENTITY clearAllReports.label      "Remove All Reports">
> -

and reverted this, both locally
Comment on attachment 8760971 [details] [diff] [review]
use_buttons.patch

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

Looks good to me. 

We might also want to consider adding a "Submit All Reports" similar to the "Remove All Reports".

Also the "Date Submitted" header is weird because not all of them have been submitted. Maybe just "Date" ?
Attachment #8760971 - Flags: ui-review?(shorlander) → ui-review+
Comment on attachment 8760971 [details] [diff] [review]
use_buttons.patch

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

::: toolkit/crashreporter/content/crashes.js
@@ +37,3 @@
>  
> +      let bundle = Services.strings.createBundle("chrome://global/locale/crashes.properties");
> +      button.firstChild.textContent = bundle.GetStringFromName("view.label");

I'm not crazy about duplicating this button creation here.

What I think I'd prefer is that for each report that isn't submitted, we create both the View and Submit buttons, but we hide the View button. On successful submission, we then hide the Submit button and show the View button. In fact, you can mostly do most of the logic that way with some CSS.

CSS like:

tr[submitted] > td > .submit,
tr:not([submitted]) > td > .view {
  display:none;
}

Then give the Submit button a "submit" class attribute, and the View button a "view" class attribute. When adding rows, set the submitted attribute on the row to show the right state.

Then, when a submission goes through, reach up to the <tr>, and set the submitted attribute to do the hiding of the Submit and showing of the View button.

@@ +37,4 @@
>  
> +      let bundle = Services.strings.createBundle("chrome://global/locale/crashes.properties");
> +      button.firstChild.textContent = bundle.GetStringFromName("view.label");
> +      button.addEventListener("click", navigateToReport, true);

Shouldn't be capturing

@@ +119,5 @@
>      row.appendChild(cell);
> +    let button = document.createElement("button");
> +    if (reports[i].pending) {
> +      button.appendChild(document.createTextNode(bundle.GetStringFromName("submit.label")));
> +      button.addEventListener("click", submitPendingReport, true);

This should probably not be capturing (last argument).

Also, ideally, there'd just be a single click event handler on the whole table, and the logic for knowing what to do would be computed by examining the event target. That kind of work is probably out of scope for this bug, but I thought I'd mention it for posterity.

@@ +120,5 @@
> +    let button = document.createElement("button");
> +    if (reports[i].pending) {
> +      button.appendChild(document.createTextNode(bundle.GetStringFromName("submit.label")));
> +      button.addEventListener("click", submitPendingReport, true);
> +      button.setAttribute("href", reportURL + reports[i].id);

Before, we would send the user to some kind of throttling page (aboutThrottling.spec)... That seems pretty intentional. Are you sure we need to change this, and why?

Also, I don't actually think we use this href attribute for anything. We _do_ end up doing some tree gymnastics to resolve to the report ID though. Maybe stash the ID into the button directly instead of the href to avoid the tree gymnastics.
Attachment #8760971 - Flags: review?(mconley) → review-
Created attachment 8762214 [details] [diff] [review]
use_buttons.patch

the throttling url is a safety net in case someone clicks the link before the report finishes submitting. Since we don't show the view button until the report finishes submitting, we don't need that protection.

In a belt and suspenders move, I've also marked the view button as disabled if we have a pending report.
Attachment #8760971 - Attachment is obsolete: true
Attachment #8762214 - Flags: review?(mconley)
Comment on attachment 8762214 [details] [diff] [review]
use_buttons.patch

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

I haven't tested this, but this looks mostly okay - there are just some bits of leftover / debugging code that need to be removed, and some very minor suggestions.

::: toolkit/crashreporter/content/crashes.js
@@ +19,5 @@
>  
> +function navigateToReport(event) {
> +  let button = event.target;
> +  let url = button.getAttribute("report_url");
> +  console.log(url);

Please remove the console.log

@@ +27,3 @@
>  function submitPendingReport(event) {
> +  let button = event.target;
> +  let span = button.parentNode.parentNode.firstChild;

There are two references to button.parentNode.parentNode in here (line 29, and line 38). Maybe a good idea to alias that.

@@ +117,5 @@
> +    let viewButton = document.createElement("button");
> +    viewButton.appendChild(document.createTextNode(bundle.GetStringFromName("view.label")));
> +    viewButton.addEventListener("click", navigateToReport);
> +    viewButton.className = "view";
> +    viewButton.setAttribute("view", true);

I don't think this attribute is used, is it?

@@ +126,5 @@
> +      submitButton.appendChild(document.createTextNode(bundle.GetStringFromName("submit.label")));
> +      submitButton.addEventListener("click", submitPendingReport);
> +      submitButton.setAttribute("report_id", reports[i].id);
> +      submitButton.className = "submit";
> +      submitButton.setAttribute("submit", true);

I also don't think this attribute is used

::: toolkit/crashreporter/content/crashes.xhtml
@@ +81,5 @@
>    }
>  }
> +
> +tr[submitted] {
> +background-color: "red";

This looks like leftovers...
Attachment #8762214 - Flags: review?(mconley) → review-
Since bug 512479 landed we should be able to do better here. We now have separated lists of submitted and pending crash reports, so the submitted ones can continue to be links to crash-stats, but we could make the pending ones not be links, but have a "Submit" button next to each one, and maybe a "Submit All" button at the top of the pending list.
Keywords: feature
status-firefox57: --- → wontfix
(sorry for the multiple updates.  Getting clarity on how we're using the flags this week.)
Severity: normal → enhancement
You need to log in before you can comment on or make changes to this bug.