1498224 - Apply new classnames as you type in the .cls section of the rule-view

Status: RESOLVED FIXED

(DevTools :: Inspector: Rules, enhancement, P3)

Description

[Patrick Brosset <:pbro>]

Steps:
- open the inspector on this page
- in the rule-view sidebar, click the .cls button
- in the new section that appears, start typing a new classname in the input field

==> On Chrome, this new class is applied as you type
==> On Firefox, you have to press enter for this class to be applied.

It would be good to align with Chrome to match users' expectations.

I think this might be a good first bug for someone with some JavaScript knowledge.
If you are interested, please first go through our getting started guide: https://docs.firefox-dev.tools/getting-started/
And when ready, the main changes you'll need to make will be in file /devtools/client/inspector/rules/views/class-list-previewer.js
Happy coding!

Comment 1

[Brahian E. Soto Mercedes :bsotodo]

@pbro I would like to work on this if that's ok

Comment 2

[Patrick Brosset <:pbro>]

Hi Brahian, thanks for your interest in fixing this. I'll assign the bug to you.
Please go through the docs I linked in comment 0, and let me know if you need any specific help.
I also gave the name of the file that will likely need to be changed to implement this feature in comment 0.
Don't hesitate to ask questions here or join our Slack channel to ask the community there: https://devtools-html-slack.herokuapp.com/

Comment 3

[Patrick Brosset <:pbro>]

Hey Brahian, are you still interested in working on this bug?
If not, please let me know so I can reset the assignee field.

Comment 4

[Brahian E. Soto Mercedes :bsotodo]

Hey Patrick, please don't. I will be working on this very soon.
Thanks

Comment 5

[Patrick Brosset <:pbro>]

Hi Brahian, sorry for asking again, but it's been 2 months. Please let me know if you still intend to work on this bug?

Comment 6

[Patrick Brosset <:pbro>]

Unassigning this bug so others can pick up where Brahian left off.

Comment 7

[Cédric :ced :cedgrvl]

Hey there may I ?

Comment 8

[Cédric :ced :cedgrvl]

Let me just the time to read the getting started guide

Comment 9

[Tanmaya [:Tanny_m]]

Hi,

Unless Cedric is not working on this issue, I would like to contribute to this bug

Thanks

Comment 10

[Patrick Brosset <:pbro>]

Hi Cédric, have you had a chance to go through the guide, install the dev environment, and give this a try?
If so, great, let me know and I'll assign the bug to you. Otherwise I will assign it to Tanmaya.

Once again, the file that needs changes is /devtools/client/inspector/rules/views/class-list-previewer.js, and as an additional clue, I think the onKeyPress function in there will need to be modified. Right now it only responds to the Enter key. But in this bug, we want changes to class names to happen live as you type. So this function should probably also handle other keys.

Comment 11

[Patrick Brosset <:pbro>]

(In reply to Tanmaya [:Tanny_m] from comment #9)

Hi,

Unless Cedric is not working on this issue, I would like to contribute to this bug

Thanks

It's all yours, feel free to comment here if you have any questions.

Comment 12

[Tanmaya [:Tanny_m]]

Hi Patrick,

I think the onKeyUp event should be used here to solve the purpose. I will try to integrate it with the devtools class-list-previewer.js and update you with my first patch. Meanwhile, could you suggest me how can I test the changes I make, so that I can know whether I am going in the right direction.

Thanks a bunch!

Comment 13

[Patrick Brosset <:pbro>]

Hi Tanmaya, in order to test your changes, you need to do the following:

• make sure you've built Firefox once (./mach build), this takes time
• make your code changes
• re-build them, this time it will much faster because you don't have to build C++, just JavaScript files: ./mach build faster
• open Firefox: ./mach run

Then you can just go back and forth between your code changes and testing them by redoing ./mach build faster and ./mach run every time.

Comment 14

[Tanmaya [:Tanny_m]]

Attachment: Bug 1498224 - Apply new classnames as you type in the .cls section of the rule-view. r=pbro

Comment 15

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Tanmaya, are you still planning to work on this bug? There is a review comment waiting for you.

Honza

Comment 16

[Tanmaya [:Tanny_m]]

Hi Honza,

Yes I am planning to work on this bug and try to address the revisions.

Tanmaya

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 18

[vishal]

Hi Honza,
I am very excited to contribute to mozilla. Because, I build firefox succefully on my pc yesterday.
I am interested in this issue please assign this issue to me and give me way to start.
Thanks

Comment 19

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Vishal, thanks for helping with this

Some instructions:

1. Contributor docs are now available here:
   https://firefox-source-docs.mozilla.org/devtools/

2. The keyboard listener that handlers pressing the "Enter" key is here
   https://searchfox.org/mozilla-central/rev/9043e515e9608cc55b252a40cb2dfb6f767bcffd/devtools/client/inspector/rules/views/class-list-previewer.js#184-203

3. The listener function onKeyPress needs to be improved and not only handle pressing Enter, but any key that is changing the value in the input. The new value should consequently be applied directly.

Honza

Comment 20

[vishal]

Thanks for reply. I just want to know why browser is not reflecting any changes even I am changing the whole file please help me out.
Thanks

Comment 21

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to vishal from comment #20)

Thanks for reply. I just want to know why browser is not reflecting any changes even I am changing the whole file please help me out.
Thanks

You need to rebuild Firefox after you make changes to the source code

mach build

When changing JS/CSS/HTML only you might:

mach build faster

See also:
https://firefox-source-docs.mozilla.org/devtools/contributing/fixing-bugs.html#find-out-where-are-the-files-that-need-changing

Comment 22

[vishal]

Thanks It is working.

Comment 23

[vishal]

Hello Sir,
I am working on this bug. But, here I am facing a problem. The keypress event is laging behind one key from actual response. How to overcome this problem please help me.

Comment 24

[vishal]

Can I use input event instead of using keypress event.

Comment 25

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to vishal from comment #24)

Can I use input event instead of using keypress event.

Sure, I don't have any specific solution in mind so, you can experiment with any DOM event that's available.
As soon as you have something (semi)working I can test it on my machine and provide some feedback.

I am working on this bug. But, here I am facing a problem. The keypress event is laging behind one key from actual response. How to overcome this problem please help me.

Not sure what could be the problem. If using different event doesn't help attach your patch and I can test it.

Honza

Comment 26

[vishal]

Thanks, I have created that solution using input event. Can I send the patch?

Comment 27

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Yes, please attach it through Phabricator

https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html

Honza

Comment 28

[vishal]

Attachment: WIP: Bug 1498224 - Apply new classnames as you type in the .cls section of the rule view

Comment 29

[vishal]

Hello sir,
I have submited my code changes to the phabricator. But, I don't know to give it for review.

Comment 30

[vishal]

Attachment: WIP: Bug 1498224 - Apply new classnames as you type in the .cls section of the rule view

Comment 31

[vishal]

Hello sir,
I commit mistakes in second last patch.
Please review this patch.
Thanks

Comment 32

[vishal]

Attachment: Bug 1498224 Reafactor some code to better performance

Depends on D110559

Comment 33

[vishal]

This is my final patch. Please review my code it is working perfectly for me.
Thanks

Comment 34

[vishal]

Hello sir,
Did you review my code?

Comment 35

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thank you for the update

Some notes:

1. Please add additional fixes to the same commit/patch to avoid attaching a new patch for every change
   See also: https://moz-conduit.readthedocs.io/en/latest/walkthrough.html

2. The first attached patch contains huge amount of unnecessary files

3. The second patch is changing .hgignore I don't think it should be part of the fix here.

Can you please fix the comments above and attach one patch that contains just the necessary changes?
(after that we can remove the old three patches)

Thank you for working on this issue,

Honza

Comment 36

[vishal]

I will do it. But, Did you review my changes?
If this was helpful please let me know.

Comment 37

[vishal]

Attachment: Bug 1498224 fix the comments in class-list-previewer.js file

Depends on D110560

Comment 38

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Here are the changes I've got together from all three patches.
I've been testing it and it fixes the problem.
For the review, I'd like to have feedback from :jdescottes.

diff --git a/devtools/client/inspector/rules/views/class-list-previewer.js b/devtools/client/inspector/rules/views/class-list-previewer.js
--- a/devtools/client/inspector/rules/views/class-list-previewer.js
+++ b/devtools/client/inspector/rules/views/class-list-previewer.js
@@ -198,16 +198,35 @@ class ClassListPreviewer {
     }
 
     if (this.addEl.value !== "" && event.key === "Enter") {
       this.addClassName(this.addEl.value);
     }
   }
 
   async onAddElementInputModified() {
+    const currentClasses = this.model.currentClasses;
+    const isTemp = currentClasses[currentClasses.length - 1];
+    if (this.addEl.value !== "") {
+      if (isTemp.isTemp && isTemp.isTemp === true) {
+        isTemp.name = this.addEl.value;
+        this.model.applyClassState();
+      } else {
+        currentClasses.push({
+          name: this.addEl.value,
+          isApplied: true,
+          isTemp: true,
+        });
+        this.model.applyClassState();
+      }
+    } else if (isTemp.isTemp === true && this.addEl.value === "") {
+      currentClasses.pop();
+      this.model.applyClassState();
+    }
+
     const newValue = this.addEl.value;
 
     // if the input is empty, let's close the popup, if it was open.
     if (newValue === "") {
       if (this.autocompletePopup.isOpen) {
         this.autocompletePopup.hidePopup();
         this.autocompletePopup.clearItems();
       }
@@ -239,16 +258,17 @@ class ClassListPreviewer {
     } else {
       this.autocompletePopup.setItems(items);
       this.autocompletePopup.openPopup();
     }
   }
 
   async addClassName(className) {
     try {
+      await this.model.currentClasses.pop();
       await this.model.addClassName(className);
       this.render();
       this.addEl.value = "";
     } catch (e) {
       // Only log the error if the panel wasn't destroyed in the meantime.
       if (this.containerEl) {
         console.error(e);
       }

Comment 39

[vishal]

Attachment: Bug 1498224 fix the comment line at 185 in class-list-previewer.js

Depends on D110658

Comment 40

[vishal]

Ok Thanks to inform me.

Comment 41

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Vishal, can you please merge all wanted changes into one patch?
It would be a lot easier to review it.

Honza

Comment 42

[vishal]

Attachment: Bug 1498224 - not for review

Depends on D111055

Comment 43

[vishal]

Attachment: Bug 1498224 - m adding more code r=Honza

Depends on D110559

Comment 44

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Hi Vishal,
I posted a comment into Phabricator (where the patch is), but all we need is to have one patch with all the necessary changes (see comment #38). All the existing patches attached to this bug can be marked as obsolete.

Honza

Comment 45

[vishal]

Attachment: WIP: Bug 1498224 - fake commit

Depends on D111060

Comment 46

[vishal]

Attachment: Bug 1498224 final commit. r=Honza

Depends on D111105

Comment 47

[vishal]

Will the final patch do it?

Comment 48

[vishal]

Can I Abandon all the previous patches?

Comment 49

[Julian Descottes [:jdescottes]]

(In reply to vishal from comment #47)

Will the final patch do it?

The stack on Phabricator is hard to understand.

Can you try to upload a single patch, based on top of the latest mozilla-central revision (hg update central), and send that for review?
Do not set it as the child of another diff or anything.

Take a look at the Phabricator user guide if needed: https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html

Comment 50

[vishal]

Attachment: Bug 1498224 - [devtools] Apply new classnames as you type in the .cls section of the rule-view.

use they input handling event.

Comment 51

[Julian Descottes [:jdescottes]]

(In reply to vishal from comment #48)

Can I Abandon all the previous patches?

Yes, we should abandon all the other patches. It will clean up the dashboard for this bug.
Thanks

Comment 52

[vishal]

Hello,
Sorry to late response. I just want to know that where to put that new functionality in class-list.js. Please response me so i can start it.

Comment 53

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 54

[Raphaël Ferrand]

Hello,
I see this ticket is unassigned. Can I jump on it? Is it ready to be worked on? (from what I saw in the chat history there was some issue of it being blocked by some other ticket)
Thanks

Comment 55

[Julian Descottes [:jdescottes]]

I missed the needinfo from Vishal, but since there was no update after the bug was unassigned, I assume it's fine to reassign.
Thanks for offering to work on this Raphael, I am assigning the bug to you.

You can take a look at previous attempts to work on this https://phabricator.services.mozilla.com/D111232 and https://phabricator.services.mozilla.com/D61316 .

The contributor documentation can be found at https://firefox-source-docs.mozilla.org/setup/index.html and if you need help either setting up the environment or with the bug, you can either ask here (make sure to use the "Request information from" check below the comment box so that we don't miss you question) or on our chat: https://chat.mozilla.org/#/room/#devtools:mozilla.org

Comment 56

[Raphaël Ferrand]

Attachment: Bug 1498224 - [devtools] Apply new classnames as you type in the .cls section of the rule-view r=jdescottes,Honza

Comment 57

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c74d6dc5ebd7
[devtools] Apply new classnames as you type in the .cls section of the rule-view r=jdescottes

Comment 58

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/c74d6dc5ebd7