Use moz-support-link in Mixed Content Blocking
Categories
(Toolkit :: XUL Widgets, task, P3)
Tracking
()
People
(Reporter: tgiles, Assigned: osuolale49, Mentored)
References
(Blocks 1 open bug)
Details
(Keywords: good-first-bug, Whiteboard: [fidefe-reusable-components] [lang=js])
Attachments
(2 files)
There are a few places in identityPanel.inc.xhtml
that are using XUL learn more label links. We should replace those links, the ones using "identity-popup-mcb-learn-more", with moz-support-link
. (Note: fixing "identity-popup-custom-root-learn-more" is Bug 1814266)
Updated•2 years ago
|
Reporter | ||
Updated•2 years ago
|
Comment 1•2 years ago
|
||
Hello, I see that the bug that this one depends on (Bug 1813077) has been fixed. Does that mean I can start working on this bug, or has it become obsolete?
If I can still work on this bug, I would like to be assigned to it.
Reporter | ||
Comment 2•2 years ago
|
||
Thanks for reaching out but the dependent bug has not been fixed yet. Therefore this bug cannot be worked on yet.
Reporter | ||
Comment 3•2 years ago
|
||
To help Mozilla out with this bug, here's the steps:
- Comment here on the bug that you want to volunteer to help.
This will tell others that you're working on the next steps. - Download and build the Firefox source code
- If you have any problems, please ask on Element/Matrix in the
#introduction
channel. They're there to help you get started.
- If you have any problems, please ask on Element/Matrix in the
- Start working on this bug.
- The easiest way I've found to show all these elements in the user interface (UI) is the following:
- Go to any website (say bugzilla.mozilla.org)
- Open the browser toolbox and enable the "Disable Popup Auto-Hide" feature under the three dots/meatball/kebab menu
- In the main urlbar, activate the lock icon to bring up the "Connection security for bugzilla.mozilla.org" popup. Thanks to the previous setting, this popup will not automatically close if you happen to move focus in the window.
- Use the browser toolbox to inspect this popup and find one of the hidden <description> tags then disable the
display:none
rule under the:is([when-connection], [when-customroot], [when-mixedcontent], [when-ciphers], [when-httpsonlystatus])
selector - Now you should be able to find the "Learn more" links that are relevant to this bug (please only fix the "identity-popup-mcb-learn-more" in this bug!)
- Change these <label class="identity-popup-mcb-learn-more ..."/> links to moz-support-links. View this patch to see how to use the moz-support-link markup
- Remove the redundant code in browser-siteIdentity.js after adding these new moz-support-links
- If, after changing the markup, the new moz-support-link does not work correctly in the places you have changed, please reach out to me as we may need to import the moz-support-link module in identity-popup.
- If you have any problems with this bug, please comment on this bug and set the needinfo flag for me. Also, you can find me and my teammates on the
#reusable-components
channel on Element/Matrix most hours of most days.
- The easiest way I've found to show all these elements in the user interface (UI) is the following:
- Build your change with
mach build
and test your change withmach test toolkit/components/passwordmgr --headless
. Also check your changes for adherence to our style guidelines by usingmach lint
. - Submit the patch (including an automated test, if applicable) for review. Mark me as a reviewer so I'll get an email to come look at your code.
- How to Submit a Patch
- This is when the bug will be assigned to you.
- After a series of reviews and changes to your patch, I'll push it to autoland.
- If there are changes requested, please read the "To update a submitted patch" section to ensure you don't accidentally create a duplicate revision!
- Your code will soon be shipping to Firefox users worldwide!
Assignee | ||
Comment 4•2 years ago
|
||
I am currently looking into the bug.
Assignee | ||
Comment 6•2 years ago
|
||
Updated•2 years ago
|
Assignee | ||
Comment 7•2 years ago
|
||
After investigating further, I think there might be a problem with this:
If, after changing the markup, the new moz-support-link does not work correctly in the places you have changed, please reach out to me as we may need to import the moz-support-link module in identity-popup.
How do we import the mpz-support-link module in identity-popup?
Reporter | ||
Comment 8•2 years ago
|
||
Okay, so this is a bit more work than I previously estimated...no perfect way to know how much work a bug is until you start working on it unfortunately 😅 First, to answer your immediate question, we will need to add window.ensureCustomElements("moz-support-link")
in browser-siteIdentity.js in the
_initializePopup()function. We will add this to section where the popup has not yet been initialized. This will allow us to use the
moz-support-linkcustom element in the
identityPanel.inc.xhtmlfile. If you want to finish up your contribution here (adding the
window.ensureCustomElementscall in
browser-siteIdentity.js`), I can modify the bug and file the rest of the issues as follow-up bugs. Otherwise, you can fix these other issues as part of this bug.
I thought this would be a straightforward "swap the label for moz-support-link" bug, but this isn't the case. Since the Fluent strings in the Mixed Content Blocking section have markup in them, for example identity-description-custom-root, your current change of replacing the <label>s with <html:a is="moz-support-link"> will never work. This is because the newly added <html:a> element will be destroyed when Fluent translates (localizes) the parent element.
Given that the current markup situations works, we can use markup inside of Fluent. We will need to change the related parent strings in browser.ftl to handle the new additions of the moz-support-link
elements. For example, one of the labels you changed is a child of data-l10n-id="identity-description-active-blocked"
. So we need to go to browser.ftl and change the markup within this string. Instead of <label ...> ... </label>, it will instead need to be <a ...></a>. We don't need to add a string in this anchor element as the moz-support-link handles its own translation.
However, I found a bug in moz-support-link for this case where the translation does not work correctly. It can easily be fixed by moving one line around in moz-support-link.mjs. Instead of translateFragment
being inside of that if statement, it needs to be on the outside so the fragment is always translated when the support link becomes connected to the DOM.
Again, you can choose not to work on these follow-up issues and I can file bugs instead. The one thing you need to do is to get the moz-support-link imported in browser-siteIdentity.js. Let me know how you want to proceed 🙂
Assignee | ||
Comment 9•2 years ago
|
||
Hi Tim,
I have worked on importing moz-support-link in browser-siteIdentity.js as shown below (I included the async/await syntax)
_popupInitialized: false,
async _initializePopup() {
if (!this._popupInitialized) {
await window.ensureCustomElements("moz-support-link");
let wrapper = document.getElementById("template-identity-popup");
wrapper.replaceWith(wrapper.content);
this._popupInitialized = true;
}
},
Working on the other issues, what I currently have in identityPanel.inc.xhtml remains the same as in my above patch, however, I have made the required change to the moz-support-link.mjs file, to have the below (document.l10n.translateFragment(this) has been moved outside of the if statement).
connectedCallback() {
this.#register();
this.#setHref();
this.setAttribute("target", "_blank");
this.addEventListener("click", this);
if (!this.getAttribute("data-l10n-id")) {
document.l10n.setAttributes(this, "moz-support-link-text");
}
document.l10n.translateFragment(this);
}
Lastly, I have made the required change in browser/locales/en-US/browser/browser.ftl file, to use the anchor tag instead. Does this statement, "We don't need to add a string in this anchor element as the moz-support-link handles its own translation." mean the attribute, data-l10n-name="link" should be removed so we have <a>Learn More</a> or it should still stay <a data-l10n-name="link">Learn More</a>
If all these are verified, I can submit a second patch right away. Thank you
Reporter | ||
Comment 10•2 years ago
|
||
(Just a friendly suggestion that you can use backticks or tildes to create fenced code blocks that will help with formatting in the future. See this extended syntax of Markdown that shows some examples 🙂)
(In reply to Noah Osuolale from comment #9)
Hi Tim,
I have worked on importing moz-support-link in browser-siteIdentity.js as shown below (I included the async/await syntax)_popupInitialized: false,
async _initializePopup() {
if (!this._popupInitialized) {
await window.ensureCustomElements("moz-support-link");
let wrapper = document.getElementById("template-identity-popup");
wrapper.replaceWith(wrapper.content);
this._popupInitialized = true;
}
},
You don't need to worry about the async/await syntax here. I'm not sure what the ramifications are of using async/await here and I've observed no test failures or other oddities in my own testing. You can just call window.ensureCustomElements(...)
to simplify things.
Working on the other issues, what I currently have in identityPanel.inc.xhtml remains the same as in my above patch, however, I have made the required change to the moz-support-link.mjs file, to have the below (document.l10n.translateFragment(this) has been moved outside of the if statement).
connectedCallback() {
this.#register();
this.#setHref();
this.setAttribute("target", "_blank");
this.addEventListener("click", this);
if (!this.getAttribute("data-l10n-id")) {
document.l10n.setAttributes(this, "moz-support-link-text");
}
document.l10n.translateFragment(this);
}
Thanks for doing that, looks good to submit.
Lastly, I have made the required change in browser/locales/en-US/browser/browser.ftl file, to use the anchor tag instead. Does this statement, "We don't need to add a string in this anchor element as the moz-support-link handles its own translation." mean the attribute, data-l10n-name="link" should be removed so we have <a>Learn More</a> or it should still stay <a data-l10n-name="link">Learn More</a>
I've been made aware we'll have to do two things for this case:
- You'll need to use
<a data-l10n-name="link"></a>
. Since moz-support-link handles its own localization, we don't need to include that extra string between the tags. - Since we're modifying a Fluent string, we're going to need to change the ID and update the references to the new string. (This is how Fluent works and ensures modified strings are localized correctly). You'll need to update the string ID and update the reference in
identityPanel.inc.xhtml
Assignee | ||
Comment 11•2 years ago
|
||
- Since we're modifying a Fluent string, we're going to need to change the ID and update the references to the new string. (This is how Fluent works and ensures modified strings are localized correctly). You'll need to update the string ID and update the reference in
identityPanel.inc.xhtml
Please, I don't get this. Can you elaborate? Do you mean I need to add an id attribute to say, <a data-l10n-name="link"></a>
and reference it in identityPanel.inc.xhtml
where <html:a is="moz-support-link"/>
is used?
Thank you.
Reporter | ||
Comment 12•2 years ago
|
||
(In reply to Noah Osuolale from comment #11)
- Since we're modifying a Fluent string, we're going to need to change the ID and update the references to the new string. (This is how Fluent works and ensures modified strings are localized correctly). You'll need to update the string ID and update the reference in
identityPanel.inc.xhtml
Please, I don't get this. Can you elaborate? Do you mean I need to add an id attribute to say,
<a data-l10n-name="link"></a>
and reference it inidentityPanel.inc.xhtml
where<html:a is="moz-support-link"/>
is used?
Thank you.
Sure 🙂 you do not need to add an ID to the anchor (<a>
) element, but will need to update the relevant IDs in the browser.ftl (ftl is Fluent) file. So, for example, you will have modified the "identity-description-active-blocked" string in browser.ftl to no longer have the "Learn more" text between the newly added <a> tags. Since we have changed the content of this string, we will need to update the name of the string and any references to it. So in the previous case, identity-description-active-blocked
becomes identity-description-active-blocked2
(it is perfectly fine to simply add a "2" to the string, or increment the number if a string already ended with a "2"). Now that we have updated that string in the Fluent file, we will need to update the other references to this string. The easiest way (that I know of) is to use searchfox for this and search for the previous ID of the Fluent string before you made your change. In this case, we do not need to worry about changing the code under the "Third-party code" section, only the "Core code" section. So you'll need to go back to identityPanel.inc.xhtml
and update the relevant data-l10n-id
of the string you updated over in the Fluent file. So the identity-description-active-blocked
becomes identity-description-active-blocked2
in identityPanel.inc.xhtml
Note, you'll need to do this process for each of the moz-support-link elements you added (but the process should be the same. Find, then replace the relevant strings/IDs). Feel free to needinfo me if you need additional clarification 🙂 You can do this by checking the "Request information from" checkbox and selecting the "mentor" option from the dropdown box next to the "Request information from" string. This way I get a direct notification instead of a plain email.
Updated•2 years ago
|
Updated•2 years ago
|
Reporter | ||
Comment 13•2 years ago
|
||
Depends on D173620
Comment 14•2 years ago
|
||
Comment 16•2 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c00c8fc88dbd
https://hg.mozilla.org/mozilla-central/rev/8640c4a18d9e
Reporter | ||
Comment 17•2 years ago
|
||
There are some other changes that need to be applied that are a bit out of scope for a good first bug. I'll add those changes to this bug as needed.
Updated•2 years ago
|
Comment 18•2 years ago
|
||
Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/6d48e301e9fb
Comment 19•2 years ago
|
||
Comment 20•2 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/98b9ed7d4093
https://hg.mozilla.org/mozilla-central/rev/182986ee7412
Reporter | ||
Updated•2 years ago
|
Updated•2 years ago
|
Description
•