Closed Bug 1814261 Opened 1 year ago Closed 1 year ago

Use moz-support-link in Mixed Content Blocking

Categories

(Toolkit :: UI Widgets, task, P3)

task

Tracking

()

RESOLVED FIXED
113 Branch
Tracking Status
firefox113 --- wontfix
firefox114 --- fixed

People

(Reporter: tgiles, Assigned: osuolale49, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug, Whiteboard: [recomp] [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)

See Also: → 1814266
Depends on: 1813077

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.

Flags: needinfo?(tgiles)

Thanks for reaching out but the dependent bug has not been fixed yet. Therefore this bug cannot be worked on yet.

Flags: needinfo?(tgiles)

To help Mozilla out with this bug, here's the steps:

  1. Comment here on the bug that you want to volunteer to help.
    This will tell others that you're working on the next steps.
  2. 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.
  3. 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.
  4. Build your change with mach build and test your change with mach test toolkit/components/passwordmgr --headless. Also check your changes for adherence to our style guidelines by using mach lint.
  5. 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.
  6. After a series of reviews and changes to your patch, I'll push it to autoland.
Mentor: tgiles
Keywords: good-first-bug
Whiteboard: [fidefe-reusable-components] → [fidefe-reusable-components] [lang=js]

I am currently looking into the bug.

I am currently working on the problem!

Assignee: nobody → osuolale49
Status: NEW → ASSIGNED

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?

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 themoz-support-linkcustom element in theidentityPanel.inc.xhtmlfile. If you want to finish up your contribution here (adding thewindow.ensureCustomElementscall inbrowser-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 🙂

Flags: needinfo?(osuolale49)

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

Flags: needinfo?(osuolale49)

(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:

  1. 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.
  2. 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
  1. 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.

(In reply to Noah Osuolale from comment #11)

  1. 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.

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.

Attachment #9325020 - Attachment description: Bug 1814261 - Use moz-support-link in Mixed Content Blocking. r?tgiles → WIP: Bug 1814261 - Use moz-support-link in Mixed Content Blocking. r?tgiles
Attachment #9325020 - Attachment description: WIP: Bug 1814261 - Use moz-support-link in Mixed Content Blocking. r?tgiles → Bug 1814261 - Use moz-support-link in Mixed Content Blocking. r?tgiles
Pushed by flodolo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c00c8fc88dbd
Use moz-support-link in Mixed Content Blocking. r=tgiles,flod
https://hg.mozilla.org/integration/autoland/rev/8640c4a18d9e
Fix strings after removing 'Learn More' markup. r=fluent-reviewers,flod
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 113 Branch

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.

Status: RESOLVED → REOPENED
Flags: needinfo?(osuolale49) → needinfo?(tgiles)
Resolution: FIXED → ---
Depends on: 1826824
Pushed by tgiles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/98b9ed7d4093
Use moz-support-link in Mixed Content Blocking. r=tgiles,flod
https://hg.mozilla.org/integration/autoland/rev/182986ee7412
Fix strings after removing 'Learn More' markup. r=fluent-reviewers,flod
Status: REOPENED → RESOLVED
Closed: 1 year ago1 year ago
Resolution: --- → FIXED
Flags: needinfo?(tgiles)
Whiteboard: [fidefe-reusable-components] [lang=js] → [recomp] [lang=js]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: