1961757 - toDisplayUrl in URLStringUtils returns String when the URL is RTL

Status: RESOLVED FIXED

(Firefox for Android :: Tooling, defect)

Description

[Michel Le Bihan]

This is something that I missed in https://bugzilla.mozilla.org/show_bug.cgi?id=1954340. When the URL is RTL, toDisplayUrl does "\u200E" + strippedText effectively converting the CharSequence to a string and stripping all markup.

Comment 1

[Michel Le Bihan]

Attachment: Bug 1961757 - Set text direction in Android toolbar instead of adding directional marks. r=tthibaud,petru

Comment 2

[Michel Le Bihan]

I see that the logic of prepending the LTR mark was added in https://github.com/mozilla-mobile/android-components/pull/10599 following https://bugzilla.mozilla.org/show_bug.cgi?id=1704422.

Is there a reason why this approach was preferred over setting textDirection on the TextView with a patch like:

--- a/mobile/android/android-components/components/browser/toolbar/src/main/java/mozilla/components/browser/toolbar/display/OriginView.kt
+++ b/mobile/android/android-components/components/browser/toolbar/src/main/java/mozilla/components/browser/toolbar/display/OriginView.kt
@@ -48,6 +48,8 @@ internal class OriginView @JvmOverloads 
         isClickable = true
         isFocusable = true
 
+        textDirection = View.TEXT_DIRECTION_LTR
+
         setOnClickListener {
             if (onUrlClicked()) {
                 toolbar.editMode()

Comment 3

[Petru-Mugurel Lingurar [:petru]]

(In reply to Michel Le Bihan from comment #2)

I see that the logic of prepending the LTR mark was added in https://github.com/mozilla-mobile/android-components/pull/10599 following https://bugzilla.mozilla.org/show_bug.cgi?id=1704422.

Is there a reason why this approach was preferred over setting textDirection on the TextView with a patch like:

Think the response (and a test case) is in bug 1704422 comment 5.
No matter the textDirection we set if the text starts with RTL characters then the TextView will render them at the right.

Comment 4

[Michel Le Bihan]

The referenced issue and comment do not mention attempting to set textDirection. Do you remember how you were setting it? When I apply the diff from https://bugzilla.mozilla.org/show_bug.cgi?id=1961757#c2, all test URLs display correctly for me.

Comment 5

[Petru-Mugurel Lingurar [:petru]]

Indeed.
Tested the scenario from bug 1704422 with LTR set for urlView after removing \u200E from toDisplayUrl and all looks good so seems like we can stop using \u200E.

Comment 6

[BugBot [:suhaib / :marco]]

The severity field is not set for this bug.
:007, could you have a look please?

For more information, please visit BugBot documentation.

Comment 7

[Michel Le Bihan]

Could you please look at the linked revision?

Comment 8

[Petru-Mugurel Lingurar [:petru]]

(In reply to Michel Le Bihan from comment #7)

Could you please look at the linked revision?

Testing it I see that the issue from bug 1704422#c5 does not reproduce so I do appreciate the simplified overall code, thanks!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4b0be69146de
Set text direction in Android toolbar instead of adding directional marks. r=tthibaud,android-reviewers,petru

Comment 10

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/4b0be69146de