Open Bug 1543628 Opened 5 years ago Updated 2 years ago

Rename Debugger image classes for easier sharing with other panels

Categories

(DevTools :: Debugger, task, P3)

task

Tracking

(Not tracked)

People

(Reporter: fvsch, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug)

Attachments

(1 file)

Currently images in Debugger are styled like this:
https://searchfox.org/mozilla-central/source/devtools/client/debugger/src/components/shared/AccessibleImage.css

Abridged:

.img {
  width: 16px;
  height: 16px;
  background-color: var(--theme-icon-color);
}

.img.foo {
  mask: url(debugger/images/foo.svg);
}

.img.bar {
  mask: url(debugger/images/bar.svg);
}

This can create styling issues because we are popping very generic class names on elements, including img itself and others like refresh, help, tab, close, file, etc. These classes could easily be styled by existing Console or Netmonitor styles.

This also creates issues in Debugger itself, for instance we have this in the Threads pane:

<div class="workers-list">
  <div class="worker">
    <div class="icon">
      <span class="img worker"></span>
    </div>
    <div class="label">
      ...
    </div>
  </div>
</div>

<style>
.workers-list .worker:hover {
  background-color: var(--search-overlays-semitransparent);
}
</style>

That style will apply to both the "worker" line and the "worker" icon (changing the icon color to an invisible shade on hover).

It can be worked around with a more precise selector, but my point is that for a shared element we can't work around all possible conflicts and should prevent them with stricter naming.

Proposal

  1. Ideally, rename the AccessibleImage component to just Image or DebuggerImage (it doesn't have any accessibility feature; we could add the ability to pass alt text, though.)

  2. Change the base class from img to something a bit less generic, for instance dbg-img.

  3. Change all the image specific class names from <name> to dbg-img-<name>, for instance: dbg-img-worker.

This would be a small-to-medium refactoring effort, but would allow reusing Debugger images in other contexts, especially in Console.

Nicolas, do you think that could solve your needs with images from Debugger (e.g. frameworks logos) in Console?

Flags: needinfo?(nchevobbe)

definitely!

Flags: needinfo?(nchevobbe)
Type: defect → task
Priority: -- → P3
Blocks: 1565711
Blocks: 1565713

Hi @jlast,
I would like to work on this bug

Go for it.

Assignee: nobody → kanurag94

Welcome Anurag; let us know if you need help :)

Thank you!

Hi David, Do I need to change the specific classes names too? I will update the patch if yes.

Hi Anurag. The patch is looking great. I think we may need more changes to the class names we output though, to be on the safe side.

Securing classnames a bit

For some background on why this matters, see this issue: Debugger CSS uses global classNames, risks conflicts.

Currently when we use AccessibleImage component or the updated DebuggerImage component, this is what we get:

// JSX
<AccessibleImage className="close" />
<DebuggerImage className="close" />

// DOM
<span class="img close" />
<span class="dbg-img close" />

If we drop this kind of DOM in Console or some other DevTools panel, every generic-looking class is a conflict risk. With your changes, we avoid dropping an element with the generic img class, but we're still taking risks with the close class. What if Console is styling .webconsole-output-wrapper .close, or ends up doing it in the future?

Not an awful risk, but still risky IMO.

So my recommended change would be to change the component's API so that it works like this:

// JSX
<DebuggerImage name="file" />
<DebuggerImage name="file" className="active" />

// DOM
<span class="dbg-img dbg-img-file" />
<span class="dbg-img dbg-img-file active" />

(Keeping the className prop for compatibility with how this component is used in Debugger in some specific instances.)

And on the CSS side, we can have:

- .img.file {
+ .dbg-img-file {
  mask-image: url(resource://devtools/client/debugger/images/file-small.svg);
  mask-size: 12px 12px;
}

Changes to Debugger CSS files

Since we're renaming the .img class, we're also going to have to rename it in a bunch of stylesheets.
Searching for .img in Debugger stylesheets shows:

  • 41 results in AccessibleImage.css (addressed in your patch)
  • 30 results in SourceIcon.css (to do)
  • 35 results in 15 other files (to do)

For instance, we could change:

- .search-field .img.search {
+ .search-field .dbg-img-search {

Thanks Florens for such helpful review. I'll do the changes and update patch soon

Hi Florens, I have updated the patch. You can checkout the current revision

I will update complete revision soon with changes to component's API

Hi Florens,

I understand good first bugs should be self explanatory and you have given pretty much all details.
Although I couldn't figure out how to do changes as per 'Securing classnames a bit' part.
Am I supposed to change the class names manually, for eg
dbg-img.worker to dbg-img-worker in Debugger.css
also
<div className="loader" key="pretty-loader"> <DebuggerImage className="arrow" /> </div>
to

<div className="loader" key="pretty-loader"> <DebuggerImage className="dbg-img dbg-img-arrow" /> </div>

I was unable to follow the example:

 JSX
<DebuggerImage name="file" />
<DebuggerImage name="file" className="active" />

DOM
<span class="dbg-img dbg-img-file" />
<span class="dbg-img dbg-img-file active" />

Does this mean now name='worker' and className='down'?

My current revision has debugger.css updated. I am quite new to frontend. Hope you don't mind. :D

So the idea was to use a name prop on the DebuggerImage component that would automatically prefix the name with dbg-img-. This can be implemented as:

const DebuggerImage = (props: Object) => {
  const { name, className, ...attributes } = props;
  return (
    <span
      className={classnames("dbg-img", `dbg-img-${name}`, className)}
      {...attributes}
    />
  );
};

I'm trying it out locally, with some Flow types it seems to be working well.
I can update your patch if that helps.

One could use this compoment as:

<DebuggerImage name="folder" />
// <img class="dbg-img dbg-img-folder" />

<DebuggerImage name="arrow" className={{ expanded }} />
// if expanded == false: <img class="dbg-img dbg-img-arrow" />
// if expanded == true:  <img class="dbg-img dbg-img-arrow expanded" />

Yes, please go ahead and do changes. I really wish to know what I was doing wrong.

No longer blocks: 1565711
No longer blocks: 1565713

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

Assignee: kanurag94 → nobody

@kanurag94 Id like to work on this project for my outreachy contribution. please assign it to me

Sure! assigned to you.

Assignee: nobody → teresiawachirakabura1
Whiteboard: dt-outreachy-2021

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

Assignee: teresiawachirakabura1 → nobody
Severity: normal → S3
Whiteboard: dt-outreachy-2021
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: