Closed
Bug 1407562
Opened 7 years ago
Closed 7 years ago
Fix about:telemetry in RTL locales
Categories
(Toolkit :: Telemetry, defect, P3)
Toolkit
Telemetry
Tracking
()
VERIFIED
FIXED
mozilla59
People
(Reporter: magicp.jp, Assigned: adbugger, Mentored)
References
Details
(Whiteboard: [good first bug][lang=css])
Attachments
(5 files, 2 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
chutten
:
review+
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•7 years ago
|
||
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)
Assignee | ||
Comment 2•7 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)
Comment 3•7 years ago
|
||
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•7 years ago
|
Assignee: nobody → adibhar97
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•7 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?
(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•7 years ago
|
||
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)
Comment 7•7 years ago
|
||
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®exp=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 8•7 years ago
|
||
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•7 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•7 years ago
|
||
Assignee | ||
Comment 11•7 years ago
|
||
Assignee | ||
Comment 12•7 years ago
|
||
Assignee | ||
Comment 13•7 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•7 years ago
|
||
Output of export. Patch file complete with appropriate fields.
Attachment #8928631 -
Attachment is obsolete: true
Attachment #8928639 -
Flags: review?(chutten)
Updated•7 years ago
|
Attachment #8928639 -
Flags: review?(chutten) → review+
Updated•7 years ago
|
Keywords: checkin-needed
Comment 15•7 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•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment 17•7 years ago
|
||
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 18•7 years ago
|
||
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+
Comment 19•7 years ago
|
||
bugherder uplift |
Updated•7 years ago
|
Flags: qe-verify+
Comment 20•7 years ago
|
||
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+
Comment 21•7 years ago
|
||
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.
Description
•