Fix about:telemetry in RTL locales

VERIFIED FIXED in Firefox 58

Status

()

defect
P3
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: magicp.jp, Assigned: adbugger, Mentored)

Tracking

(Blocks 1 bug)

Trunk
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 verified, firefox59 verified)

Details

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

Attachments

(5 attachments, 2 obsolete attachments)

Reporter

Description

2 years ago
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]
Assignee

Comment 2

2 years ago
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?
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/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!
Flags: needinfo?(chutten)

Updated

2 years ago
Assignee: nobody → adibhar97
Status: NEW → ASSIGNED
Assignee

Comment 4

2 years ago
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?
Reporter

Comment 5

2 years ago
(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';"
Assignee

Comment 6

2 years ago
Posted 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: 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
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/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)
Assignee

Comment 9

2 years ago
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.
Assignee

Comment 10

2 years ago
Assignee

Comment 11

2 years ago
Posted image add_new_address.png
Assignee

Comment 12

2 years ago
Posted image telemetry.png
Assignee

Comment 13

2 years ago
Changed selector in common.inc.css. Screenshots attached. Seems fine. Any other suggestions regarding pages to test?
Attachment #8928461 - Attachment is obsolete: true
Assignee

Comment 14

2 years ago
Output of export. Patch file complete with appropriate fields.
Attachment #8928631 - Attachment is obsolete: true
Attachment #8928639 - Flags: review?(chutten)

Updated

2 years ago
Attachment #8928639 - Flags: review?(chutten) → review+

Updated

2 years ago
Keywords: checkin-needed

Comment 15

2 years ago
Pushed by archaeopteryx@coole-files.de:
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
Keywords: checkin-needed

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/696538512fd9
Status: ASSIGNED → RESOLVED
Closed: 2 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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.