957634 - Add a warning icon to the debugger resumption order panel

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Victor Porof [:vporof][:vp]]

Followup for bug 947143.

Comment 1

[Alexey Novak]

I would love to work on this bug.
Will take care of it right after https://bugzilla.mozilla.org/show_bug.cgi?id=947143

Should not take me long.

Comment 2

[Alexey Novak]

Attachment: first dummy diff

I will need some help with this bug since I have several concerns/questions in order to understand the scope of this bug correctly.

- I wonder if you have any strategy in mind for a Tooltip for showing this alert icon. Does a developer have an ability to add this alert icon for a simple text notification or other notification types as well? (e.g. setVariableContent, setImageContent, setColorPickerContent, etc) Or is it is a property of the Tooltip class which will be applied its methods? 
Personally, I think it should be used with text-type tooltips when required only. In my dummy diff I have an example of applying alert to setTextContent() function.

- I'm not sure how to display an alert image and where the images are stored. In my dummy diff file I decided to reuse the alert image markup from an old resumption order panel but it would not work the same for Tooltip(At least when I tested it, it did not show the image). Image which is used in resumption-order-panel does not have any image source attributes and only uses "alert-icon" class. Searching everywhere for this class name in the project did not give me any results... Not sure how to approach this image problem.

- Do we need to add "alt" attribute to the icon saying something "Alert icon" ? Not sure if this is a11y helpful for this widget.

- If I understand correctly I would need to write some tests to test/browser_toolbar_tooltip.js ?

Comment 3

[Victor Porof [:vporof][:vp]]

Comment on attachment 8357763 [details] [diff] [review]
first dummy diff

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

Great start!

The current strategy is fine, we don't need the warning icon for anything else right now, so just adding it to `setTextContent` is perfect. If we're ever going to need the warning icon for anything else, we'll think about it then.

Using an alt attribute is entirely optional and shouldn't matter much, because the image is on disk. I wouldn't add it.

I don't think test/browser_toolbar_tooltip.js is what you'll want to modify. Worst case scenario you'll have to create a new debugger test that causes the resumption order popup to show, and check if it's shown correctly. But I'll have to think about it.

::: browser/devtools/shared/widgets/Tooltip.js
@@ +423,5 @@
>      vbox.className = "devtools-tooltip-simple-text-container " + containerClass;
>      vbox.setAttribute("flex", "1");
>  
> +    if (isAlertTooltip) {
> +      let alertImg = this.doc.createElement("img");

Hah, another XUL caveat :) You're not in the HTML namespace here, so the element's name is actually "image". Simply using that instead of "img" should work fine.

If that doesn't work, some other form of magic might be affecting it, in which case you should just go ahead and use chrome://global/skin/icons/warning-64.png as the image source. FYI, the "alert-icon" class is defined in themes/*/global/global.css

Comment 4

[Alexey Novak]

Thanks for the feedback! It gives me enough information to poke around... I will keep you updated with my progress.

Comment 5

[Alexey Novak]

Attachment: My second dummy diff

So I finished my second diff. It is looks good or almost ready then I will attach a proper patch here.

I was playing around with the tooltip and (personally) found 64x64 icon not appropriate at all. It makes tooltip so ugly (it is my subjective opinion). I changed it to be 32x32 and it looks much more elegant in this use case. Please take a look and let me know what you think.

Another thing I was looking into this line:
this._resumeOrderTooltip.setTextContent([label], defaultStyle, defaultStyle, true);

Tried to use ES6 something like:
this._resumeOrderTooltip.setTextContent([label], ...true);
but it did not work. Slightly inconvenient that developers have to specify default class arguments constantly only because they want to set argument isAlertTooltip to be true.


=== This comment block is in case we want to use 32x32 icon ===
There are no CSS classes to work with 32x32 icons but only 64x64 in global.css as you pointed to me. It would make sense to add some rules for smaller images so that other developers could reuse them elsewhere. Like the following ->

/* ::::: alert icons 32x32 :::::*/

.message-icon-32,
.alert-icon-32,
.error-icon-32,
.question-icon-32 {
  width: 32px;
  height: 32px;
  margin: 6px;
  -moz-margin-end: 20px;
}

.message-icon-32 {
  list-style-image: url("chrome://global/skin/icons/information-32.png");
}

.alert-icon-32 {
  list-style-image: url("chrome://global/skin/icons/warning-32.png");
}

.error-icon-32 {
  list-style-image: url("chrome://global/skin/icons/error-32.png");
}

.question-icon-32 {
  list-style-image: url("chrome://global/skin/icons/question-32.png");
}

Although any changes I do to global.css they are not picked up as changes by mercurial (not sure why). So I ended up adding some CSS rules to debugger.inc.css as you will find in the diff I attached.
=== This comment block is in case we want to use 32x32 icon ===


=== Some generic questions not related to the bug ===
How do you make code blocks in the comments in Bugzilla?

Why Mozilla decided to use XUL instead of standard HTML?

I wonder if there is a GIT repository for fx-team? I never used Mercurial but have solid experience with GIT.

Was wondering if I can use some function in JS file in order to log in the terminal window whenever I run dev tools?

Comment 6

[Victor Porof [:vporof][:vp]]

Comment on attachment 8358807 [details] [diff] [review]
My second dummy diff

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

(In reply to Alexey Novak from comment #5)
> 
> I was playing around with the tooltip and (personally) found 64x64 icon not
> appropriate at all. It makes tooltip so ugly (it is my subjective opinion).
> I changed it to be 32x32 and it looks much more elegant in this use case.
> Please take a look and let me know what you think.
> 

Sure, whichever icon works best. A 32x32 should work fine.

> Another thing I was looking into this line:
> this._resumeOrderTooltip.setTextContent([label], defaultStyle, defaultStyle,
> true);
> 
> Tried to use ES6 something like:
> this._resumeOrderTooltip.setTextContent([label], ...true);

That's not how it works, unfortunately. The arguments spread operator is useful when defining functions, not when calling them.

> but it did not work. Slightly inconvenient that developers have to specify
> default class arguments constantly only because they want to set argument
> isAlertTooltip to be true.
> 

How about turning all arguments into properties of an object literal, and use destructuring in the method signature? This is pretty common in our codebase, and the function definition would be something like

setTextContent: function({ messages, messagesClass, containerClass, isAlertTooltip }) {
  messagesClass = messagesClass || "...";  
  containerClass = containerClass || "...";
  ... etc.
}

Then, you can call this method python-style:

.setTextContent({ messages: [...], isAlertTooltip: false });

> === This comment block is in case we want to use 32x32 icon ===
> There are no CSS classes to work with 32x32 icons but only 64x64 in
> global.css as you pointed to me. It would make sense to add some rules for
> smaller images so that other developers could reuse them elsewhere. Like the
> following ->
> 

Don't modify global.css. It's not included in the debugger, and used only for the browser chrome. We shouldn't bloat that file. The approach you took in this patch is definitely better.

> === Some generic questions not related to the bug ===
> How do you make code blocks in the comments in Bugzilla?
> 

You can't afaik :(

> Why Mozilla decided to use XUL instead of standard HTML?
> 

Oh man, this question renders an incredibly complex answer :) But to over-simplify things: there used to be a time when HTML was pretty awful at building UIs, and we wanted to use something like it for the browser UI, so Mozilla built XUL. It's been used ever since in lots of Mozilla and even other third-party products.

Nowadays, HTML is really quite powerful, so we try to use it instead of XUL whenever it makes more sense. For example, the Web Console is built in HTML these days, also the markup tree in the Inspector, for a few reasons like maybe allowing formatted output in the future, or making it easier for addons to extend them. There's no incentive to switch the Debugger away from using XUL yet, so we go along with that. I personally like XUL just as much as HTML.

> I wonder if there is a GIT repository for fx-team? I never used Mercurial
> but have solid experience with GIT.

Not fx-team afaik, but we have this mirror of mozilla-central: https://github.com/mozilla/gecko-dev

> Was wondering if I can use some function in JS file in order to log in the
> terminal window whenever I run dev tools?

Sure, you can use console.log/error, and that will log stuff to the browser console. Or dump(), which logs things to stdout in the terminal.

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

> > I wonder if there is a GIT repository for fx-team? I never used Mercurial
> > but have solid experience with GIT.
> 
> Not fx-team afaik, but we have this mirror of mozilla-central:
> https://github.com/mozilla/gecko-dev

That repo has an fx-team branch, which you can base your work off.

Comment 8

[Alexey Novak]

Attachment: 957634.patch

Much appreciated for your reply it was very educational for me.

I decided to go ahead and use deconstructing for setTextContent() function. Had to do a global search for this function and carefully change one by one whenever the function was Tooltip related. I do hope that did not miss anything.

Please let me know if there is anything I need to change.

Thanks

Comment 9

[Victor Porof [:vporof][:vp]]

Comment on attachment 8359241 [details] [diff] [review]
957634.patch

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

Right, this looks almost good to go! One last final change for r+.

Thanks for contributing!

::: browser/themes/shared/devtools/debugger.inc.css
@@ +615,5 @@
>  }
> +
> +/* Debugger toolbar alert icon */
> +
> +.debugger-toolbar-alert-icon {

Since other consumers of Tooltip.js might need to show alert icons in their text popups, these styles should be moved into a more shared file, because debugger.css isn't included in all tools.

themes/shared/devtools/common.css is the place where shared tooltip styles are placed at the moment, so move these there, along with the "Tooltip: Simple Text" styles.

Rename the class to something more generic too, like "devtools-tooltip-alert-icon".

Comment 10

[Alexey Novak]

Attachment: 957634.patch

Moved classes to common.css and renamed the class.

Comment 11

[Victor Porof [:vporof][:vp]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=ed7018eeb376

Comment 12

[Alexey Novak]

I'm sorry but I'm not sure what this link is (looks like a testing link for patches or something like that). Is something wrong?

Comment 13

[Alexey Novak]

Attachment: 957634.2.patch

Just in case.

I'm adding another version of the patch but WITHOUT the object literal argument in setTextContent()
In case if my previous path started to break things.

Comment 14

[Victor Porof [:vporof][:vp]]

"try runs" are means of testing whether everything still works properly. Standard procedure before landing, no need to worry about it.

Comment 15

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/6115213c5d03

Comment 16

[Alexey Novak]

Another silly question - do I need to change the status of the bug to be FIXED or someone else supposed to do that?

Comment 17

[Victor Porof [:vporof][:vp]]

(In reply to Alexey Novak from comment #16)
> Another silly question - do I need to change the status of the bug to be
> FIXED or someone else supposed to do that?

We'll take care of it once it merges over to mozilla-central.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6115213c5d03