Closed Bug 1313486 Opened 3 years ago Closed 3 years ago

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

Categories

(DevTools :: DOM, defect, P2)

defect

Tracking

(firefox52 affected, firefox53 fixed)

RESOLVED FIXED
Firefox 53
Tracking Status
firefox52 --- affected
firefox53 --- fixed

People

(Reporter: magicp.jp, Assigned: Towkir, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(4 files)

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.
Attached image 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
Flags: needinfo?(magicp.jp)
Attached image expected-results.png
Hi Honza, here is an example for expected results.
Flags: needinfo?(magicp.jp)
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.
(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
Mentor: odvarko
Keywords: good-first-bug
(In reply to Jan Honza Odvarko [:Honza] from comment #5)
> JS Array should be displayed as follows in RTL:
> 
> `[] Array`

I see!
Hi, I would like to work on this, can you suggest where to start ?
Thanks
Flags: needinfo?(odvarko)
(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
Flags: needinfo?(odvarko) → needinfo?(eitan)
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.
Flags: needinfo?(eitan)
(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
Flags: needinfo?(3ugzilla)
See also bug 1290056 for more details how to fix the problem.

Honza
Can we use "unicode-bidi: -moz-plaintext;" (or plaintext) ?
Hi :Honza
I will get back to this with my progress soon, for now, not clearing the needinfo intentionally.
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
Flags: needinfo?(3ugzilla)
Flags: needinfo?(odvarko)
(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
Flags: needinfo?(odvarko)
Yes, 
Will submit a patch soon.
Assignee: nobody → 3ugzilla
Status: NEW → ASSIGNED
Hi Honza,
Please have a look at this, Hope this helps.
Thanks
Attachment #8814470 - Flags: review?(odvarko)
Comment on attachment 8814470 [details] [diff] [review]
unicodebdifixed.patch

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

Looks good to me, thanks!

Honza
Attachment #8814470 - Flags: review?(odvarko) → review+
For checking in.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b922bd2f864
Fix unicode-bidi for DOM label and value in RTL locales. r=Honza
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8b922bd2f864
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Honza, is it something that we want to uplift in 52? Thanks
Flags: needinfo?(odvarko)
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
Flags: needinfo?(odvarko)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.