Stop using xlink:href in SVG (just use href) in remaining parts of the codebase
Categories
(Firefox :: Theme, enhancement, P5)
Tracking
()
Tracking | Status | |
---|---|---|
firefox91 | --- | fixed |
People
(Reporter: jaws, Assigned: silke, Mentored)
References
Details
(Keywords: good-first-bug, perf)
Attachments
(7 files, 3 obsolete files)
1.72 KB,
text/x-ruby-script
|
nhnt11
:
feedback+
|
Details |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review |
Comment 1•7 years ago
|
||
Comment 2•7 years ago
|
||
Comment 3•7 years ago
|
||
Reporter | ||
Comment 4•7 years ago
|
||
Comment 5•7 years ago
|
||
Comment 6•7 years ago
|
||
Comment 7•7 years ago
|
||
Comment 8•7 years ago
|
||
Comment 9•5 years ago
|
||
Hey Jeremy, what's the status of this bug? Are you still interested in driving this over the line? :-)
Comment 10•5 years ago
|
||
Oh I totally forgot about this one :/
Sure I need to finish fixing an important one first, and I'll keep this one in mind (since it will now appear on top of my dashboard it should be better !)
Thanks for reminding me of it! :)
Comment 11•4 years ago
|
||
This good-first-bug hasn't had any activity for 6 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.
Updated•4 years ago
|
Comment 12•4 years ago
|
||
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 | ||
Comment 13•4 years ago
|
||
Updated•4 years ago
|
Assignee | ||
Comment 14•4 years ago
|
||
I wrote this script to generate the patch. I'll provide more information this weekend, but in the meantime I just wanted to have this uploaded already.
Updated•4 years ago
|
Assignee | ||
Comment 15•4 years ago
|
||
Some info about the attached script: It finds all SVG files recursively, starting from the given directory. Each file is searched for the Strings to be modified, and the found Strings are replaced. By default the script only prints the changed files as a dry run. With -w the changes are written to the files.
Let me know if there is any additional info I can provide! :)
Comment 16•4 years ago
|
||
(In reply to Silke Hofmann from comment #15)
Some info about the attached script: It finds all SVG files recursively, starting from the given directory. Each file is searched for the Strings to be modified, and the found Strings are replaced. By default the script only prints the changed files as a dry run. With -w the changes are written to the files.
Let me know if there is any additional info I can provide! :)
Nice, I love the dry-run feature! :-)
Assignee | ||
Comment 17•4 years ago
|
||
v2 includes two changes in the original script:
- Files that are in test/ directories are now omitted.
- When an attribute is removed from the svg tag, empty lines and lines only containing ">" are cleaned up.
Thanks for the input & feedback! :)
Updated•4 years ago
|
Assignee | ||
Comment 18•4 years ago
|
||
Assignee | ||
Comment 19•4 years ago
|
||
Depends on D117203
Assignee | ||
Comment 20•4 years ago
|
||
Depends on D117204
Assignee | ||
Comment 21•4 years ago
|
||
Depends on D117205
Assignee | ||
Comment 22•4 years ago
|
||
Depends on D117206
Assignee | ||
Comment 23•4 years ago
|
||
Depends on D117207
Assignee | ||
Comment 24•4 years ago
|
||
I’ve made the following changes since the last (now abandoned) commit:
- The script now only searches & modifies SVG files that are not located in a test directory. The logic for identifying all test directories has been taken from Searchfox.
- The regex patterns used for find & replace the Strings have been extended to remove any empty lines, and lines only containing “>” after modification.
- As for SVG files within the “docs” directory, it turns out the xlink variable is called xl, so additional logic has been implemented to also find and replace xl:href & xmlns:xl="http://www.w3.org/1999/xlink”.
- I’ve extended the script to identify all top level directories that contain relevant SVG files: “docs”, "browser”, “devtools", "dom", "toolkit" and ”services”. I’ve created one new commit for each directory. Across all directories, only 58 SVG files are touched now.
Comment 25•4 years ago
|
||
(In reply to Silke Hofmann from comment #24)
Awesome. I skimmed all of your patches and they look good. At the moment, they are all marked as WIP, could you add me as a reviewer on each of them and submit for review? I can help find additional reviewers as well where necessary.
I’ve made the following changes since the last (now abandoned) commit:
- The script now only searches & modifies SVG files that are not located in a test directory. The logic for identifying all test directories has been taken from Searchfox.
✨
- The regex patterns used for find & replace the Strings have been extended to remove any empty lines, and lines only containing “>” after modification.
✨
- As for SVG files within the “docs” directory, it turns out the xlink variable is called xl, so additional logic has been implemented to also find and replace xl:href & xmlns:xl="http://www.w3.org/1999/xlink”.
🧐👍
I just realized, since we are removing the xlink namespace we should not only remove xlink:href but any other attribute name that is prefixed with xlink:, i.e. xlink:foo. But going through the patches, it's fairly trivial to verify that there are no other values of foo than "href", so we are good.
- I’ve extended the script to identify all top level directories that contain relevant SVG files: “docs”, "browser”, “devtools", "dom", "toolkit" and ”services”. I’ve created one new commit for each directory. Across all directories, only 58 SVG files are touched now.
Wow, that's two orders of magnitude smaller than if we include tests. Could you file a follow up bug, blocking this one, for investigating removing this from SVGs in test directories where possible?
We should also have a follow-up bug to investigate adding some kind of linting that prevents us from introducing more of these in the future. I'm not aware of whether we have any infrastructure to lint SVGs, but that can be figured out by whoever works on it when the time comes. 😉
Thanks for working on this!
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Comment 26•4 years ago
|
||
Assignee | ||
Updated•4 years ago
|
Comment 27•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/00fd6a554c23
https://hg.mozilla.org/mozilla-central/rev/aa2d06d17d5b
https://hg.mozilla.org/mozilla-central/rev/24f0b71ae997
https://hg.mozilla.org/mozilla-central/rev/2b5c3a210260
https://hg.mozilla.org/mozilla-central/rev/623dff6d1af4
https://hg.mozilla.org/mozilla-central/rev/f531f12e5c35
Comment 28•4 years ago
|
||
Comment on attachment 9225078 [details]
Script to generate patch - v2
A little late to set this flag but just wanted to acknowledge for posterity that I've seen this script and also collaborated on it with Silke off of the bug and it looks good.
Updated•1 year ago
|
Description
•