Correctly name "Edit" option for SVG, XML, and MathML
Categories
(DevTools :: Inspector, enhancement, P5)
Tracking
(firefox88 fixed)
Tracking | Status | |
---|---|---|
firefox88 | --- | fixed |
People
(Reporter: sebo, Assigned: akjain6067.aj, Mentored)
Details
(Keywords: good-first-bug, Whiteboard: [lang=js])
Attachments
(3 files)
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
Reporter | ||
Comment 1•4 years ago
|
||
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
i want to contribute to code but dont know how to get started please help
Reporter | ||
Comment 4•4 years ago
|
||
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
Reporter | ||
Comment 5•4 years ago
|
||
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
Hi sebastian, I am new to open source contribution, but I gotta start somewhere, this looks interesting for me, I wanna take this up
Reporter | ||
Comment 7•4 years ago
•
|
||
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
Comment 9•4 years ago
|
||
(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
Comment 10•4 years ago
|
||
I agree with what Honza suggested above. And thanks for filing and offering to mentor :sebo!
Assignee | ||
Comment 11•4 years ago
|
||
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.
Comment 12•4 years ago
|
||
(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?
Assignee | ||
Comment 13•4 years ago
|
||
Thanks Julian,
I am still interested working on this, in the case Hitesh is no more interested working on this.
Ankit
Comment 14•4 years ago
|
||
Hi, I am new to open source and want to start. If it is still open I am ready to contribute.
Reporter | ||
Comment 15•4 years ago
|
||
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
Reporter | ||
Comment 16•4 years ago
|
||
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
Assignee | ||
Comment 17•4 years ago
|
||
Yes, I'm still interested working on this issue.
Thanks Sebastian,
Ankit
Reporter | ||
Updated•4 years ago
|
Comment 18•4 years ago
|
||
Hi Sebastian,
i have the working code for the dynamic edit option , can i create a patch and submit for review
thanks.
Reporter | ||
Comment 19•4 years ago
|
||
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
Reporter | ||
Comment 20•4 years ago
|
||
And btw., that was really fast! :-)
Sebastian
Assignee | ||
Comment 21•4 years ago
|
||
Then what should I do?
Comment 22•4 years ago
|
||
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
Reporter | ||
Comment 23•4 years ago
|
||
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
Assignee | ||
Comment 24•4 years ago
|
||
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
Reporter | ||
Comment 25•4 years ago
|
||
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
Assignee | ||
Comment 26•4 years ago
|
||
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.
Reporter | ||
Comment 27•4 years ago
|
||
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
Assignee | ||
Comment 28•4 years ago
|
||
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
Assignee | ||
Comment 29•4 years ago
|
||
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
Reporter | ||
Comment 30•4 years ago
|
||
(In reply to Ankit Jain from comment #28)
Hi Sebastian,
from comment 23Ankit, 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.
Reporter | ||
Comment 31•4 years ago
|
||
(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
Assignee | ||
Comment 32•4 years ago
|
||
(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
Reporter | ||
Comment 33•4 years ago
•
|
||
(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
Assignee | ||
Comment 34•4 years ago
|
||
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
Reporter | ||
Comment 35•4 years ago
|
||
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
Assignee | ||
Comment 36•4 years ago
|
||
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
Reporter | ||
Comment 37•4 years ago
•
|
||
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
Assignee | ||
Comment 38•4 years ago
|
||
Assignee | ||
Comment 39•4 years ago
|
||
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
Reporter | ||
Comment 40•4 years ago
|
||
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.
Assignee | ||
Comment 41•4 years ago
|
||
Okay okay no problem, get well soon Sebastian.
Thanks for all your help,
Ankit
Reporter | ||
Comment 42•4 years ago
|
||
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
Assignee | ||
Comment 43•4 years ago
|
||
Sure, I'll do that
Ankit
Assignee | ||
Comment 44•4 years ago
|
||
Hi Sebastian,
I've made the changes as you said.
Take care of your health
Thanks,
Ankit
Assignee | ||
Comment 45•4 years ago
|
||
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
Comment 46•4 years ago
|
||
Reporter | ||
Comment 47•4 years ago
•
|
||
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
Comment 48•4 years ago
|
||
bugherder |
Reporter | ||
Comment 49•4 years ago
|
||
Congratulations Ankit! Your patch is now merged and will be in Firefox 88.
Sebastian
Assignee | ||
Comment 50•4 years ago
|
||
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
Comment 51•4 years ago
|
||
Ankit & Sebastian, very well done!
Honza
Description
•