1407562 - Fix about:telemetry in RTL locales

Status: VERIFIED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[magicp]

Attachment: 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

Comment 1

[Chris H-C :chutten]

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)

Comment 2

[Aditya Bharti [:adbugger]]

Hi,
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:
mozilla-central/dom/xml/test/old/books/common.css
mozilla-central/testing/web-platform/tests/css/css-regions-1/interactivity/full-screen/support/common.css
mozilla-central/testing/web-platform/tests/css/css-regions-1/contentEditable/support/common.css
mozilla-central/editor/libeditor/tests/browserscope/lib/richtext2/richtext2/static/common.css
mozilla-central/devtools/client/themes/common.css
mozilla-central/toolkit/themes/linux/global/in-content/common.css
mozilla-central/toolkit/themes/windows/global/in-content/common.css
mozilla-central/toolkit/themes/osx/global/in-content/common.css
mozilla-central/layout/reftests/css-ruby/common.css
mozilla-central/layout/reftests/table-background/common.css

These don't have the particular selector, so I don't think these are relevant?

Comment 3

[Chris H-C :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/common.inc.css

A handy tool for searching the codebase is searchfox. This is how I tracked down where the file was located: http://searchfox.org/mozilla-central/search?q=html%7Cselect%3Anot(%5Bsize%5D)%3Anot(%5Bmultiple%5D)&path=

Let me know if I can be of any help!

Comment 4

[Aditya Bharti [:adbugger]]

Thanks for the tip. 
So I tried using this add-on to switch to RTL mode without changing language:
https://addons.mozilla.org/en-US/firefox/addon/force-rtl/

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?

Comment 5

[magicp]

(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';"

Comment 6

[Aditya Bharti [:adbugger]]

Attachment: Patch file v1

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?

Comment 7

[Chris H-C :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: https://searchfox.org/mozilla-central/search?q=%3Cselect&case=false&regexp=false&path=.xhtml

One example is the "Add an autofill credit card dialog" which you can reach through about:preferences > Privacy & Security > Saved Credit Cards... > Add

Comment 8

[Chris H-C :chutten]

Comment on attachment 8928461 [details] [diff] [review]
Patch file v1

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

::: toolkit/themes/shared/in-content/common.inc.css
@@ +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:

html|select:not([size]):not([multiple]):dir(rtl)

Comment 9

[Aditya Bharti [:adbugger]]

Ah, that would explain it. I was leaving the tests to run and shifting to other applications.
I will change the selector in the common.inc.css file.

Comment 10

[Aditya Bharti [:adbugger]]

Attachment: add_new_credit_card.png

Comment 11

[Aditya Bharti [:adbugger]]

Attachment: add_new_address.png

Comment 12

[Aditya Bharti [:adbugger]]

Attachment: telemetry.png

Comment 13

[Aditya Bharti [:adbugger]]

Attachment: Patch file v2: changed selector

Changed selector in common.inc.css. Screenshots attached. Seems fine. Any other suggestions regarding pages to test?

Comment 14

[Aditya Bharti [:adbugger]]

Attachment: Changed selector. Fixed additional places in UI.

Output of export. Patch file complete with appropriate fields.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/696538512fd9
Fix in-content HTML selects and about:telemetry "Raw JSON" element for RTL locales r=chutten

Comment 16

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/696538512fd9

Comment 17

[Chris H-C :chutten]

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

Comment 18

[Gerry Chang [:gchang]]

Comment on attachment 8928639 [details] [diff] [review]
Changed selector. Fixed additional places in UI.

Fix about:telemetry in RTL locales. Beta58+.

Comment 19

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1703a61d3d22

Comment 20

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

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.

Comment 21

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

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.