JSON viewer should not be opened automatically when opening about:telemetry in RTL locales

RESOLVED FIXED in Firefox 59

Status

()

P1
normal
RESOLVED FIXED
a year ago
6 months ago

People

(Reporter: magicp.jp, Assigned: adbugger)

Tracking

Trunk
mozilla59
Points:
---

Firefox Tracking Flags

(firefox56 wontfix, firefox57 wontfix, firefox58 wontfix, firefox59 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
Steps to reproduce:
1. Start Firefox in RTL locales (e.g. ar)
2. Open "about:telemetry"

Actual Results:
JSON viewer will be opened automatically.

Expected Results:
JSON viewer should not be opened automatically in RTL locales.
(Reporter)

Updated

a year ago
Has STR: --- → yes
status-firefox56: --- → wontfix
status-firefox57: --- → affected
status-firefox57: affected → wontfix
status-firefox58: affected → fix-optional
Priority: -- → P2
Priority: P2 → P1
(Assignee)

Comment 1

a year ago
Hi,
I would like to work on this bug. Can you tell how to start Firefox in RTL mode? For now, I am doing these two things:
1. Changing intl.uidirection = 1 in about:config
2. Going to about:telemetry and typing document.body.dir = 'rtl' in the console.

While everything is in RTL layout, I am unable to reproduce the error. I suspect it only happens when you change the locale before starting up? Could you tell me how to emulate that behaviour without changing the language, since I don't know any language that requires RTL?

Also, in general, how do I change the locale in Nightly? I worked on a similar bug recently and needed to do the same.
Flags: needinfo?(magicp.jp)
https://dxr.mozilla.org/mozilla-central/source/toolkit/content/aboutTelemetry.js#1154 is the code, and the error message is:

hgram.buckets is undefined

Tested on an Arabic mac Nightly.
Flags: needinfo?(magicp.jp)
(Assignee)

Comment 3

a year ago
Created attachment 8928930 [details]
Histogram Object Structure

The hgram object has no property 'buckets'. hgram.values exists and hgram.values.reverse() does not throw any errors. For now, simply commenting out the 'hgram.buckets.reverse()' line works. 

I understand that the values in a histogram need to be reversed for RTL modes but I also think that the buckets is a remnant from a previous version of the code with a specific function. What is that line supposed to do? The buckets property is not mentioned anywhere else in aboutTelemetry.js.
Flags: needinfo?(gfritzsche)
(Assignee)

Comment 4

a year ago
Created attachment 8928956 [details] [diff] [review]
hgram.buckets not defined. Deleting line.

Alright, so according to :Dexter from #telemetry, this is indeed a remnant from older versions when histograms had a 'bucket' field. That was refactored in bug 112480: https://bugzilla.mozilla.org/show_bug.cgi?id=1122480.

The appropriate fix for this seems to be deleting the 'hgram.buckets.reverse()' line. Uploading initial patch. Please take a look.
Flags: needinfo?(gfritzsche)
Do the histograms displayed by about:telemetry have the low-value buckets on the right with your change?
Assignee: nobody → adibhar97
Status: NEW → ASSIGNED
Flags: needinfo?(adibhar97)
(Assignee)

Comment 6

a year ago
Created attachment 8929035 [details]
Lower indexed buckets on the right

Yes, it looks like the lower valued buckets are on the right with my change. Screenshot attached.
Flags: needinfo?(adibhar97) → needinfo?(chutten)
Excellent! Form this into a commit with a good message and I'll give it a review.
Flags: needinfo?(chutten)
(Assignee)

Comment 8

a year ago
Created attachment 8929042 [details] [diff] [review]
Removed call to hgram.buckets.reverse().

Submitting patch for review.
Attachment #8928956 - Attachment is obsolete: true
Attachment #8929042 - Flags: review?(chutten)
Comment on attachment 8929042 [details] [diff] [review]
Removed call to hgram.buckets.reverse().

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

Looks good to me!
Attachment #8929042 - Flags: review?(chutten) → review+
Keywords: checkin-needed

Comment 10

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8ea0fdcd066
Fix about:telemetry page in RTL locales opens JSON viewer. r=chutten
Keywords: checkin-needed

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f8ea0fdcd066
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
I have reprodcued this bug with Nightly 58.0a1 (2017-11-05) -  AR on Windows 10, 6 Bit!

This bug's fix is verified with Latest Latest Nightly (Arabic build)!

Build ID  : 20171201100115
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0
QA Whiteboard: [bugday-20171129]
status-firefox58: fix-optional → wontfix
You need to log in before you can comment on or make changes to this bug.