Closed Bug 1919230 Opened 2 months ago Closed 2 months ago

Firefox 130 translates radio and checkbox input values despite of translate=no attribute

Categories

(Firefox :: Translations, defect)

Product:
Firefox
Component:
Translations
Version:
Firefox 130
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
132 Branch
Tracking Flags:
Tracking Status
firefox-esr115 --- unaffected
firefox-esr128 --- unaffected
firefox130 --- wontfix
firefox131 --- fixed
firefox132 --- fixed

People

(Reporter: cweiss, Assigned: nordzilla)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(4 files)

Bug 1919230 - Fix no-shadow lint warnings in translations-document.sys.mjs r=#translations-reviewers!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1919230 - Rework attribute translations r=#translations-reviewers
48 bytes, text/x-phabricator-request
Details | Review
Bug 1919230 - Fix no-shadow lint warnings in translations-document.sys.mjs
48 bytes, text/x-phabricator-request
phab-bot
: approval-mozilla-release+
Details | Review
Bug 1919230 - Rework attribute translations
48 bytes, text/x-phabricator-request
phab-bot
: approval-mozilla-release+
Details | Review

Steps to reproduce:

User agent string: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:130.0) Gecko/20100101 Firefox/130.0
Also happens with Windows version.

Here's a simple page with a test form:

<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8">
<title>Simple Form</title>
</head>
<body>
<h1>Simple POST Form to test potential Firefox translation bug</h1>
<form action="test.php" method="POST">
<label>
<input type="radio" name="option" value="option_one" translate="no">
This is option 1
</label><br>
<label>
<input type="radio" name="option" value="option_two" translate="no">
And this is option 2
</label><br>
<label>
<input type="checkbox" name="checkbox" value="checkbox_value" translate="no">
Let's also try with a checkbox
</label><br><br>
<input type="submit" value="Submit">
</form>
</body>
</html>

Use the "Translate page" function to translate from English to German, then submit the form.

Actual results:

Input values are translated/changed on submit:

"option_one" becomes "option-one" (notice the dash instead of underscore) and
"checkbox_value" becomes "Checkbox-Wert".

The translate="no" attribute on the inputs is entirely ignored. If I add the attribute to the <form> tag, at least the labels are not translated, but the values still are.

Expected results:

Checkbox and radio values should not be changed on submit.

The Bugbug bot thinks this bug should belong to the 'Firefox::Translations' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Component: Untriaged → Translations

Also the "Submit" button gets translated to "Untermit", which makes no sense. "Absenden" or "Abschicken" would be correct.

Hi Clemens Weiß, thanks for submitting this bug!

I do believe that this is a genuine defect in which we are respecting no-translate attributes with regard to a node's text content, but not with regard to its other translatable attributes.

I'll write a patch to fix this and add an automated test case to cover this behavior.

Thanks for taking the time to create an example HTML document. That was super helpful!

(In reply to Clemens Weiß from comment #2)

Also the "Submit" button gets translated to "Untermit", which makes no sense. "Absenden" or "Abschicken" would be correct.

Unfortunately this is beyond the scope of this patch as it relates to the quality of the output from the English-to-German machine-learning model.

We do hope to continue to iterate on the quality of our models as well as release newly trained language models. Any issues related to model translation quality should be reported here. We appreciate the feedback!

https://github.com/mozilla/firefox-translations-training

Assignee: nobody → enordin
Blocks: 1845772
Severity: -- → S3

Fixes areas within translations-document.sys.mjs that are not compatible
with the newly enable no-shadow rule for ESLint.

Ensures that nodes that should be excluded from translations due to
having attributes such as translate="no" or class="notranslate"
are excluded from attribute translation. Previously, we were excluding
them from translating their text content, but would still translate
other attributes such as value.

Depends on D222518

Attachment #9425379 - Attachment description: WIP: Bug 1919230 - Fix no-shadow lint warnings in translations-document.sys.mjs r=#translations-reviewers! → Bug 1919230 - Fix no-shadow lint warnings in translations-document.sys.mjs r=#translations-reviewers!
Attachment #9425380 - Attachment description: WIP: Bug 1919230 - Respect no-translate attributes for attribute translations r=#translations-reviewers → Bug 1919230 - Respect no-translate attributes for attribute translations r=#translations-reviewers

The bug has a release status flag that shows some version of Firefox is affected, thus it will be considered confirmed.

Status: UNCONFIRMED → NEW
Ever confirmed: true

Hi Erik Nordin, thanks for taking care of this!
In my opinion the value of these radio/checkbox inputs should never be translated, because they are not shown to the user. Instead they are defined by the website/application, which usually breaks if it receives an unexpected value.
I guess the same applies to select boxes...

In my opinion the value of these radio/checkbox inputs should never be translated, because they are not shown to the user. Instead they are defined by the website/application, which usually breaks if it receives an unexpected value.

Yeah I agree with this, and was an oversight on the original implementation.

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

In my opinion the value of these radio/checkbox inputs should never be translated, because they are not shown to the user. Instead they are defined by the website/application, which usually breaks if it receives an unexpected value.

Yeah I agree with this, and was an oversight on the original implementation.

Same, agreed!

I will modify the patches to ensure that translate="no" is respected, and that the [value] attribute is not translated ever for <input> elements.

(In reply to Erik Nordin [:nordzilla] from comment #9)

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

In my opinion the value of these radio/checkbox inputs should never be translated, because they are not shown to the user. Instead they are defined by the website/application, which usually breaks if it receives an unexpected value.

Yeah I agree with this, and was an oversight on the original implementation.

Same, agreed!

I will modify the patches to ensure that translate="no" is respected, and that the [value] attribute is not translated ever for <input> elements.

Okay, I've done some further investigation and come up with a plan.

Background

Bug 1878710 introduced translating all value attributes for <input> elements, so I am going to mark that as the regressor.

I believe the original intent for including <input> elements in this bug was to translate button text, which is completely valid. This did, however, introduce the side effect of translating value attributes for radio buttons and checkboxes as described by this bug.

Plan of Action

I audited all of the <input> types (MDN), and grouped them into three categories:

Button-like <input> types:

  • button: A push button with no default behavior.
  • image: A graphical submit button.
  • reset: A button that resets the contents of the form to default values.
  • submit: A button that submits the form.

Text-input-like <input> types:

  • datetime (Deprecated): A control for entering a date and time based on UTC time zone.
  • email: A field for editing an email address.
  • password: A single-line text field whose value is obscured.
  • search: A single-line text field for entering search strings.
  • tel: A control for entering a telephone number.
  • text: The default value; a single-line text field.
  • url: A field for entering a URL.

Other <input> types:

  • checkbox: A check box allowing single values to be selected/deselected.
  • color: A control for specifying a color.
  • date: A control for entering a date.
  • datetime-local: A control for entering a date and time.
  • file: A control that lets the user select a file.
  • hidden: A control that is not displayed but whose value is submitted to the server.
  • month: A control for entering a month and year.
  • number: A control for entering a number.
  • radio: A radio button for selecting a single value out of multiple choices.
  • range: A control for entering a number where the exact value is not important.
  • time: A control for entering a time value.
  • week: A control for entering a week-year number and a week number.

Ultimately, I think it's clear that we want to continue translating all of the button-like <input> types. button, reset, and submit all use the value attribute for their text, however the image type uses alt if the image cannot be displayed.

We could, in theory, translate default values for types such as text and search. This would only apply to the pre-populated value within the markup, and, in my testing, any manually inputted value does not get translated. However, populating default values would be more appropriate for the placeholder attribute, which we do translate.

The "other" group of input types should decidedly not be translated.

I will update this patch stack to

  1. Respect the no-translate rules for attribute translation (as I've already implemented)
  2. Only translate <input> elements that match the specified types.
Keywords: regression
Regressed by: 1878710

I mistagged this before. ESR-128 is not affected.

status-firefox-esr128: affected → unaffected

I agree with this plan. Thanks for writing it out.

Attachment #9425379 - Attachment description: Bug 1919230 - Fix no-shadow lint warnings in translations-document.sys.mjs r=#translations-reviewers! → WIP: Bug 1919230 - Fix no-shadow lint warnings in translations-document.sys.mjs r=#translations-reviewers!
Attachment #9425380 - Attachment description: Bug 1919230 - Respect no-translate attributes for attribute translations r=#translations-reviewers → WIP: Bug 1919230 - Respect no-translate attributes for attribute translations r=#translations-reviewers
Attachment #9425379 - Attachment description: WIP: Bug 1919230 - Fix no-shadow lint warnings in translations-document.sys.mjs r=#translations-reviewers! → Bug 1919230 - Fix no-shadow lint warnings in translations-document.sys.mjs r=#translations-reviewers!
Attachment #9425380 - Attachment description: WIP: Bug 1919230 - Respect no-translate attributes for attribute translations r=#translations-reviewers → Bug 1919230 - Respect no-translate attributes for attribute translations r=#translations-reviewers
Attachment #9425380 - Attachment description: Bug 1919230 - Respect no-translate attributes for attribute translations r=#translations-reviewers → Bug 1919230 - Rework attribute translations r=#translations-reviewers
Pushed by enordin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c7c79b33d74a Fix no-shadow lint warnings in translations-document.sys.mjs r=translations-reviewers,gregtatum https://hg.mozilla.org/integration/autoland/rev/fc9e1fc232d0 Rework attribute translations r=translations-reviewers,gregtatum

https://hg.mozilla.org/mozilla-central/rev/c7c79b33d74a
https://hg.mozilla.org/mozilla-central/rev/fc9e1fc232d0

Status: NEW → RESOLVED
Closed: 2 months ago
status-firefox132: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → 132 Branch
Duplicate of this bug: 1921990

Fixes areas within translations-document.sys.mjs that are not compatible
with the newly enable no-shadow rule for ESLint.

Original Revision: https://phabricator.services.mozilla.com/D222518

Attachment #9428549 - Flags: approval-mozilla-release?

Reworks the structures behind attribute translations, allowing us to further constrain
for which types of elements certain attributes are considered translatable.

This patch also improves the algorithm to fix the correctness of attribute translations
respecting class="notranslate" and translate="no" et al.

Original Revision: https://phabricator.services.mozilla.com/D222519
Depends on D222518

Depends on D224332

Attachment #9428550 - Flags: approval-mozilla-release?

release Uplift Approval Request

  • User impact if declined: The value attributes will be translated for all <input> elements. For button-like <input> elements this makes sense as it will translate the text on the button. For other <input> elements such as radio, this will translate the non-user-visible value that is sent to the database upon submit, which is not intended.
  • Code covered by automated testing: yes
  • Fix verified in Nightly: yes
  • Needs manual QE test: no
  • Steps to reproduce for manual QE testing: N/A
  • Risk associated with taking this patch: Low risk
  • Explanation of risk level: This patch is well tested in automation, and the fixes have been verified in the beta build by the contributors who reported the defect.
  • String changes made/needed: None
  • Is Android affected?: yes
Duplicate of this bug: 1923027
Attachment #9428549 - Flags: approval-mozilla-release? → approval-mozilla-release+
Attachment #9428550 - Flags: approval-mozilla-release? → approval-mozilla-release+
Duplicate of this bug: 1928321
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: