Closed Bug 1407562 Opened 3 years ago Closed 3 years ago

Fix about:telemetry in RTL locales


(Toolkit :: Telemetry, defect, P3)




Tracking Status
firefox58 --- verified
firefox59 --- verified


(Reporter:, Assigned: adbugger, Mentored)



(Whiteboard: [good first bug][lang=css])


(5 files, 2 obsolete files)

Attached image rtl_about_telemetry.png
Steps to Reproduce:
1. Start Nightly in RTL locales
2. Go to about:telemetry

Actual Results:
- JSON menu is in opposite
- dropdown marker is in opposite

Expected Results:
correct position
The dropdown marker position is determined by common.css:206's html|select:not([size]):not([multiple]) selector. It needs a dir=rtl version.

The "Raw JSON" menu item is absolutely positioned at aboutTelemetry.css:35's #category-raw selector which also needs a dir=rtl version. Either that, or #category-raw can be re-parented and positioned within the scrolling div using flex (if you want to get creative)
Blocks: 1384534
Mentor: chutten
Priority: -- → P3
Whiteboard: [good first bug][lang=css]
I am new to open source and I am interested in working on this bug.

I was looking at the common.css file in the obj-x86_64-pc-linux-gnu directory as it seems to be the only one with the html|select:not([size]):not([multiple]) selector. However, my changes are being overwritten at every build. Is this directory generated at compile time? If so, which files do I edit so that the changes are reflected here?

The other "common.css" files are:

These don't have the particular selector, so I don't think these are relevant?
Flags: needinfo?(chutten)
The obj-x86_64-pc-linux-gnu directory is the object directory where build products are sent during compile. 

You'll want to make your changes in a source directory. Specifically, you'll find the html|select:not([size]):not([multiple]) selector in toolkit/themes/shared/in-content/

A handy tool for searching the codebase is searchfox. This is how I tracked down where the file was located:

Let me know if I can be of any help!
Flags: needinfo?(chutten)
Assignee: nobody → adibhar97
Thanks for the tip. 
So I tried using this add-on to switch to RTL mode without changing language:

This is incompatible with nightly. I tried changing extensions.legacy.enabled = true from the about:config page. It still doesn't install. What do you suggest I use to change to RTL mode?
(In reply to Aditya Bharti from comment #4)

Please try as below...
1. Open about:telemetry
2. Open DevTools (Ctrl+Shift+I)
3. Select Scratchpad tab
4. Run "document.body.dir = 'rtl';"
Attached patch Patch file v1 (obsolete) — Splinter Review
Thanks @magicp for the help. Here is an initial patch file with the appropriate changes. I need some help unit testing it. I'm running "./mach mochitest" on the changed directories. I can see the tests being run in nightly. However they seem to take a lot of time, or even hang sometimes. What am I doing wrong?
Flags: needinfo?(chutten)
mochitests require focus be placed on the window performing the test. This is because some things behave poorly when Firefox isn't the foreground application. This, unfortunately, makes mochitests inconvenient to test :S

That being said, mochitests aren't used to test in-content pages, so you don't need to worry about it.

What that does mean is we need to test a couple of places to see what the behaviour is, with and without your change. Here's a quick list of a couple of places that use SELECTs:

One example is the "Add an autofill credit card dialog" which you can reach through about:preferences > Privacy & Security > Saved Credit Cards... > Add
Flags: needinfo?(chutten)
Comment on attachment 8928461 [details] [diff] [review]
Patch file v1

Review of attachment 8928461 [details] [diff] [review]:

::: toolkit/themes/shared/in-content/
@@ +215,4 @@
>    text-overflow: ellipsis;
>  }
> +html|body[dir=rtl] select:not([size]):not([multiple]){

The following selector is preferred according to :ntim and :Gijs from #fx-team:

Ah, that would explain it. I was leaving the tests to run and shifting to other applications.
I will change the selector in the file.
Attached image add_new_credit_card.png
Attached image add_new_address.png
Attached image telemetry.png
Attached patch Patch file v2: changed selector (obsolete) — Splinter Review
Changed selector in Screenshots attached. Seems fine. Any other suggestions regarding pages to test?
Attachment #8928461 - Attachment is obsolete: true
Output of export. Patch file complete with appropriate fields.
Attachment #8928631 - Attachment is obsolete: true
Attachment #8928639 - Flags: review?(chutten)
Attachment #8928639 - Flags: review?(chutten) → review+
Keywords: checkin-needed
Pushed by
Fix in-content HTML selects and about:telemetry "Raw JSON" element for RTL locales r=chutten
Keywords: checkin-needed
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment on attachment 8928639 [details] [diff] [review]
Changed selector. Fixed additional places in UI.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1365489 (perhaps)
[User impact if declined]: in-content HTML SELECT (used in formautofill dialogs, about:telemetry, possibly elsewhere) will look awful in RTL locales
[Is this code covered by automated tests?]: Nope
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]:  Open about:telemetry in a Firefox profile with an RTL locale
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: The change is RTL-only styles for one specific element in about:telemetry, and a mostly-unused-in-content HTML element
[String changes made/needed]: None
Attachment #8928639 - Flags: approval-mozilla-beta?
Comment on attachment 8928639 [details] [diff] [review]
Changed selector. Fixed additional places in UI.

Fix about:telemetry in RTL locales. Beta58+.
Attachment #8928639 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I have reproduced this issue using Firefox  58.0a1 RTL-build (2017.10.11) on Win 8.1 x64.
I can confirm this issue is fixed, I verified using Firefox 58.0b6 RTL-build (ID = 20171123161455) on Win 8.1 x64, Ubuntu 14.04 x64 and Mac OS X 10.12.6.
Note: this part of issue is fixed: - JSON menu is on right side of the screen
                                   - drop-down marker is on left side of the button
      In formautofill on "Add New Address" form/ "Country of Region" button, the drop-down is still on right 
      side of the button. The same things happens on "Add New Credit Card" form. 
      The "Form Autofill" was tested only on en-us builds, not tested on RTL builds.
Flags: qe-verify+
I can confirm this issue is fixed, I verified using Firefox 59.0a1 RTL-build on Win 8.1 x64, Ubuntu 14.04 x64 and Mac OS X 10.12.6.
You need to log in before you can comment on or make changes to this bug.