Closed Bug 1690880 Opened 4 months ago Closed 3 months ago

Correctly name "Edit" option for SVG, XML, and MathML

Categories

(DevTools :: Inspector, enhancement, P5)

Product:
DevTools
Component:
Inspector
Type:
enhancement

Tracking

(firefox88 fixed)

Status:
RESOLVED FIXED
Milestone:
88 Branch
Tracking Flags:
Tracking Status
firefox88 --- fixed

People

(Reporter: sebo, Assigned: akjain6067.aj, Mentored)

Details

(Keywords: good-first-bug, Whiteboard: [lang=js])

Attachments

(3 files)

XML with namespace.xml
331 bytes, text/xml
Details
Edit VSCode settings for ESLint
173.98 KB, image/png
Details
Bug 1690880 - Made label for element Edit option change dynamically depending on the element's namespace. r=sebo
48 bytes, text/x-phabricator-request
Details | Review

The option for editing the markup structure within the Inspector is currently called "Edit As HTML" no matter what markup structure the current node actually relates to.

Instead, the option should be called "Edit As SVG", "Edit As MathML", or "Edit As XML" depending on what namespace the selected element has.
You may also consider to just call it "Edit" or "Edit Markup" or something similar.

Sebastian

This also looks like a good start to dive into DevTools development, so marking it as good-first-bug. And I'd be happy to mentor anybody willing to change that.

Though before creating a patch for this, it first needs to be clarified if this option should actually be renamed and if yes, what naming scheme should be used.

In any case, the places that need to be touched in order to change this are here:

Usage of the label: https://searchfox.org/mozilla-central/rev/f982032c7c7618c626165bb557968f478a1952dc/devtools/client/inspector/markup/markup-context-menu.js#734
Determine whether a node is an HTML element (could be copied and adjusted for the other markup types): https://searchfox.org/mozilla-central/rev/f982032c7c7618c626165bb557968f478a1952dc/devtools/client/framework/selection.js#193-196
Localization: https://searchfox.org/mozilla-central/rev/f982032c7c7618c626165bb557968f478a1952dc/devtools/client/locales/en-US/inspector.properties#191

Sebastian

Mentor: sebastianzartner
Keywords: good-first-bug
Whiteboard: [lang=js]

Thanks for the report Sebastian.
Honza

Severity: -- → S3
Priority: -- → P5

i want to contribute to code but dont know how to get started please help

Hi Hitesh,

to get started with contributing to the Firefox DevTools, I suggest you first read the documentation at https://docs.firefox-dev.tools/. First you need to set up your development environment, which is described at https://docs.firefox-dev.tools/getting-started/build.html. There is also a quick reference at https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html.

Please let me know if you have any issues or once you're set up and ready to start coding!

Sebastian

Honza, Julian, do you think it's better to give the option a static but more generic name like "Edit" or "Edit Markup" or to dynamically change the it depending on the markup type?

Sebastian

Flags: needinfo?(odvarko)
Flags: needinfo?(jdescottes)

Hi sebastian, I am new to open source contribution, but I gotta start somewhere, this looks interesting for me, I wanna take this up

Hi James, in comment 3 Hitesh already wanted to work on this. So let's have them a try on this first. In the meantime you may want to pick up some other DevTools bug to work on.

In comment 2 I linked to the docs describing how you can set up your development environment.

Best regards,

Sebastian

(In reply to Sebastian Zartner [:sebo] from comment #7)

Hi James, in comment 3 Hitesh already wanted to work on this. So let's give them a try on this first. In the meantime you may want to pick up some other DevTools bug to work on.

In comment 2 I linked to the docs describing how you can set up your development environment.

Best regards,

Sebastian

Thanks Sebastian, I'll setup the enviornment and get myself to work on some other bugs if I can

(In reply to Sebastian Zartner [:sebo] from comment #5)

Honza, Julian, do you think it's better to give the option a static but more generic name like "Edit" or "Edit Markup" or to dynamically change the it depending on the markup type?

Chrome is also using "Edit as HTML" label (not dynamically changed when there is SVG, etc.), so I think that we should keep as is and dynamically change it according to the current markup type.

Honza

Flags: needinfo?(odvarko)

I agree with what Honza suggested above. And thanks for filing and offering to mentor :sebo!

Flags: needinfo?(jdescottes)

Hello Everyone,
I'm new to open source and Mozilla, I want to work one this enhancement, can you please assign me this.
I've set development environment.

(In reply to Ankit Jain from comment #11)

Hello Everyone,
I'm new to open source and Mozilla, I want to work one this enhancement, can you please assign me this.
I've set development environment.

Thanks Ankit!
Since Hitesh already offered to take this bug, we should check if they still want to work on it.
You can look for other DevTools good-first-bugs here https://bugs.firefox-dev.tools/?tool=all in the meantime.

Hitesh: can you confirm you want to take this bug, can I assign it to you?

Flags: needinfo?(kotechahitesh517)

Thanks Julian,
I am still interested working on this, in the case Hitesh is no more interested working on this.

Ankit

Hi, I am new to open source and want to start. If it is still open I am ready to contribute.

Hi Deepak, please read the previous messages on this bug. Hitesh was the first one to say they wanted to work on it and two others also replied already. If you want to contribute to the DevTools, please first set up your development environment as I outlined above. Then you can search for good-first-bugs at https://bugs.firefox-dev.tools/?tool=all.

Sebastian

Ok, Hitesh didn't answer Julian's request and James looks like already working on other things for now. So Ankit, are you still interested? If so, I'll assign the bug to you.

Please see comment 1 for a starting point. And note that as of comment 9 the displayed text should change depending on the markup type the selected element belongs to. The markup type is determined by the namespaceURI of the element.

In case the markup type cannot be determined, I'd say the fallback should be "Edit Markup".

Sebastian

Flags: needinfo?(kotechahitesh517) → needinfo?(akjain6067.aj)

Yes, I'm still interested working on this issue.
Thanks Sebastian,

Ankit

Flags: needinfo?(akjain6067.aj)
Assignee: nobody → akjain6067.aj
Status: NEW → ASSIGNED

Hi Sebastian,

i have the working code for the dynamic edit option , can i create a patch and submit for review

thanks.

Sure, you can submit it. Please ensure your commit message contains r=sebo at the end, so that I am set as reviewer for the patch.
Let me know if you need help with the submission for review!

Sebastian

And btw., that was really fast! :-)

Sebastian

Then what should I do?

Hi sebastian , this is my first contribution to open source .I dont know the steps to create and submit a patch ,it would be helpful if you could share some links showing the steps.

Thanks

Oh, wait kalyanshiva98. I assume you are not Ankit, who is already assigned to this bug and working on a patch.
I didn't realize that the persons writing me aren't the same. So let's give Ankit the chance to finish their patch.

Ankit, before submitting your patch for review, please ensure you run the linting test and fix any issues the test may warn about. How to do that is described in https://docs.firefox-dev.tools/contributing/eslint.html.

If there are no errors, you should also check whether the changes caused any automated tests to fail. In order to do that please read https://docs.firefox-dev.tools/tests/.

As far as I can see there is only one test related to the context menu item. To run that test execute this in the command line:

mach test devtools/client/inspector/test/browser_inspector_menu-01-sensitivity.js

To run all Inspector tests (which takes some time to finish) you can execute this

mach test devtools/client/inspector/test/*

When the tests say that everything's ok, you can submit the patch for review. How that's done is described at https://docs.firefox-dev.tools/contributing/code-reviews.html and in more detail at https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html.

If there are any more questions, feel free to reach out to me.

Sebastian

Hi Sebastian,
I've implemented this feature, the label is changing dynamically with HTML and SVG, now tell me what other markups should I add for eg. XML, MathML and also tell me what will be the namespaceURI for these markups.
Thanks,

Ankit

Attached file XML with namespace.xml —

Hi Ankit,

Great to hear that you could implement the change for HTML and SVG.

Yes, MathML and XML should be added to. MathML has http://www.w3.org/1998/Math/MathML as namespaceURI. MDN web docs has some examples where you can test your changes at https://developer.mozilla.org/en-US/docs/Web/MathML/Examples/MathML_Pythagorean_Theorem.

XML lets you define individual namespaces. Without any specification, namespaceURI returns null. See https://www.javatpoint.com/xmlpages/books.xml or https://www.xmlfiles.com/example/cd_catalog_with_css.xml as examples. Though I have also attached you an example XML with a namespace of https://bugzilla.mozilla.org/. Namespaces there can be any kind of URI.
So, I think we should use "Edit As XML" as a fallback label if the namespace is either null or is not one of HTML, SVG, or MathML.

And just for completeness, the namespace of SVG is http://www.w3.org/2000/svg. See https://www.mozilla.org/media/protocol/img/logos/mozilla/black.40d1af88c248.svg as an example.

There are other defined namespaces like for RDF, see the list at http://www.html-5.com/tutorials/html-namespaces.html. Though I guess the four mentioned are the most common ones. And in the end, all those markups are XML variants.

Sebastian

Hi Sebastian,
Thanks for this information.
In comment 16 you said to add fallback label as "Edit As Markup", now I am confused what should be the fallback label.

I changed my mind on "Edit As Markup". In XML basically any namespace is allowed, so I think the best is to let "Edit As XML" be the fallback.

Sebastian

Hi Sebastian,
from comment 23

Ankit, before submitting your patch for review, please ensure you run the linting test and fix any issues the test may warn about. How to do that is described in https://docs.firefox-dev.tools/contributing/eslint.html.

I'm following that article, it says,

Then go to the package settings and set the following option:
"eslint.nodePath": "tools/lint/eslint/node_modules/.bin"

I could not figure out in which file I should add this line
"eslint.nodePath": "tools/lint/eslint/node_modules/.bin"
Need help.
Thanks,

Ankit

Hi Sebastian,
When I ran mach test devtools/client/inspector/test/browser_inspector_menu-01-sensitivity.js test, I got this error
Method '_openMenu' has a complexity of 21. Maximum allowed is 20.eslint(complexity)
Before running this test there was not this error.
What does it mean and how should I fix it?

Thanks,
Ankit

(In reply to Ankit Jain from comment #28)

Hi Sebastian,
from comment 23

Ankit, before submitting your patch for review, please ensure you run the linting test and fix any issues the test may warn about. How to do that is described in https://docs.firefox-dev.tools/contributing/eslint.html.

I'm following that article, it says,

Then go to the package settings and set the following option:
"eslint.nodePath": "tools/lint/eslint/node_modules/.bin"

I could not figure out in which file I should add this line
"eslint.nodePath": "tools/lint/eslint/node_modules/.bin"
Need help.

Note that the paragraph you are referring to only applies if you are using Visual Studio Code.

That setting needs to be added to the settings.json of your VSCode workspace. There are multiple ways to get there. I've attached a screenshot for how to easily get there.

If you are not using VSCode, you can do the same by running mach eslint path/to/file in the command line.

(In reply to Ankit Jain from comment #29)

When I ran mach test devtools/client/inspector/test/browser_inspector_menu-01-sensitivity.js test, I got this error
Method '_openMenu' has a complexity of 21. Maximum allowed is 20.eslint(complexity)
Before running this test there was not this error.
What does it mean and how should I fix it?

The error message obviously comes from running ESLint. It is unrelated to the test file you are mentioning. The error basically says that the _openMenu function is too long. I assume the error refers to devtools/client/inspector/markup/markup-context-menu.js, which you probably changed in your patch. One way to get rid of that error is to move the parts determining the label out into their own function. Without actually seeing the code, though, it's hard for me to tell what exactly needs to be changed.

So if the patch works already, feel free to submit it for review and I'll have a look at it.

Sebastian

(In reply to Sebastian Zartner from comment #31)
Yes you guessed it right, that error was from ESLint and also I made separate function for determining label and error is gone.
Now tell me about naming convention of functions, what name should I use for function myFunction or _myFunction

(In reply to Ankit Jain from comment #32)

Yes you guessed it right, that error was from ESLint and also I made separate function for determining label and error is gone.

Great to hear that!

Now tell me about naming convention of functions, what name should I use for function myFunction or _myFunction

The underscore as method prefix indicates that the method is considered private per convention, i.e. only called from within the current class.

So, as the labelling function is only used within the MarkupContextMenu class, it should be _myFunction.

Sebastian

Hi Sebastian,
Help me with moz-phab, I have worked with git before, but I can not figure out what should be the next step with mercurial and moz-phab,
I have these questions,
What commit message should I write?
What command should I run to create and submit the patch?
What command should I run so you will be reviewer for this patch?

Thanks,
Ankit

Hi Ankit,

Sure, I'll help you!

Here's the command to put your changes into a commit (including the commit message):

hg commit -m "Made label for element "Edit" option change dynamically depending on the element's namespace. r=sebo"

The r=sebo causes Phabricator to automatically assign me as reviewer when you submit your patch for review.

To push your code to Phabricator you first need to set up moz-phab. How to do that is explained in https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html. See also the DevTools guide at https://docs.firefox-dev.tools/contributing/code-reviews-setup.html for help.

Once that's done, also you need to do is execute moz-phab in the command line without any parameters. When you did it right, moz-phab then shows you exactly that one commit and asks you to push your changes. Then you just need to press Enter and your patch will be submitted for review.

Let me know if something doesn't work as expected.

Sebastian

when I ran moz-phab I got this error

Submitting 1 commit for review:
(New) 568563:6dbc81e34e4b Made label for element Edit option change dynamically depending on the element's namespace. r=sebo
!! Missing Bug ID
Unable to submit commits:

568563:6dbc81e34e4b Made label for element Edit option change dynamically depending on the element's namespace. r=sebo
- missing bug-id

Oh, sorry, my fault!

Commit messages always require to have the bug ID.

You can change it by calling this in the command line:

hg commit --amend -m "Bug 1690880 - Made label for element Edit option change dynamically depending on the element's namespace. r=sebo"

Or you can call hg commit --amend without parameters, which opens an editor where you can add the bug ID to the commit message.

Sebastian

Thanks,
I think that is done, I got this message

Creating new revision:
568564:c755a7cd53d1 Bug 1690880 - Made label for element Edit option change dynamically depending on the element's namespace. r=sebo
0 files updated, 0 files merged, 0 files removed, 0 files unresolved

Completed
(D106574) 568565:33486d08ae6d Bug 1690880 - Made label for element Edit option change dynamically depending on the element's namespace. r=sebo
-> https://phabricator.services.mozilla.com/D106574

Now what should be my next step?

Ankit

Great, the submit for review worked! You can see it in comment 38.

I'll have a look at it. If I have any notes, I'll write them as comments in Phabricator.

Sebastian

PS: The last few days I am feeling a bit ill, so please forgive me if the review may take a little bit.

Okay okay no problem, get well soon Sebastian.

Thanks for all your help,

Ankit

I didn't feel that bad the last few hours so I still did the review. The changes aren't that big after all and already look really good. Thanks for that!
I just have two little remarks. You should have got an email from Phabricator related to the review.

To do the changes, just edit the code again and when you're finished, execute hg commit --amend in the command line, keep the commit message as is, and execute moz-phab again after that. That will automatically update the revision on Phabricator.

Sebastian

Sure, I'll do that

Ankit

Hi Sebastian,
I've made the changes as you said.
Take care of your health

Thanks,
Ankit

Hi Sebastian,
I've changed variable name as you mentioned.
I have one doubt related to release of firefox, when are the changes made to source code reflected to stable firefox?
What I think, first changes are released to nightly build and than developer edition and then stable firefox and developer edition is 3 months ahead of stable version of firefox, am I right?

And when can I consider myself as a open source contributor? because I want to show it off to my friends ( don't judge me :D ).

Thanks Sebastian for being a very good mentor, thank you very much for all of your help.

Ankit

Pushed by sebastianzartner@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2477c8f6bc4b
Made label for element Edit option change dynamically depending on the element's namespace. r=sebo

Hi Ankit,

Thank you for the last changes! I have pushed your changes to Lando right now! That means, in case there are no issues found in the automated tests on Treeherder, your patch will be merged soon! Be patient, though, landing a patch normally takes one or a few days.

(In reply to Ankit Jain from comment #45)

I have one doubt related to release of firefox, when are the changes made to source code reflected to stable firefox?
What I think, first changes are released to nightly build and than developer edition and then stable firefox and developer edition is 3 months ahead of stable version of firefox, am I right?

Your assumption of the process is right. Once the patch is merged, it will be available in the next day's Firefox Nightly build, which is at version 88. According to the release calendar, that version will officially be released on the 20th of April. So "just" 1.5 months to wait.

And when can I consider myself as a open source contributor? because I want to show it off to my friends ( don't judge me :D ).

I'd say, you can already consider yourself an open source contributor because you already provided a patch. :-) But once the patch landed and this bug is closed as "FIXED", you can officially brag about it. ;-) And don't worry, I am also proud that I am contributing things to Firefox. For Firefox 86, my changes related to the inactive CSS feature even made it in the official release notes. :-)

Thanks Sebastian for being a very good mentor, thank you very much for all of your help.

You're very welcome!

Sebastian

https://hg.mozilla.org/mozilla-central/rev/2477c8f6bc4b

Status: ASSIGNED → RESOLVED
Closed: 3 months ago
status-firefox88: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 88 Branch

Congratulations Ankit! Your patch is now merged and will be in Firefox 88.

Sebastian

I am very very happy becoming an open source contributor in Mozilla Firefox.
Thanks Sebastian and Mozilla team for being very supportive, I'll always try to contribute to Mozilla however and whenever possible.

Ankit

Ankit & Sebastian, very well done!

Honza

You need to log in before you can comment on or make changes to this bug.