Last Comment Bug 704017 - [RTL] The Inspector rules view should be LTR
: [RTL] The Inspector rules view should be LTR
Status: VERIFIED FIXED
[qa!]
: rtl
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: unspecified
: All All
: -- normal with 2 votes (vote)
: Firefox 12
Assigned To: Paul Rouget [:paul]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-20 15:08 PST by Tomer Cohen :tomer
Modified: 2012-02-22 01:52 PST (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
verified


Attachments
Screenshot (531.74 KB, image/png)
2011-11-20 15:08 PST, Tomer Cohen :tomer
no flags Details
a more detailed screenshot showing what should and what should not be RTL (260.55 KB, image/png)
2011-12-24 13:09 PST, Amir Aharoni
no flags Details
RTL test (205.58 KB, image/png)
2012-01-03 05:34 PST, Paul Rouget [:paul]
no flags Details
patch v1 (1.34 KB, patch)
2012-01-03 05:58 PST, Paul Rouget [:paul]
jwalker: review+
Details | Diff | Splinter Review
patch v1.1 (1.33 KB, patch)
2012-01-03 08:55 PST, Paul Rouget [:paul]
jwalker: review+
dao+bmo: review-
Details | Diff | Splinter Review
patch v1.2 (2.71 KB, patch)
2012-01-05 03:47 PST, Paul Rouget [:paul]
dao+bmo: review+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Tomer Cohen :tomer 2011-11-20 15:08:19 PST
Created attachment 575778 [details]
Screenshot

Given that code such as CSS and JavaScript is written in latin characters, I'd suggest making sure that the Style Inspector/Editor will be hardcoded to LTR even on RTL environments.

Steps to reproduce:
a. Launch RTL Firefox build, or use FORCE-RTL addon.
b. Go to any website, launch Inspector (Aurora→Developer Tools→Inspector).
c. Click Style button at the bottom left.

Expected result:
CSS code should not be right-aligned (direction:rtl), checkboxes should be at the left side of each line.
Comment 1 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-20 16:02:16 PST
CCing Ehsan because I have a feeling that he's of the opinion that some locales do use RTL in CSS.

Maybe this bug should be to make reading direction separately configurable for Developer Tools.
Comment 2 Tomer Cohen :tomer 2011-11-20 23:45:22 PST
(In reply to Joe Walker from comment #1)
> Maybe this bug should be to make reading direction separately configurable
> for Developer Tools.
I'm not sure if it is needed, as I've never seen someone actually writing code in RTL environment, and I guess that if we'll have it configurable in the localization strings, all RTL locales will keep it as LTR.

Please also note that we keep other tools such as View Source, and the inspector HTML view as LTR, so the stylesheet should be kept as LTR as well because of consistently. 

The only exception I see here is if the rules are about to get translated (so instead of 'border' we should write it in other languages). If this is going to happen, it would make the situation more difficult and it might be better to leave it as RTL…
Comment 3 Mihai Sucan [:msucan] 2011-11-21 05:51:39 PST
This bug seems to be about the style rules view of the Inspector - which I agree it needs more RTL-related polish. Tomer, please correct me if I am wrong.

The style editor we landed forces LTR for the editor.
Comment 4 Amir Aharoni 2011-12-24 13:09:33 PST
Created attachment 584222 [details]
a more detailed screenshot showing what should and what should not be RTL

The actual rules must definitely be left-to-right in all locales, because CSS is inherently a left-to-right language.

The titles that say "inherited from X" should remain right-to-left in RTL locales, as they already are, because they have text that can be translated into a right-to-left language.
Comment 5 Tomer Cohen :tomer 2012-01-02 00:01:46 PST
This issue still exists in recent Aurora builds (and Beta?) and should be easy to resolve if we just force it to LTR as I suggested in comment 0. Please fix it before final release as we wish to see people actually using it.
Comment 6 Paul Rouget [:paul] 2012-01-03 05:34:23 PST
Created attachment 585386 [details]
RTL test

Is that better? (beside the treetwisties image being RTL)
Comment 7 Amir Aharoni 2012-01-03 05:41:30 PST
Yes, this looks good.
Comment 8 Paul Rouget [:paul] 2012-01-03 05:58:30 PST
Created attachment 585390 [details] [diff] [review]
patch v1
Comment 9 Paul Rouget [:paul] 2012-01-03 08:55:06 PST
Created attachment 585428 [details] [diff] [review]
patch v1.1
Comment 10 Dão Gottwald [:dao] 2012-01-04 03:11:12 PST
Comment on attachment 585428 [details] [diff] [review]
patch v1.1

>--- a/browser/themes/gnomestripe/devtools/csshtmltree.css
>+++ b/browser/themes/gnomestripe/devtools/csshtmltree.css

>-.ruleview-expander:-moz-locale-dir(rtl) {
>-  background-position: 16px 0;
>-}

It looks like winstripe and pinstripe need this change as well.
Comment 11 Paul Rouget [:paul] 2012-01-05 03:47:59 PST
Created attachment 586032 [details] [diff] [review]
patch v1.2

addressed Dao's comments
Comment 12 Paul Rouget [:paul] 2012-01-05 03:48:46 PST
(In reply to Dão Gottwald [:dao] from comment #10)
> Comment on attachment 585428 [details] [diff] [review]
> patch v1.1
> 
> >--- a/browser/themes/gnomestripe/devtools/csshtmltree.css
> >+++ b/browser/themes/gnomestripe/devtools/csshtmltree.css
> 
> >-.ruleview-expander:-moz-locale-dir(rtl) {
> >-  background-position: 16px 0;
> >-}
> 
> It looks like winstripe and pinstripe need this change as well.

Hmm, you're right. I was convinced we were using this twisty only on Linux.
Comment 13 Dão Gottwald [:dao] 2012-01-05 04:02:42 PST
Comment on attachment 586032 [details] [diff] [review]
patch v1.2

We really shouldn't be using this twisty anywhere, it's quite ugly.
Comment 14 Paul Rouget [:paul] 2012-01-05 04:19:42 PST
Yeah. bug 699840
Comment 15 :Ehsan Akhgari 2012-01-10 14:45:56 PST
Do you know when/how this will be merged to mozilla-central?  Thanks!
Comment 16 Rob Campbell [:rc] (:robcee) 2012-01-14 09:57:26 PST
https://hg.mozilla.org/integration/fx-team/rev/2344d3d74214
Comment 17 Tim Taubert [:ttaubert] 2012-01-16 02:28:09 PST
https://hg.mozilla.org/mozilla-central/rev/2344d3d74214
Comment 18 Tomer Cohen :tomer 2012-01-30 11:43:55 PST
A user have submitted feedback requesting fixing this issue. 
http://input.mozilla.com/he/opinion/2574939
Comment 19 Paul Rouget [:paul] 2012-01-30 11:52:06 PST
(In reply to Tomer Cohen :tomer from comment #18)
> A user have submitted feedback requesting fixing this issue.
> http://input.mozilla.com/he/opinion/2574939

This issues is FIXED. If more work is needed for RTL support, can you please open a new bug with some more details about what is supposed to be done (like in attachment 584222 [details])?
Comment 20 Tomer Cohen :tomer 2012-01-30 11:57:40 PST
(In reply to Paul Rouget [:paul] from comment #19)
> This issues is FIXED. If more work is needed for RTL support, can you please
> open a new bug with some more details about what is supposed to be done
> (like in attachment 584222 [details])?

As far as I know, it is fixed only on Nightly, making it possible that we will release a broken feature. I can still reproduce it on recent Aurora builds.
Comment 21 Dave Camp (:dcamp) 2012-02-02 11:54:03 PST
Comment on attachment 586032 [details] [diff] [review]
patch v1.2

[Approval Request Comment]
Regression caused by (bug #): New feature
User impact if declined: Rule View is confusing for people in RTL locales.
Testing completed (on m-c, etc.): Is on m-c for 2 weeks, aurora since the merge.
Risk to taking this patch (and alternatives if risky): CSS patch, not much risk.
String changes made by this patch: None.
Comment 22 Alex Keybl [:akeybl] 2012-02-05 13:59:20 PST
Comment on attachment 586032 [details] [diff] [review]
patch v1.2

[Triage Comment]
Issue with new feature and low risk - approved for Beta 11.
Comment 24 Vlad [QA] 2012-02-22 01:52:52 PST
I have followed the steps from the description on FA locale:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:11.0) Gecko/20100101 Firefox/11.0 beta 3
Mozilla/5.0 (Windows NT 6.1; rv:11.0) Gecko/20100101 Firefox/11.0 beta 3
Mozilla/5.0 (X11; Linux i686; rv:11.0) Gecko/20100101 Firefox/11.0 beta 3

The checkboxes areat the left side of each line.
Setting the resolution to Verified Fixed on beta.

Note You need to log in before you can comment on or make changes to this bug.