Change "Style sheet could not be loaded” Red Error Banner to a Neutral Warning Banner

RESOLVED FIXED in Firefox 60

Status

P3
normal
RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: victoria, Assigned: jesse.cordeiro, Mentored)

Tracking

(Blocks: 1 bug, {good-first-bug})

58 Branch
Firefox 60
good-first-bug

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

a year ago
The bright red stylesheet error styling is too strong for this situation, so let's change it to match the neutral-looking “Debugger is paused” warning bar with yellow warning triangle. 

(Future task that requires more design work: This error should be displayed inside Style Editor window somewhere.)
(Reporter)

Updated

a year ago
Blocks: 1389730
Priority: -- → P3
(Reporter)

Comment 1

a year ago
STR: 
1. In Nightly, create a New Tab. 
2. Open Devtools while on this tab. 
3. Type "Test" in the search bar and press enter.
The google search results page should trigger a "stylesheet not loaded" error
In time, we should really get rid of this error, it's the only tool in the toolbox that does this very in-your-face sort of error. It's often confusing because the stylesheet href isn't always displayed. And the message says the stylesheet couldn't be loaded, while really the browser did load it, it's just the style editor that failed at displaying it in the list.

But, in the meantime, given we're not focusing on this tool much, I agree with Victoria, we should make it consistent with the debugger.

I believe this would make a good bug for someone who wants to start hacking on DevTool's front-end. It shouldn't be too involved.

These are the occurrences in the code where the message is being triggered: https://searchfox.org/mozilla-central/search?q=LOAD_ERROR&path=devtools
And I believe this is where it actually gets displayed: https://searchfox.org/mozilla-central/rev/cf149b7b63ff97023e28723167725e38cf5df757/devtools/client/styleeditor/styleeditor-panel.js#76-98
It looks like this uses the NotificationBox, which is a component that renders its messages from here:
https://searchfox.org/mozilla-central/rev/cf149b7b63ff97023e28723167725e38cf5df757/devtools/client/shared/components/NotificationBox.js#229-256
And, finally, it looks like the CSS for the notification is in this file: https://searchfox.org/mozilla-central/source/devtools/client/shared/components/NotificationBox.css

Several hints to work on this:
- using the Browser Toolbox will be very helpful (with it you can use devtools on devtools, and therefore inspect the notification box): https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox
- there is documentation about contributing to devtools here: http://docs.firefox-dev.tools/

Feel free to ask questions on this bug if you need more information. Otherwise, feel free to claim this bug and attach code changes here.
Mentor: pbrosset
Keywords: good-first-bug

Comment 3

a year ago
I think it's related to bug 1401948
Feel free to remove if I got it wrong :)
See Also: → bug 1401948
(Reporter)

Comment 4

a year ago
Created attachment 8941226 [details]
Red error banner for stylesheets not loaded
(Assignee)

Comment 5

a year ago
I'm interested in picking up this bug but could @Victoria or @Ahmed confirm if this can be resolved with bug 1401948. If so, I noticed there hasn't been any progress with bug 1401948 for 4 months and I would be willing to tackle that. Thanks!
Flags: needinfo?(victoria)
Flags: needinfo?(3ugzilla)
(Reporter)

Comment 6

a year ago
Thanks Jesse - No progress on 1401948 planned for now, so it would be great to move this bug forward!
Flags: needinfo?(victoria)
(Assignee)

Comment 7

a year ago
(In reply to Victoria Wang [:victoria] from comment #6)
> Thanks Jesse - No progress on 1401948 planned for now, so it would be great
> to move this bug forward!

Thanks for the prompt response - I'll get started with this bug then!

Comment 8

a year ago
Hope you got what you need. :)
Clearing the flag
Flags: needinfo?(3ugzilla)
(Assignee)

Comment 9

a year ago
Created attachment 8955982 [details] [diff] [review]
Changed banner appearance of "Style sheet could not be loaded” warning
(Assignee)

Comment 10

a year ago
Please review the patch that I've attached, thanks! :)
Comment on attachment 8955982 [details] [diff] [review]
Changed banner appearance of "Style sheet could not be loaded” warning

Thanks for the patch. Marking it as R? so I don't forget to review the code.
Attachment #8955982 - Flags: review?(pbrosset)
Also assigning the bug to you now.
Assignee: nobody → jesse.cordeiro
Status: NEW → ASSIGNED
Created attachment 8955990 [details]
warning.png

Hey Victoria, here's what it looks like with the change proposed in this bug.
It's now a warning message just like when the debugger is paused.
Does this look good?
Flags: needinfo?(victoria)
Comment on attachment 8955982 [details] [diff] [review]
Changed banner appearance of "Style sheet could not be loaded” warning

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

Thanks for this! Let's wait for Victoria to answer, but in the meantime, I have a couple minor comments about the code. Can you please address them and attach a new patch?

::: devtools/client/styleeditor/StyleSheetEditor.jsm
@@ +298,4 @@
>          console.error(e);
>        } else {
>          console.error(e);
> +        this.emit("error", { key: LOAD_ERROR, append: this.styleSheet.href, level: "warning" });

Note that this line is longer than 90 characters, so will cause eslint warnings that will prevent the patch from landing.
See http://docs.firefox-dev.tools/contributing/eslint.html for some docs about this.
You will need to wrap the line like so:

        this.emit("error", { key: LOAD_ERROR, append: this.styleSheet.href,
          level: "warning" });

::: devtools/client/styleeditor/styleeditor-panel.js
@@ +95,5 @@
> +        return notificationBox.PRIORITY_WARNING_LOW;
> +      } else {
> +        return notificationBox.PRIORITY_CRITICAL_LOW;
> +      }
> +    })(data.level)

This works fine, but I'm not sure the added complexity of defining and executing a function is really needed here. Why not a simple if/else:

    let level = notificationBox.PRIORITY_CRITICAL_LOW;
    if (data.level === "info") {
      level = notificationBox.PRIORITY_INFO_LOW;
    } else if (data.level === "warning") {
      level = notificationBox.PRIORITY_WARNING_LOW;
    }
Attachment #8955982 - Flags: review?(pbrosset)
(Assignee)

Comment 15

a year ago
Created attachment 8956130 [details] [diff] [review]
Minor tweaks for linter and code complexity

I haven't had much experience with Mercurial - is there anyway to merge the two patch files into one?
(In reply to jesse.cordeiro from comment #15)
> Created attachment 8956130 [details] [diff] [review]
> Minor tweaks for linter and code complexity
> 
> I haven't had much experience with Mercurial - is there anyway to merge the
> two patch files into one?

Yes, check here for more details on this:

http://docs.firefox-dev.tools/contributing/making-prs.html

There are tips for both Git and Mercurial.
(Reporter)

Comment 17

a year ago
(In reply to Patrick Brosset <:pbro> from comment #13)
> Created attachment 8955990 [details]
> warning.png
> 
> Hey Victoria, here's what it looks like with the change proposed in this bug.
> It's now a warning message just like when the debugger is paused.
> Does this look good?

Looks good! Excited to see this land.
Flags: needinfo?(victoria)
(Assignee)

Comment 18

a year ago
Created attachment 8956185 [details] [diff] [review]
Merged patch files

Attached the merged patch, thanks for the tip @jryans!
Comment on attachment 8956185 [details] [diff] [review]
Merged patch files

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

Looks good thank you!
I'll push this to the CI environment to make sure this is safe, and then we'll land it!
Attachment #8956185 - Flags: review+
Attachment #8955982 - Attachment is obsolete: true
Attachment #8956130 - Attachment is obsolete: true
Also, here are a few tips that should help you if you decide to contribute more on bugzilla:
- when you attach a new patch here, you should mark the previous one as "obsolete", so it's easy to know what is your latest version
- also, when you want someone to review your code changes, you can set the review flag to ? (on the patch upload page), and enter the name of the reviewer in the field next to it
- finally, it looks like you only attached a simple diff file, which did not contain a proper commit. It didn't have a commit message nor your author information. You can find more info on this here https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Don't worry about all this here, I'll make a commit out of the diff you attached here.
Comment hidden (mozreview-request)
Attachment #8956185 - Attachment is obsolete: true

Comment 22

a year ago
mozreview-review
Comment on attachment 8956358 [details]
Bug 1421387 - Change the error banner for styleditor load errors to warnings;

https://reviewboard.mozilla.org/r/225228/#review231196
Attachment #8956358 - Flags: review?(pbrosset) → review+

Comment 24

a year ago
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/08afac3c7b3c
Change the error banner for styleditor load errors to warnings; r=pbro

Comment 25

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/08afac3c7b3c
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
status-firefox60: --- → fixed
tracking-seamonkey2.15: --- → ---

Updated

8 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.