Sorry I just noticed you had already pushed.

I pushed this patch to the TRY server to see if all tests still pass: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4d3cbfcd590cc2aafa24c9324f927a383303fa9

Awesome, thank you!

Some improvements and cleanup

Great! Thank you Sebastian.

Addressed last review comments

This is probably a thing for Julien (or somebody else from the perf tool team), so forwarding to them.

Rebased and added a couple test cases and comments

Sorry about the delay here @Pranav.pandey . Based on the discussion so far, there isn't any way for DevTools alone (i.e. only using JS) to detect whether text-overflow is active on an element. the check we have now using the computed style isn't enough, and we need to implement a special privileged API in C++ that make it possible.
One thing we *can* do however is keep the second of the 2 warnings you added. The one that checks if overflow:hidden is defined. If this style is not applied, then for sure text-overflow won't have any effect. So we could keep this validator only, which means removing the other one, and the corresponding part of the test and localized string.
At least we could land that, and then continue the discussion in another bug.
The warning won't be catching *all* cases where text-overflow is inactive, but at least it won't be showing false positives, which is the most important.
Are you ok making those changes in this patch?

Rebased

Second attempt at fixing this, let me know what you think

I'm fine with the changes, I just have a few minor comments. But I also don't really know much about how clients and servers are set up, so maybe Julian should take a quick look at it too.

The other solution would be to stop using pre here. Split the string into 2 strings, and separate them by a <br>. This way the message can freely wrap as normal text does on web pages.
I'd prefer not to have to introduce this min-width if possible.

Sorry about the delay, please find some comments about your code changes below. Let me know if you have any questions, either here or on the bug.

Remove the flag and wait on the promise instead

This looks great to me, thanks!
Just 2 things needed now: a simple update of the string names as advised below. And a quick review from Daniel Holbert who I pinged on the bugzilla page, just to double check the logic.
Thanks for your work so far!

Thanks a lot for working on it.
I have one small comment about the code. But here's an idea: in order to make it even more useful for users, why not create 2 validators instead of 1. So, one would be for cases where the element isn't a block, and the other would be for cases where the element does not overflow.
This way, we'd have 2 different messages, and they'd be a lot more self-explanatory for users.
Would you be interested in amending the patch to do this?

Looks great to me. Thank you for working on this and removing this eslint warning.
I'll proceed to land this revision to m-c now.

This looks good to me, thank you for the patch!
I'll proceed to landing it in a second.

Moving this review to Razvan as this is something he knows better than I do.

Thank you for this patch!

Thanks for the patch!
I have one main comment about it that I'd like you to address before moving any further: the class-list.js file contains the model logic. The ClassList in there should only be concerned with managing the data related to classes on dom nodes. It should not worry about whether the space or enter keys were pressed.
So I'd like it if you could modify your code changes so the object only exposes methods that deal with data, instead of keyboard keys states.
Right now the main method that is exposed is addClass, I'm thinking perhaps it could instead expose a method called previewClass that takes a single name argument. Calling it the first time would add the class, and then calling it again and again would just modify that same class.
This way, you can keep the ClassList logic simple to understand and easy to use, and the rest of the logic can remain inside class-list-previewer.js.
Does that make sense?

Always refreshing on new top level targets

Thanks for the patch. I made a bunch of comments below, but I also posted inside the bug a comment related to investigating doing a client-side-only implementation of this. Please consider it and let me know what you think.

yes let's push it!

forgot to change target to currentTarget in a few tests

reverted back to the same id

@Honza could you please review the changes here. @ochameau has already looked at the approach related to how to use the TargetList API, but I could use your review too. Also, Alex, I added a new test that I think covers the use case now.

added a test

Perfect! Tested with and without the patch, and can confirm that the intended behaviour when copying is here, and keyboard navigation still works like before.
Code changes look sane to me.

Sorry I just realized I did not add a test. I'll find some time to add one soon.

addressed comments

Thanks Razvan, this looks good.

fixed apostrophe

Thanks for the review Daisuke. I got stuck trying to implement the solution you proposed. Let's chat later about this.

Changing the DevTools reviewer to Nicolas for the console changes.

rebased

Accounting for sideways too