Closed Bug 704017 Opened 13 years ago Closed 12 years ago

[RTL] The Inspector rules view should be LTR

Categories

(DevTools :: Inspector, defect)

defect
Not set
normal

Tracking

(firefox11- verified)

VERIFIED FIXED
Firefox 12
Tracking Status
firefox11 - verified

People

(Reporter: tomer, Assigned: paul)

Details

(Keywords: rtl, Whiteboard: [qa!])

Attachments

(4 files, 2 obsolete files)

Attached image 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.
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.
OS: Linux → All
Hardware: x86 → All
(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…
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.
Component: Developer Tools: Style Editor → Developer Tools: Inspector
QA Contact: developer.tools.style.editor → developer.tools.inspector
Summary: [RTL] Firefox Style Editor should be LTR → [RTL] The Inspector rules view should be LTR
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.
Attachment #584222 - Flags: ui-review+
Attachment #584222 - Flags: ui-review+
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.
Attached image RTL test
Is that better? (beside the treetwisties image being RTL)
Yes, this looks good.
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #585390 - Flags: review?(jwalker)
Attached patch patch v1.1 (obsolete) — Splinter Review
Attachment #585390 - Attachment is obsolete: true
Attachment #585390 - Flags: review?(jwalker)
Attachment #585428 - Flags: review?(jwalker)
Attachment #585390 - Flags: review+
Attachment #585428 - Flags: review?(jwalker) → review+
Attachment #585428 - Flags: review?(dao)
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.
Attachment #585428 - Flags: review?(dao) → review-
Attached patch patch v1.2Splinter Review
addressed Dao's comments
Attachment #585428 - Attachment is obsolete: true
(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.
Attachment #586032 - Flags: review?(dao)
Comment on attachment 586032 [details] [diff] [review]
patch v1.2

We really shouldn't be using this twisty anywhere, it's quite ugly.
Attachment #586032 - Flags: review?(dao) → review+
Yeah. bug 699840
Whiteboard: [land-in-fx-team]
Do you know when/how this will be merged to mozilla-central?  Thanks!
Assignee: nobody → paul
Status: NEW → ASSIGNED
https://hg.mozilla.org/integration/fx-team/rev/2344d3d74214
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/2344d3d74214
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 12
A user have submitted feedback requesting fixing this issue. 
http://input.mozilla.com/he/opinion/2574939
(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])?
(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 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.
Attachment #586032 - Flags: approval-mozilla-beta?
Comment on attachment 586032 [details] [diff] [review]
patch v1.2

[Triage Comment]
Issue with new feature and low risk - approved for Beta 11.
Attachment #586032 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [qa+]
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.
Status: RESOLVED → VERIFIED
Whiteboard: [qa+] → [qa!]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.