1313486 - Fix unicode-bidi for DOM label and value in RTL locales

Status: RESOLVED FIXED

(DevTools :: DOM, defect, P2)

Description

[magicp]

Attachment: fix-unicode-bidi-in-dom.png

Steps to Reproduce:
1. Start Nightly in RTL locales (e.g. Arabic)
2. Go to about:home
3. Open DevTools > DOM
4. Type "_" in filter input
5. Check label and value

Actual Results:
unicode-bidi is wrong.

Expected Results:
fix unicode-bidi for DOM label and value in RTL locales.

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: dom-unicode-bidi.png

Thanks for the report!

I am not sue if I understand the core of the issue. I am attaching my screenshot, can you explain what's wrong on it?

Do we somehow mix RTL with LTR?

Honza

Comment 2

[magicp]

Attachment: expected-results.png

Hi Honza, here is an example for expected results.

Comment 3

[magicp]

I think this bug is one of unicode-bidi issues. For example, tooltip of bookmark display "/https://www.mozilla.org/en-US/contribute/" as "https://www.mozilla.org/en-US/contribute" in RTL locales. I will file these issues later.

Comment 4

[magicp]

> "/https://www.mozilla.org/en-US/contribute/"
"/https://www.mozilla.org/en-US/contribute"

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to magicp from comment #2)
> Created attachment 8805471 [details]
> expected-results.png
> 
> Hi Honza, here is an example for expected results.
Thanks for the screenshot!

So, I see two issues to be fixed in this report:

1) JS array: 
JS Array should be displayed as follows in RTL:

`[] Array`


2) Variables with underscore
Variables should not change position of `_` character.

`_generateCallbackID` should look the same in RTL an LTR.

Honza

Comment 6

[magicp]

(In reply to Jan Honza Odvarko [:Honza] from comment #5)
> JS Array should be displayed as follows in RTL:
> 
> `[] Array`

I see!

Comment 7

[[:Towkir] Ahmed]

Hi, I would like to work on this, can you suggest where to start ?
Thanks

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to [:Towkir] Ahmed from comment #7)
> Hi, I would like to work on this, can you suggest where to start ?
Excellent!

Let me first clear what is needed to do here.


1) A little correction. JS array should look the same in RTL and LTR so, we don't have to do anything in this case. Let's follow how math works - it looks the same in LTR and RTL. See [1] as an example.

2) I believe that the second thing should be fixed:
`_generateCallbackID` should look the same in RTL an LTR.

Currently, the variable name looks like as follows in RTL:
`generateCallbackID_`

(the underscore character is moved at the end)

@Eitan: variable names shouldn't be changed, right?


Honza




[1] https://he.wikibooks.org/wiki/%D7%9E%D7%AA%D7%9E%D7%98%D7%99%D7%A7%D7%94_%D7%AA%D7%99%D7%9B%D7%95%D7%A0%D7%99%D7%AA/%D7%90%D7%9C%D7%92%D7%91%D7%A8%D7%94_%D7%AA%D7%99%D7%9B%D7%95%D7%A0%D7%99%D7%AA/%D7%9E%D7%A9%D7%95%D7%95%D7%90%D7%95%D7%AA/%D7%9E%D7%A9%D7%95%D7%95%D7%90%D7%95%D7%AA_%D7%A8%D7%99%D7%91%D7%95%D7%A2%D7%99%D7%95%D7%AA

Comment 9

[Eitan Isaacson [:eeejay]]

Correct. I think a good place to start is to keep block containers as rtl according to locale, but any inline content should be ltr. For example, the DOM tree view should be rendered rtl, but each column in each row should be ltr.

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Eitan Isaacson [:eeejay] from comment #9)
> Correct. I think a good place to start is to keep block containers as rtl
> according to locale, but any inline content should be ltr. For example, the
> DOM tree view should be rendered rtl, but each column in each row should be
> ltr.
Yes, agree.


:Towkir, I did some analysis of the #2 problem (comment #8) and the underscore character has special meaning in RTL. We can fix it by explicit setting dir="ltr". This is partially done for functions see:
https://dxr.mozilla.org/mozilla-central/rev/ade8d4a63e57560410de106450f37b50ed71cca5/devtools/client/shared/components/reps/function.js#47

... and we need to also do it for Tree labels. It should be done in: devtools/client/shared/components/tree/label-cell. See the render method, we need to put dir="ltr" into the treeLabel element. I wasn't able to fix it using CSS (direction: ltr) and so, I guess we need to use an attribute.

Does it make sense?

Honza

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

See also bug 1290056 for more details how to fix the problem.

Honza

Comment 12

[magicp]

Can we use "unicode-bidi: -moz-plaintext;" (or plaintext) ?

Comment 13

[[:Towkir] Ahmed]

Hi :Honza
I will get back to this with my progress soon, for now, not clearing the needinfo intentionally.

Comment 14

[[:Towkir] Ahmed]

Hello Honza,
I had a look at this and found that magicp's comment 12 fixes the issue. (This is when I want to fix this via css)
If we have to fix this by JS, it would be nice if you could explain the reason behind it, this is just my curiosity, not any judgment. Meanwhile I am looking into your comment 10
Thanks

Comment 15

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to [:Towkir] Ahmed from comment #14)
> Hello Honza,
> I had a look at this and found that magicp's comment 12 fixes the issue.
> (This is when I want to fix this via css)
Sounds good to me. Do you have a patch to attach?

Honza

Comment 16

[[:Towkir] Ahmed]

Yes, 
Will submit a patch soon.

Comment 17

[[:Towkir] Ahmed]

Attachment: unicodebdifixed.patch

Hi Honza,
Please have a look at this, Hope this helps.
Thanks

Comment 18

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8814470 [details] [diff] [review]
unicodebdifixed.patch

Review of attachment 8814470 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, thanks!

Honza

Comment 19

[[:Towkir] Ahmed]

For checking in.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b922bd2f864
Fix unicode-bidi for DOM label and value in RTL locales. r=Honza

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8b922bd2f864

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Honza, is it something that we want to uplift in 52? Thanks

Comment 23

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I think that this was a minor issue and we can wait. Also, I recall that there have been some other patches related to RTL so, I am rather reluctant to uplift just this one patch. In any case, thanks for the notification!

Honza