1809972 - Cookie banner CFR: code should not try to replace localized text when it can pass a variable

Status: RESOLVED FIXED

(Fenix :: Privacy, defect)

Description

[Francesco Lodolo [:flod]]

https://github.com/mozilla-mobile/firefox-android/blame/4af98f17650c9c3215b7c24b497b99357ab8dc78/focus-android/app/src/main/java/org/mozilla/focus/browser/integration/BrowserToolbarIntegration.kt#L337-L359

These are the strings in question

    <!-- Contextual Feature Recommendation Popups -->
    <!-- CFR for Cookie Banner (Banner Info Message). The placeholder will be updated with the app name. -->
    <string name="cfr_for_cookie_banner">%1$s tries to reject cookie requests to dismiss annoying cookie banners.\n\nManage cookie banner preferences in settings.</string>

    <!-- CFR for Cookie Banner (Banner Info Message).This text should be the same as the settings text from "cfr_for_cookie_banner".The text should be underlined and should redirect the user to the cookie banner settings screen. -->
    <string name="cfr_for_cookie_banner_underline_settings">settings</string>

The code tries to find the localized text for settings in the first string, which doesn't make a lot of sense, because it's going to break if the translation is not consistent (including capitalization, as far as I understand Kotlin).

On the other hand, the link for the text could just be passed as a variable.

    <!-- CFR for Cookie Banner (Banner Info Message). %1$s will be replaced by the app name, %2$s will be an active link using the string cfr_cookie_banner_link as text. -->
    <string name="cfr_cookie_banner">%1$s tries to reject cookie requests to dismiss annoying cookie banners.\n\nManage cookie banner preferences in %2$s.</string>

    <!-- CFR for Cookie Banner (Banner Info Message). This string is used as text for a link in cfr_cookie_banner and takes the user to the app settings. -->
    <string name="cfr_cookie_banner_link">settings</string>

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

:giorga, since you are the author of the regressor, bug 1797600, could you take a look? Also, could you set the severity field?

For more information, please visit auto_nag documentation.

Comment 2

[Francesco Lodolo [:flod]]

Note that I used different IDs in the second example, because we need them at this point.

Comment 3

[BMO Automation]

Attachment: [mozilla-mobile/firefox-android] Bug 1809972 - Cookie banner CFR: code should not try to replace localized text when it can pass a variable (#540)

Comment 4

[BMO Automation]

Attachment: [mozilla-mobile/firefox-android] Bug 1809972 - Part 2: Use the correct moz:RemovedIn version for cookie banner CFR strings (#557)

Comment 5

[BMO Automation]

Attachment: [mozilla-mobile/firefox-android] Bug 1809972 - Part 3: Re-add the removed localization string comments for the cookie banner CFR (#572)

Comment 6

[Chris Peterson [:cpeterson]]

Merged https://github.com/mozilla-mobile/firefox-android/commit/2b346a8881b46fef6ff0bf9a67ae946ba6394ba4

Comment 7

[Chris Peterson [:cpeterson]]

https://github.com/mozilla-mobile/firefox-android/commit/1ade3830005a02ef1fb7bdaf5bb6b2fcf30334fd

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6594ef81cd7b
Cookie banner CFR: code should not try to replace localized text when it can pass a variable
https://hg.mozilla.org/integration/autoland/rev/0828827fea7c
Part 2: Use the correct moz:RemovedIn version for cookie banner CFR strings
https://hg.mozilla.org/integration/autoland/rev/f19b9ac05e87
Part 3: Re-add the removed localization string comments for the cookie banner CFR