1819205 - Translate the 'title' and 'placeholder' attributes in Full Page Translations

Status: RESOLVED FIXED

(Firefox :: Translations, enhancement, P3)

Description

[Greg Tatum [:gregtatum] (REO Fx132)]

After Bug 1813793, it will need to handle placeholders for inputs, which are currently not translated.

Comment 1

[Greg Tatum [:gregtatum] (REO Fx132)]

I'm updating this to be about both the title attribute and placeholder attribute as they are both visible to the UI, and should involve the same implementation.

Comment 2

[Greg Tatum [:gregtatum] (REO Fx132)]

The addon doesn't do this, but other browsers do, so it's a parity feature.

Comment 3

[Greg Tatum [:gregtatum] (REO Fx132)]

And alt.

Comment 4

[Greg Tatum [:gregtatum] (REO Fx132)]

I think since the addon doesn't have this either, we should add this to the general backlog, not the Desktop MVP.

Comment 6

[Marco Castelluccio [:marco]]

The addon supports translating the title unless the options to display errors is selected. That said, I still think this is a follow-up and doesn't have to be part of the MVP.

Comment 7

[Greg Tatum [:gregtatum] (REO Fx132)]

The addon and the built-in feature both support the <title> tag, but not the title attribute.

https://github.com/mozilla/firefox-translations/blob/03fce47a662b7878eccf50c5bd912f68a42d3cc2/extension/mediator.js#L330

https://searchfox.org/mozilla-central/rev/f03ce34731ff8d1f6c77d8241c41d10df13e9aca/toolkit/components/translations/content/translations-engine.sys.mjs#231

Comment 8

[Andy]

This is a great new feature!

Is there a reason why attribute translation was tracked per attribute? There’s a whole bunch of attributes that are exposed to the user. Most importantly those that are exposed via the accessibility API.

A question whether ARIA attributes should be translated, led me to creating an—as far as I know—exhaustive list based on the standards.

Leaving out OpenGraph meta data, which is most likely not exposed to the user:

• alt attributes and title, also for <iframes> or <style> elements
• label for video tracks
• placeholder and pattern attributes if the format needs localization
• value of inputs if not user-defined
• Human-readable ARIA attributes aria-description, aria-label, aria-placeholder, aria-roledescription, aria-valuetext, aria-braillelabel
• And also text added by CSS’s pseudo-elements like ::before

Would it make sense to track the whole bunch here?

Thanks!

Comment 10

[Greg Tatum [:gregtatum] (REO Fx132)]

^ These are also important. I suspect it will be simple to just add a list of the ones we want to support in a single patch, but if it's complicated we can break it up into multiple patches or multiple bugs.

I'm going to mark this as a mentored bug since it's a self-contained problem. It's good work for someone who likes algorithms. This work will happen in the TranslationsDocument. The method determineTranslationStatus will need to know about attributes.

The attribute will need to be sent to the translation engine separately, and the translated node will need to be updated. Care will need to be taken to also include cloned nodes, as inline elements can be re-located and cloned in the translation process.

A good first test for this would be:

add_task(async function test_translated_title() {
  const { translate, htmlMatches, cleanup } = await createDoc(/* html */ `
    <div title="The title is translated" class="do-not-translate-this">
      Inner text is translated.
    </div>
  `);

  translate();

  await htmlMatches(
    "Language matching of elements behaves as expected.",
    /* html */ `
    <div title="THE TITLE IS TRANSLATED" class="do-not-translate-this">
      INNER TEXT IS TRANSLATED.
    </div>
    `
  );

  cleanup();
});

And more cases can be added. This would go in browser_translations_translation_document.js

Comment 12

[anayocrescent2]

i am working on this bug, would appreciate if i could get a further help on it

Comment 13

[Meera Murthy]

Hi,

I am from Outreachy program. I am working on this issue.
I am trying to reproduce this issue on the Firefox codebase on my local system but not successful yet. I am going through the code using searchfox but I wanted to know if there is any design document about how the translation process works that I can look into?
The tag on this bug says "access", does it mean "accessibility feature" or is it some kind of bug category that I need to know about?

Thanks,
Meera

Comment 14

[Meera Murthy]

(In reply to Meera Murthy from comment #13)

Hi,

I am from Outreachy program. I am working on this issue.
I am trying to reproduce this issue on the Firefox codebase on my local system but not successful yet. I am going through the code using searchfox but I wanted to know if there is any design document about how the translation process works that I can look into?
The tag on this bug says "access", does it mean "accessibility feature" or is it some kind of bug category that I need to know about?

Thanks,
Meera

I did reproduce this in the Firefox browser with some website. I translated the website from English to French, a placeholder that said "Enter mobile number" was not translated to French.
I also saw it happen in website link mentioned in the Github issue too.

Comment 15

[Meera Murthy]

Hi All,

I have been going through the translation document implementation in SearchFox and wanted share my thoughts. The code seems to be well documented and organized, it was very helpful.
Please correct me wherever I am wrong in my understanding and do provide suggestions or guidance if you have.

I see that the translations are only applied to inner text of an HTML element. Since placeholder is an attribute, it is not translated. Placeholders can have important information for the user (like example inputs to guide the users should be in accessible language) hence it has to be translated. Apart from the placeholder we need to find out other attributes that need to be translated to support accessibility.

The node whose attributes are to be translated should be added to the queuedNodes (for translation).

submitTranslation() translates only innerText/Html currently. The implementation should be extended to translate attributes like “placeholder” as well. We can discuss the list of attributes that need to be supported.

Similarly function determineTranslationStatus, updateNodesWithTranslations need to be updated to support attributes translation.

A TranslationFunction in "Translations-Engine" needs to be defined for the attributes that need to be translated.

There is seems to be special code to translate “title” tag separately here. May be we can add the title node to the queuedNodes (for translation) in the Translations-Document consistent with other nodes to be translated.

I just wanted to check if I am on the right path, so that I have all the understanding and design for feature addition before I start making changes in the code and test.
I’ll continue exploring the code and come up with the plan, work-flow and the list of files to be updated to add the new “attribute-translation” feature.

Thanks,
Meera Murthy
(Outreachy)

Comment 16

[Erik Nordin [:nordzilla]]

Hi Meera,

Thank you for the great investigative work into this bug. I would say you're definitely getting an overall idea of the code and what needs to be done.

My suggestion would be to not focus just yet on all the attributes that may need to be translated, but start with seeing if you can just get title to translate with a passing test by following the advice in this comment:

https://bugzilla.mozilla.org/show_bug.cgi?id=1819205#c10

If you can get title working, that would be an excellent first step. Then you can look into other attributes, and the algorithm can always be revised to be more generic before a final submission.

Comment 17

[Greg Tatum [:gregtatum] (REO Fx132)]

The tag on this bug says "access", does it mean "accessibility feature" or is it some kind of bug category that I need to know about?

access does mean it's an accessibility feature. Many terms that need translating are stored as attributes, and are exposed to the accessibility tree.

For design docs we have: https://firefox-source-docs.mozilla.org/toolkit/components/translations/index.html

But as you've found, most of the documentation is inline. We typically add comments to files and to classes to explain why they exist.

Comment 18

[anayocrescent2]

since many people are working on this bug, i think i should be sharing my progress so far so it can help me learn on contributing to open source and getting the right help.
and also help anyone working on it

Comment 19

[Meera Murthy]

Hi Everyone,

I have been running the code on my local machine to understand the workflow of translation and figuring out the exact code updates to be made. I am sharing my brainstormed ideas, sorry if I sound chaotic.

Seems like the Bergamot itself translates only innerText and innerHTML, I am not able to find any code or documentation to understand Bergamot APIs. I did not find or understand any useful information in bergamot-translator.js file.

In Transition-Engine there is this parameters isHTML, how does Bergamot use this information?
I am guessing that Bergamot gets the text styling information from tags like <strong/> <code/> from the input HTML string to be translated.
So if Bergamot is able to parse the html to get to the innerText from the HTML input, can it also parse to get attribute values like “title”, “placeholder” and translate their value? If yes can I update the Bergamot code to identify these attributes?

Your suggestion in comment 10 : The attribute will need to be sent to the translation engine separately, and the translated node will need to be updated. Care will need to be taken to also include cloned nodes, as inline elements can be re-located and cloned in the translation process.
Does this mean we should send the attribute values of “title”, “placeholder” to Bergamot through Translation-Engine as a NODE_TEXT? So these nodes should be separately queued, then make a translate-request to Bergamot and then update them into translated nodes? So that will be 2 separate calls to Bergamot?

I have anyways started to work on the suggestion given comment 10. I am a little slow since I am still getting accustomed to the codebase and the design.

Any thoughts or feedback is appreciated.

Cheers,
Meera

Comment 20

[Meera Murthy]

Hi Everyone,

    I have a solution that is working. I have code for "title" attribute for now, after review we can add other attributes like "placeholder".

I shall update the code on latest code and submit for revision. I have to do this using phabricator right? I did not use the phabricator token while setting up the codebase, I shall do that now. Please give me some time to submit.

Cheers,
Meera
Outreachy

Comment 21

[Meera Murthy]

Attachment: WIP: Bug 1819205:Add nodes to Translate Attributes: title,Translating title attribute as a text instead of HTML, Update the attribute of the node with translated text

Comment 22

[Meera Murthy]

Attachment: WIP: Bug 1819205:Add nodes to Translate Attributes: title, after running mach lint analysis

Depends on D191028

Comment 23

[Greg Tatum [:gregtatum] (REO Fx132)]

The Bergamot APIs aren't documented very well. The bindings are generated here: https://github.com/browsermt/bergamot-translator/tree/main/wasm/bindings

I learned them by running the JS debugger to inspect the APIs at runtime, and then reading through the bindings that I linked above.

Whenever HTML is requested Bergamot will parse the text as HTML before it sends it to be translated. This is why for attributes, we'll want to handle the processing ourselves, and submit them as text only.

When iterating over the nodes you can check for the attributes that we are interested in, or create a selector to find them separately. I'm not sure on the performance differences between the two approaches.

Comment 24

[Meera Murthy]

(In reply to Greg Tatum [:gregtatum] from comment #23)

The Bergamot APIs aren't documented very well. The bindings are generated here: https://github.com/browsermt/bergamot-translator/tree/main/wasm/bindings

I learned them by running the JS debugger to inspect the APIs at runtime, and then reading through the bindings that I linked above.

Whenever HTML is requested Bergamot will parse the text as HTML before it sends it to be translated. This is why for attributes, we'll want to handle the processing ourselves, and submit them as text only.

When iterating over the nodes you can check for the attributes that we are interested in, or create a selector to find them separately. I'm not sure on the performance differences between the two approaches.

I have submitted the solution a few days back and requested you for the review, hope you got the notification: https://phabricator.services.mozilla.com/D191028. Let me know if I have missed something in the process, I am new to Phabricator.

About the solution: In determineTranslationStatus I am looking for "title" attribute and when I find I am marking it as READY_TO_TRANSLATE.
While submitting to Translation, I find the "title" attribute value and sending it as a TEXT to the translator.
When translation is ready I am assigning the translated text to the "title" attribute value in the translated nodes.
So I suppose it is on the same lines as what you just messaged. Please review and confirm the solution. I shall add other attributes after that.

Cheers,
Meera

Comment 25

[Meera Murthy]

(In reply to Meera Murthy from comment #24)

(In reply to Greg Tatum [:gregtatum] from comment #23)

The Bergamot APIs aren't documented very well. The bindings are generated here: https://github.com/browsermt/bergamot-translator/tree/main/wasm/bindings

I learned them by running the JS debugger to inspect the APIs at runtime, and then reading through the bindings that I linked above.

Whenever HTML is requested Bergamot will parse the text as HTML before it sends it to be translated. This is why for attributes, we'll want to handle the processing ourselves, and submit them as text only.

When iterating over the nodes you can check for the attributes that we are interested in, or create a selector to find them separately. I'm not sure on the performance differences between the two approaches.

I have submitted the solution a few days back and requested you for the review, hope you got the notification: https://phabricator.services.mozilla.com/D191028. Let me know if I have missed something in the process, I am new to Phabricator.

About the solution: In determineTranslationStatus I am looking for "title" attribute and when I find I am marking it as READY_TO_TRANSLATE.
While submitting to Translation, I find the "title" attribute value and sending it as a TEXT to the translator.
When translation is ready I am assigning the translated text to the "title" attribute value in the translated nodes.
So I suppose it is on the same lines as what you just messaged. Please review and confirm the solution. I shall add other attributes after that.

Cheers,
Meera

Cont'd...
I think performance wise the solution should be fine, since I have not added any more iterations over the nodes. I am trying to find and translate the attributes within the existing iterating functions. I have added only a couple of "if conditions" which may not affect the performance that much. Do let me know what you think.
Looking forward for the review!

Comment 26

[Greg Tatum [:gregtatum] (REO Fx132)]

I wrote some tests that will need to be passed, and I'll leave some feedback on the review.

It can be run with:

./mach mochitest toolkit/components/translations/tests/browser/browser_translations_translation_document.js --headless

add_task(async function test_title_attribute() {
  const { translate, htmlMatches, cleanup } = await createDoc(/* html */ `
    <label title="Titles are user visible">Enter information:</label>
  `);

  translate();

  await htmlMatches(
    "Titles are translated",
    /* html */ `
      <label title="TITLES ARE USER VISIBLE">ENTER INFORMATION:</label>
    `
  );

  cleanup();
});

add_task(async function test_title_attribute_subnodes() {
  const { translate, htmlMatches, cleanup } = await createDoc(/* html */ `
    <div>
      <span title="Title 1"></span>
      <span title="Title 2"></span>
      <span title="Title 3"></span>
      <span title="Title 4"></span>
      <span title="Title 5"></span>
      This is text.
    </div>
  `);

  translate();

  await htmlMatches(
    "Titles are translated",
    /* html */ `
      <div>
        <span title="TITLE 1" data-moz-translations-id="0"></span>
        <span title="TITLE 2" data-moz-translations-id="1"></span>
        <span title="TITLE 3" data-moz-translations-id="2"></span>
        <span title="TITLE 4" data-moz-translations-id="3"></span>
        <span title="TITLE 5" data-moz-translations-id="4"></span>
        THIS IS TEXT.
      </div>
    `
  );

  cleanup();
});

You can verify the fix afterwards also by running all of the translations tests:

./mach mochitest toolkit/components/translations/tests browser/components/translations/tests --headless

Comment 27

[Greg Tatum [:gregtatum] (REO Fx132)]

Ah, I also listed a test above.

Comment 28

[Meera Murthy]

Attachment: WIP: Bug Bug 1819205 - [devtools] Implement feature translate attributes such as title and placeholder. Erik Nordin, Greg Tatum!

Depends on D191088

Comment 29

[Meera Murthy]

(In reply to Greg Tatum [:gregtatum] from comment #26)

I wrote some tests that will need to be passed, and I'll leave some feedback on the review.

It can be run with:

./mach mochitest toolkit/components/translations/tests/browser/browser_translations_translation_document.js --headless

add_task(async function test_title_attribute() {
  const { translate, htmlMatches, cleanup } = await createDoc(/* html */ `
    <label title="Titles are user visible">Enter information:</label>
  `);

  translate();

  await htmlMatches(
    "Titles are translated",
    /* html */ `
      <label title="TITLES ARE USER VISIBLE">ENTER INFORMATION:</label>
    `
  );

  cleanup();
});

add_task(async function test_title_attribute_subnodes() {
  const { translate, htmlMatches, cleanup } = await createDoc(/* html */ `
    <div>
      <span title="Title 1"></span>
      <span title="Title 2"></span>
      <span title="Title 3"></span>
      <span title="Title 4"></span>
      <span title="Title 5"></span>
      This is text.
    </div>
  `);

  translate();

  await htmlMatches(
    "Titles are translated",
    /* html */ `
      <div>
        <span title="TITLE 1" data-moz-translations-id="0"></span>
        <span title="TITLE 2" data-moz-translations-id="1"></span>
        <span title="TITLE 3" data-moz-translations-id="2"></span>
        <span title="TITLE 4" data-moz-translations-id="3"></span>
        <span title="TITLE 5" data-moz-translations-id="4"></span>
        THIS IS TEXT.
      </div>
    `
  );

  cleanup();
});

You can verify the fix afterwards also by running all of the translations tests:

./mach mochitest toolkit/components/translations/tests browser/components/translations/tests --headless

Thank you for the review!

About your question: What happens when the node has text as well as a title?

Yes, I did notice a similar issue but that was regarding multiple attributes to be translated in the same element.
I have updated the code to support multiple attributes: “title” “placeholder” for now. I think the new implementation will also address the issue that you pointed out.
Now the translation of the content of the node is not dropped while translating the attributes, I have called the translation separately for each attribute. And I suppose this could affect the performance to some extent.

Before submitting for review I have some question about the test cases:
I did run the test cases with: ./mach mochitest toolkit/components/translations/tests/browser/browser_translations_translation_document.js –headless in following cases:

• Without the new implementation of attribute translation: One testcase fails saying: "FAIL Placeholders support needs to be added"
• After the implementation of attribute translation, all existing test cases pass.

I am having problem when I add the new test cases that you have given in the comment into browser_translations_translation_document.js.
I get this Fail message: “FAIL Titles are translated”.
I have added translation of title and placeholders. I am not understanding why it says FAIL when we translate “title” attribute.

I shall submit the code for review after clearing this doubt, the new code is already on Pharbricator if you want to check.

Sorry about adding names in the code, I was supposed to remove it before submitting.
Meera

Comment 30

[Greg Tatum [:gregtatum] (REO Fx132)]

You can run the tests in the following order:

1. Run the tests on the main branch without any changes and verify they work.
2. Run the tests on on your branch of the code, and they should fail with the message "Placeholders support needs to be added", since you added placeholder support.
3. Apply my test case to your code. Run the code.

If step 3 fails, then you still have some additional logic to figure out on getting your code to be correct for different situations. At this point I think you will need to create a separate queue of work, or teach the existing queue to know about node translations with an attribute.

Comment 31

[Erik Nordin [:nordzilla]]

Attachment: revisions.png

Hi Meera,

It seems like you're getting close with this bug!

I noticed that it still seems like your abandoned revisions are linked to the current revision (see attached image).

You will need to go into Phabricator, on the right side, and click "Edit related revisions..." and unlink the revisions from their parent/child relationships.

Then, locally, make sure you merge all of your code changes into one single revision that you push up to Phabricator. We have to make sure that all the code changes are in one single patch without being linked to previous abandoned revisions.

Let me know if you get stuck with this process.

Comment 32

[Meera Murthy]

Hi Erik,

I tried to unlink the revisions, please check and confirm if it is fine now.

Before I update and push the code I have to solve this problem from the review feedback right? You pointed out that: I think this will ignore nodes that need to be subdivided further. I think attributes will need a separate translation queue to work through. Sometimes a node will need to be subdivided more AND have a placeholder that is ready for translations.

I am trying to add a test case for this scenario, it is just a random HTML to test the nested elements with attributes to be translated to test the issue you pointed out.
This is the test case I added:

add_task(async function test_attributes() {
  const { translate, htmlMatches, cleanup } = await createDoc(/* html */ `
    <div title="Titles are user visible">
      This is the outer div
      <label>
        Enter information:
        <input type="text" placeholder="This is a placeholder">
      </label>
    </div>
  `);

  translate();

  await htmlMatches(
    "new test case",
    /* html */ `
      <div title="TITLES ARE USER VISIBLE">
      THIS IS THE OUTER DIV
      <label>
      ENTER INFORMATION:
        <input type="text" placeholder="THIS IS A PLACEHOLDER">
      </label>
    </div>
    `
  );

  cleanup();
});
`

First I tried to test without the new implementation for attribute translations, I did update the test case to not translate attributes for this. After testing for some reason I get this output:
Actual HTML:

<div title="Titles are user visible">
  THIS IS THE OUTER DIV
  <label data-moz-translations-id="0">
    ENTER INFORMATION:
    <input type="text" placeholder="This is a placeholder" data-moz-translations-id="1">
    </label>
  </div>

It is not matching because of data-moz-translations-id="1", why is this showing here. I did not see this with other test cases.

Do you have any idea why this is happening?

I am working on the review feedback: "... Sometimes a node will need to be subdivided more AND have a placeholder that is ready for translations..." after that I will push the code.

Thanks!
Meera

Comment 33

[Erik Nordin [:nordzilla]]

Hi Meera,

Yes, it looks like your most current revision was unlinked from the abandoned revisions.

Regarding your question, we do insert a data-moz-translations-id into the translated version of HTML.

See this test case, for example:

https://searchfox.org/mozilla-central/rev/ffdc4971dc18e1141cb2a90c2b0b776365650270/toolkit/components/translations/tests/browser/browser_translations_translation_document.js#470-526

Notice that we have:

<label data-moz-translations-id="0">

and

<input type="text" placeholder="This is a placeholder" data-moz-translations-id="1">

I would expect that once your code is fully working, the title itself will receive the id of 0, and these following two will be bumped to 1 and 2 respectively.

Comment 34

[Erik Nordin [:nordzilla]]

Note that it's a little bit confusing.

We have a heuristic that divides up the DOM tree into subtrees.

https://searchfox.org/mozilla-central/source/toolkit/components/translations/content/translations-document.sys.mjs#1279-1373

It can be a little awkward at times, but it mostly works. One weird case is that it sees a <div> of <span> elements as a single tree with multiple elements that need to be matched by their Id, but it sees a <div> of <div> as a bunch of individual subtrees, each with one element, as such they don't need matching and don't receive an Id:

https://searchfox.org/mozilla-central/rev/ffdc4971dc18e1141cb2a90c2b0b776365650270/toolkit/components/translations/tests/browser/browser_translations_translation_document.js#470-590

This is why the <div> of <span> expected HTML has assigned Id's, but the <div> of <div> expected HTML has none.

So in short, the translated HTML itself will assign Id's based on the heuristic above. Whatever it gives you with regard to assigned Id's is effectively going to be the "correct" HTML to match. We just have to ensure that the title is being translated as expected.

Comment 35

[Meera Murthy]

Attachment: Bug 1819205 [translations] Translate Attributes "title" and "placeholder". r=gregtatum!

Comment 36

[Meera Murthy]

Hi Erik,
I have submitted new changes for review.

Hope I can continue working on this bug irrespective of the Outreachy internship results. I want to resolve this bug completely and close the bug.

To test the new code I have added the test cases given by Greg in the previous comments in this thread. All these test cases are passing.
I have added a few more test cases too.

Actually I want to continue contributing to Mozilla even after Outreachy. I want to know if there is any process I need to follow to continue working on new bugs and issues later.

Cheers,
Meera

Comment 37

[Erik Nordin [:nordzilla]]

Hi Meera,

I don't think your most recent patch actually got submitted for review.

It still says WIP: Bug...

It will be uploaded as a WIP patch unless you have the r=nordzilla,... in the title, or unless you explicitly use the --no-wip flag when submitting in moz-phab.

Even though it looks like you've tagged me and Greg as reviewers, it will not show up in our review queues unless it is taken out of its WIP state.

I believe that you can do this in Phabricator, in the Add Action... dropdown at the bottom.

But it will also get taken out of WIP if you resubmit with r= in the title, too.

I will try to get to this tomorrow. I missed it when I went through all of my Outreachy reviews today.

We would all love it if you kept working on, and finished, this bug independent of any Outreachy outcomes.

Thank you!

Comment 38

[Meera Murthy]

Hi Erik,

I have submitted the changes for review, I think it is out of WIP now. Let me know if you can see it in the review queue.

Cheers,
Meera

Comment 39

[Meera Murthy]

Hi,
I wrote a few test cases to test the new updated code to translate attribute: title and placeholder. Please find the code changes and test cases in the Phabricator, I have submitted it for review.
The test cases given in comment 26 and comment 10 by Greg are also added and all of them are passing.

One of the test cases that I added is failing.

This is the input HTML for the failing test case:

<div title="Titles are user visible">
      This is the outer div
      <label>
        Enter information:
        <input type="text" placeholder="This is a placeholder">
      </label>
    </div>

I get this as output HTML:

<div title="TITLES ARE USER VISIBLE">
   THIS IS THE OUTER DIV
   <label data-moz-translations-id="0">
     ENTER INFORMATION:
     <input type="text" placeholder="This is a placeholder" data-moz-translations-id="1">
     </label>
   </div>

Attribute in the outer HTML is translating but the attribute in the nested HTML is not translating.
I see that the fakeTranslator is translating the innerText child nodes like this:

for (const childNode of node.childNodes) {
        upperCaseNode(childNode);
      }

So the placeholder in this test case is not translating since in translations-document.sys.mjs the parent node <label> is added to the queue to translate, then it does not check if the childNodes have innerHTML to be translated. According to the new code I am checking for attributes while checking for innerHTML to be translated. The new code is translating attribute: title and placeholder in many scenarios, this test case with <input> in nested <label> HTML is failing.

So I have to find the code where the node added to the translation queue is iterated for childnodes looking for innerHTML to be translated, because I need to check for attributes to be translated in those childnodes.
I expected to find this code in translations-engine.sys.mjs

I am investigating this, but if you have any pointers please let me know. Also getting that review will really help!

Thanks!
Meera

Comment 40

[Erik Nordin [:nordzilla]]

Thanks so much for the update, Meera.

I just pulled up your patch a bit last night, and I was going to continue looking at it this morning.

I'll review your latest updates.

Comment 41

[Erik Nordin [:nordzilla]]

Hi Meera,

It looks like you've got two open revisions on this bug currently.

Could you please abandon one of them in Phabricator and continue working from only the single revision that you kept?

I'd probably keep the most recent one with the latest review feedback.

The abandon option is at the bottom of Phabricator in the "Add Action..." menu.

Thanks!

Comment 42

[Meera Murthy]

Hi Erik,

I have abandoned the old review in the Phabricator. I shall revise on the latest review only. Hope I have done it correctly, I am still wrapping my head around the processes.

Thanks a lot for the detailed review!!!

I do have some doubts:

---

In the review comments you mentioned:

...In the code above, we are dealing with a case where the heuristic has decided to treat a nested structure as a single translatable group.
This is why we iterate through and assign the translationsId to each element in the subtree.
Your code, later in the function, calls node.setAttribute(...) which is why the title attribute on the outermost <div> element is being translated, but the inner elements of the subtree are not.
You would have to modify that iteration at the top where el.dataset.mozTranslationsId = i; happens and schedule attribute translates on each el as well, to fix this....

So I made these changes and all the test cases are passing now.

async submitAttrTranslation(node) {
    // Give each element an id that gets passed through the translation so it can be
    // reunited later on.
    if (node.nodeType === Node.ELEMENT_NODE) {
      node.querySelectorAll("*").forEach((el, i) => {
        el.dataset.mozTranslationsId = i;
        if (el.hasAttribute("title") || el.hasAttribute("placeholder")) {
          this.submitAttrTranslation(el);
        }
      });
    }

Then you said

However, I don't think this would be a great direction to continue down, which I will explain in the next section.
I experimented quite extensively with this code today. First, I fixed your code above such that all the test cases passed.
Next, I tried reimplementing it while keeping a single queue, instead of having separate queues and such for the attribute translations. I got this solution to work too, but it made the node subdividing algorithm a lot messier, and added complexity to how the queued translations worked, as well as to the observer.
So I had a chat with Greg and I discussed with him the tradeoffs that I saw when comparing having separate queues for attributes, vs. trying to "logic everything together."
I'm happy to say that I think we've settled on a workable solution.
Ultimately, we're going to keep the idea of having separate queues and separate observers for attribute translations, but we're not going to mess with the existing node subdivision algorithm.
Instead, we're going to build out a separate attribute translation system that utilizes querySelectorAll() function to retrieve all nodes with the attributes that we want, so querySelectorAll('[title]') for example.
This translations system will just run alongside the other one. So the document will continue to do the subdivision algorithm, and it will query-select for the translatable attributes.
Everything will be in separate queues, with separate observers, even though some nodes may exist in both queues for text-content translation and attribute translation. That's fine.

I didn’t quite understand this part. Are you proposing a re-design of the solution?
I do have a separate queue for Attribute translations, but when you say “…build out a separate attribute translation system that utilizes querySelectorAll() function to retrieve all nodes with the attributes that we want…”,
does that mean we just use querySelectorAll() to get all the nodes with attributes all at ones (of the whole page/document), iterate through the list of nodes, get the required attributes, translate and set those attributes in the document with the translations in one go?

---

Rebase on top of the latest from central, and resolve any conflicts from Greg's recent patch.

Does “hg update -C central” get the latest code from central?
I want to make sure I have the right base code. I am working in the directory c:/mozilla-source/mozilla-unified

---

This new observer will only observe changes in attributes for the nodes which had attributes to translate.

Yes, I shall add the implementation with separate observer for nodes which have attribute translation.

---

after you finish this bug, it would be great if you wanted to be the person to file a follow-up bug for the Shadow DOM work, explaining the context etc.

Yes I can do that!

---

Hope I am not getting too confused and working in sync with you.
Cheers,
Meera

Comment 43

[Meera Murthy]

(In reply to Meera Murthy from comment #42)

Hi Erik,

I have abandoned the old review in the Phabricator. I shall revise on the latest review only. Hope I have done it correctly, I am still wrapping my head around the processes.

Thanks a lot for the detailed review!!!

I do have some doubts:

---

In the review comments you mentioned:

...In the code above, we are dealing with a case where the heuristic has decided to treat a nested structure as a single translatable group.
This is why we iterate through and assign the translationsId to each element in the subtree.
Your code, later in the function, calls node.setAttribute(...) which is why the title attribute on the outermost <div> element is being translated, but the inner elements of the subtree are not.
You would have to modify that iteration at the top where el.dataset.mozTranslationsId = i; happens and schedule attribute translates on each el as well, to fix this....

So I made these changes and all the test cases are passing now.

async submitAttrTranslation(node) {
    // Give each element an id that gets passed through the translation so it can be
    // reunited later on.
    if (node.nodeType === Node.ELEMENT_NODE) {
      node.querySelectorAll("*").forEach((el, i) => {
        el.dataset.mozTranslationsId = i;
        if (el.hasAttribute("title") || el.hasAttribute("placeholder")) {
          this.submitAttrTranslation(el);
        }
      });
    }

Then you said

However, I don't think this would be a great direction to continue down, which I will explain in the next section.
I experimented quite extensively with this code today. First, I fixed your code above such that all the test cases passed.
Next, I tried reimplementing it while keeping a single queue, instead of having separate queues and such for the attribute translations. I got this solution to work too, but it made the node subdividing algorithm a lot messier, and added complexity to how the queued translations worked, as well as to the observer.
So I had a chat with Greg and I discussed with him the tradeoffs that I saw when comparing having separate queues for attributes, vs. trying to "logic everything together."
I'm happy to say that I think we've settled on a workable solution.
Ultimately, we're going to keep the idea of having separate queues and separate observers for attribute translations, but we're not going to mess with the existing node subdivision algorithm.
Instead, we're going to build out a separate attribute translation system that utilizes querySelectorAll() function to retrieve all nodes with the attributes that we want, so querySelectorAll('[title]') for example.
This translations system will just run alongside the other one. So the document will continue to do the subdivision algorithm, and it will query-select for the translatable attributes.
Everything will be in separate queues, with separate observers, even though some nodes may exist in both queues for text-content translation and attribute translation. That's fine.

I didn’t quite understand this part. Are you proposing a re-design of the solution?
I do have a separate queue for Attribute translations, but when you say “…build out a separate attribute translation system that utilizes querySelectorAll() function to retrieve all nodes with the attributes that we want…”,
does that mean we just use querySelectorAll() to get all the nodes with attributes all at ones (of the whole page/document), iterate through the list of nodes, get the required attributes, translate and set those attributes in the document with the translations in one go?

---

Rebase on top of the latest from central, and resolve any conflicts from Greg's recent patch.

Does “hg update -C central” get the latest code from central?
I want to make sure I have the right base code. I am working in the directory c:/mozilla-source/mozilla-unified

---

This new observer will only observe changes in attributes for the nodes which had attributes to translate.

Yes, I shall add the implementation with separate observer for nodes which have attribute translation.

---

after you finish this bug, it would be great if you wanted to be the person to file a follow-up bug for the Shadow DOM work, explaining the context etc.

Yes I can do that!

---

Hope I am not getting too confused and working in sync with you.
Cheers,
Meera

Rebase on top of the latest from central, and resolve any conflicts from Greg's recent patch.

I am looking through the latest code from central, there seems to be a lot of updates from the Greg's latest patch. I am seeing if it affects the implementation for this bug.
Anyway I want to know your thoughts about my previous message on this thread to make sure we are on the same page.

Comment 44

[Erik Nordin [:nordzilla]]

Hi Meera,

Regarding your questions:

---

"does that mean we just use querySelectorAll() to get all the nodes with attributes all at ones (of the whole page/document), iterate through the list of nodes, get the required attributes, translate and set those attributes in the document with the translations in one go?"

Yes, that is the approach we want to take here, rather than trying to gather all of the nodes in the tree walking process, we can just select them and have it be a separate step of the translations algorithm that works alongside the tree-walking solution.

---

"Does “hg update -C central” get the latest code from central?"

I'm really not sure about this one. I use git, not Mercurial, and I've never used Mercurial.

I did a quick Google search and found this article. I don't know how accurate it is.

In fact, I believe as of November 6, it is now public information that Mozilla is moving entirely from supporting both Mercurial and git, to only supporting git.

This process will take some time, however, so if you really like Mercurial you can certainly keep using it for the time being.

---

I hope that answers all of the open questions!

Comment 45

[Meera Murthy]

Hi Erik,

Thanks for the reply.
Yes now I am using GIT only.

I am updating the latest code with Greg's recent patch. As suggested by you, I am using querySelectorAll() function to retrieve all nodes with the attributes that we want into a separate queue for translation.

I have one doubt though, about a separate observer as you suggested:

Everything will be in separate queues, with separate observers, even though some nodes may exist in both queues for text-content translation and attribute translation. That's fine. This new observer will only observe changes in attributes for the nodes which had attributes to translate.

While making updating the code I though that using the existing observer makes more sense instead of a separate one. I have to add options for attributes like this:
const MUTATION_OBSERVER_OPTIONS = {
...,
attributes: true,
attributeFilter: ["title", "placeholder"],
}

and then in MutationObserver I am checking for attributes addition/changes. If the observer detects addition of any element, it can have attributes we are looking for, so we can handle translation of the new element as well as the translation of the attributes. As usual I use querySelectorAll() function to retrieve all nodes with the attributes that we want into a separate queue for Attribute translation. Translation of innerHTML will continue as usual with subdivideNodeForTranslations.

I am still in the process of writing test cases and testing though. I just want to check with you on the design part.
Let me know what you think.

Cheers,
Meera

Comment 46

[Erik Nordin [:nordzilla]]

Hi Meera,

I think I'd have to see the code and spin it up locally to give you an informed and detailed answer.

Feel free to update your Phabricator patch in a WIP state. That's the easiest way to share the context of patches with other contributors.

Then I can run the following command locally:

moz-phab patch <your_revision_id>

And then it will check out your revision as it exists in Phabricator for me to look at things in my own setup.

Comment 47

[Meera Murthy]

Attachment: Bug 1819205 [translations] Translate Attributes using querySelectorAll. Add Observer for attributes. r=nordzilla!

Comment 48

[Meera Murthy]

Hi Erik,

     I have submitted the new changes. 

1. Using querySelectorAll() function to retrieve all nodes with the attributes into a separate queue for translation.
2. As mentioned earlier I am using the existing observer instead of a separate one because we need to check for attributes in every new node that is added. I have to add options for attributes in the MUTATION_OBSERVER_OPTIONS.
3. I have added test cases to test the observer for adding/updating attributes.
4. I have updated on the latest code with Greg's recent patch.

All the test cases are passing. Please let me know if there are any more scenarios to be tested.

I have submitted using GIT for the first time, hope everything is fine.

Cheers,
Meera

Comment 49

[Erik Nordin [:nordzilla]]

Thanks for submitting these changes!

I think it should be fine to use a single observer, probably cleaner too.

---

There are now two open revisions. Could you please abandon this new revision, and continue modifying the existing revision that has all of the review feedback on it? I'd like to keep everything in the same revision to preserve the history.

I realize that there was probably a bit of confusion from the exceptional situation in which you were switching from Mercurial to git.

The way that you can make a commit track to a specific revision is by editing the commit message to point at that revision specifically.

Example

commit 722a12e33758b237ce6b9fc5ff201711a1ca277c (HEAD -> phab-D194459)
Author: Meera Murthy <[e-mail]>
Date:   Thu Nov 23 02:00:30 2023 -0600

    Bug 1819205 [translations] Translate Attributes using querySelectorAll. Add Observer for attributes. r=nordzilla!
    
    Differential Revision: https://phabricator.services.mozilla.com/D192217

You will simply edit your commit message by using git commit --amend to add that bottom line, pointing it to the URL of the revision that has all of the review feedback (D192217), then re-submit with moz-phab.

For future reference, you could have added this bottom line before submitting the first time, and it would have updated that revision instead of creating a new one. The only difference right now is that we have to abandon this new one as well.

I don't expect you to have known how to do this, though, so no worries there!

---

Once you get these newest changes into the correct revision, I've got some review feedback for you.

Thank you for your work on this!

Comment 50

[Meera Murthy]

Hi Erik,

I'm sorry for not updating the right revision, I should have known better by now. I shall take care in the future.

I have abandoned D194459.
I have updated D192217 with latest revision.

I am looking forward for the feedback, I do think my solution in mutation observer can be better.

Thanks,
Meera

Comment 51

[Meera Murthy]

Attachment: Bug 1819205 [translations] Add Observer for attributes. r=gregtatum!

Depends on D192217

Comment 52

[Meera Murthy]

Attachment: Bug 1819205 [translations] Refactor Pause Mutation Observer while updating the translations. r=gregtatum!

Depends on D196304

Comment 53

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d4c3d6e12ce6
[translations] Translate Attributes "title" and "placeholder". r=gregtatum,nordzilla,translations-reviewers
https://hg.mozilla.org/integration/autoland/rev/caac38b81151
[translations] Add Observer for attributes. r=gregtatum,translations-reviewers
https://hg.mozilla.org/integration/autoland/rev/7cfb5c675cce
[translations] Refactor Pause Mutation Observer while updating the translations. r=gregtatum,translations-reviewers

Comment 54

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/d4c3d6e12ce6
https://hg.mozilla.org/mozilla-central/rev/caac38b81151
https://hg.mozilla.org/mozilla-central/rev/7cfb5c675cce

Comment 55

[Marco Castelluccio [:marco]]

Congratulations Meera!

Comment 56

[Marco Castelluccio [:marco]]

Release Note Request (optional, but appreciated)
[Why is this notable]: We are now translating page titles and placeholders in web pages.
[Affects Firefox for Android]: Not yet.
[Suggested wording]: When translating web pages, we are now also translating text in tooltips (i.e. titles) and text displayed in form controls (i.e. placeholder).
[Links (documentation, blog post, etc)]: N/A.

Comment 57

[Pascal Chevrel:pascalc]

Note added to 123 nightly release notes in the Fixed section, thanks.

Comment 58

[Greg Tatum [:gregtatum] (REO Fx132)]

Some follow-ups:
Bug 1878710
Bug 1878712