Closed Bug 1299723 Opened 3 years ago Closed 3 years ago

Fix direction of propertyContainer in RTL locales

Categories

(DevTools :: Inspector: Computed, defect, P2)

defect

Tracking

(firefox48 wontfix, firefox49 wontfix, firefox-esr45 affected, firefox50 affected, firefox51 affected, firefox52 fixed)

RESOLVED FIXED
Firefox 52
Tracking Status
firefox48 --- wontfix
firefox49 --- wontfix
firefox-esr45 --- affected
firefox50 --- affected
firefox51 --- affected
firefox52 --- fixed

People

(Reporter: magicp.jp, Assigned: vi.le, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(3 files, 3 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20160831030224

Steps to reproduce:

1. Start Nightly in RTL locales (e.g. Arabic)
2. Go to any sites (e.g. about:home)
3. Open DevTools > Inspector > Computed
4. Check-on "Browser styles"
5. Scroll down until "-moz-..." properties


Actual results:

Vendor prefix "-moz-" does not display correctly. 


Expected results:

#propertyContainer should be set "direction: ltr;". This direction is same with Rules view.
Has STR: --- → yes
Component: Untriaged → Developer Tools: Computed Styles Inspector
OS: Unspecified → All
Hardware: Unspecified → All
This is ... surprising :)
Here it is in isolation: http://jsbin.com/duqegawepa/edit?html,css,output
We should force ltr on our property names. These property names are English names and therefore should be displayed in ltr to avoid this display bug.
Mentor: pbrosset
Keywords: good-first-bug
Priority: -- → P2
devtools-filterinput, devtools-searchinput and devtools-textinput have same issue.
Hello,

can I work on this bug please ?
(In reply to sky from comment #4)
> Hello,
> 
> can I work on this bug please ?
Yes, sure! Thanks for your interest.
Make sure you go through our contribution guide first: https://developer.mozilla.org/en-US/docs/Tools/Contributing
And once you have your dev environment up and running, I suggest you use the Browser Toolbox to inspect the inspector (yes you can use devtools on devtools!): https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox
This will help you inspect the part of the UI that should be in LTR and investigate further if you need.

In terms of code changes, I believe they should happen inside this CSS file:
devtools\client\themes\computed.css

I'll assign the bug to you now, let me know if you have further questions.
Once you're ready with a change, please do create a patch file attached to this bug and I'll take a look:
https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Assignee: nobody → sky
Status: NEW → ASSIGNED
Attachment #8788982 - Flags: review+
Attachment #8788982 - Flags: feedback+
Comment on attachment 8788982 [details] [diff] [review]
fix-direction-propertyContainer.patch

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

I think you meant to ask review from :pbro.

2 drive through comments:
- Please add comments before each direction: ltr; so it's obvious why it's there
Something like: /* Avoid english text (css properties) from being altered by RTL mode */
- Make sure to test that you haven't broken anything else in RTL mode with this change.
Attachment #8788982 - Flags: review?(pbrosset)
Attachment #8788982 - Flags: review+
Attachment #8788982 - Flags: feedback+
Ok, I will do some testing and comment the change. Thank you for the review
Attachment #8788982 - Attachment is obsolete: true
Attachment #8788982 - Flags: review?(pbrosset)
Attachment #8789747 - Flags: review+
Comment on attachment 8789747 [details] [diff] [review]
fix-direction-propertyContainer.patch

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

You need to get review+ from a reviewer to land the change.
Attachment #8789747 - Flags: review+ → review?(pbrosset)
Comment on attachment 8789747 [details] [diff] [review]
fix-direction-propertyContainer.patch

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

Thanks. Good start. See my comments below.

::: devtools/client/themes/common.css
@@ +510,5 @@
>    font: message-box;
> +
> +  /* Avoid english text (css properties) from being altered
> +     by RTL mode. */
> +  direction: ltr;

Please let's leave this to another bug. Let;s focus on one problem per bug. That makes it easier to land quickly and track.

::: devtools/client/themes/computed.css
@@ +54,5 @@
>    flex: auto;
> +
> +  /* Avoid english text (css properties) from being altered
> +     by RTL mode. */
> +  direction: ltr;

I think this change should go at a lower level. Here you're basically forcing the whole list of porperties to be LTR, so names will be left, and values right. Text will be left aligned. And expanding icons will be left too, which isn't what we want.
We want the UI to be familiar to RTL users, but just keep the property names as LTR, to avoid english names to be reversed.

So UI in RTL, English words LTR.

With this in mind, I think you should remove this here, and instead put this property in the .property-name rule.

In fact, after looking at the code a little bit more, I found out that property-values are already forced to LTR, which is good. And the way this is done is in 
devtools\client\inspector\computed\computed.js
Search for:
this.valueNode.setAttribute("dir", "ltr");

So in order to be consistent, let's do the same thing here (see a few lines above, where this.nameNode is used):
this.nameNode.setAttribute("dir", "ltr");

I think that would make more sense.
Attachment #8789747 - Flags: review?(pbrosset)
Attached image matchedselectors.png
Please consider direction of matchedselectors. Thanks.
Thanks for the feedback and the help
(In reply to magicp from comment #12)
> Created attachment 8789824 [details]
> matchedselectors.png
> 
> Please consider direction of matchedselectors. Thanks.

In the same bug ?
(In reply to sky from comment #14)
> (In reply to magicp from comment #12)
> > Created attachment 8789824 [details]
> > matchedselectors.png
> > 
> > Please consider direction of matchedselectors. Thanks.
> 
> In the same bug ?
Let's keep patches small and bugs focused on one single problem if possible.
Could you file another bug for this?
(In reply to Patrick Brosset <:pbro> from comment #15)

> Let's keep patches small and bugs focused on one single problem if possible.
> Could you file another bug for this?

Okay. I will file about matchedselectors after fixed this bug if needed. Thanks.
Attached patch fix-direction-nameNode.patch (obsolete) — Splinter Review
Attachment #8789747 - Attachment is obsolete: true
Attachment #8791525 - Flags: review?(pbrosset)
Comment on attachment 8791525 [details] [diff] [review]
fix-direction-nameNode.patch

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

Looks good thanks!
Could you alter the commit message a little bit:
Bug 1299723 - Force LTR direction on computed property names; r=pbro

Once done, please upload a new patch again, mark this one as obsolete, and mark the new one as R+. And ping me so I push your patch to the TRY repo so we can test this.
Attachment #8791525 - Flags: review?(pbrosset) → review+
Attachment #8791525 - Attachment is obsolete: true
Attachment #8792313 - Flags: review+
Ping? :-)
Flags: needinfo?(pbrosset)
Here's your patch on TRY : https://treeherder.mozilla.org/#/jobs?repo=try&revision=173a13c76db7
Let's wait and see if all the tests still pass (which should be the case since this shouldn't affect how tests run). Once done, we can request this patch to be checked-in.
Flags: needinfo?(pbrosset)
The TRY build isn't fully done yet, but given the nature of the change, there's enough green in there already to land this change. So let's ask for check-in.

Thanks for this fix. Let me know if you're looking for more bugs to fix, I can help.
Keywords: checkin-needed
Ok, thank you
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/fbb8dc968a5b
Force LTR direction on computed property names. r=pbro
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fbb8dc968a5b
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.