1543093 - [RTL] Browser Toolbox entire layout is reversed from the normal DevTools

Status: VERIFIED FIXED

(DevTools :: General, defect, P3)

Description

[Itiel]

See title.
The Browser Toolbox (Ctrl+Alt+Shift+I) layout should be the same as the normal DevTools (Ctrl+Shift+C) for RTL locales.

Browser Toolbox items that should be RTL'd:

1. Inspector
2. Debugger
3. Memory
4. Network
5. DOM
6. Accessibility
7. DevTools settings page
8. Re-ordering of the first and last tab in the DevTools is broken (the right one is draggable only to the right, and the left one only to the left)
9. Tooltips (of the main buttons of the UI, at least. Some tooltips of actual debugging stuff should remain LTR). For example, Accessibility's tab tooltip

See screenshots for general comparison.

Comment 1

[Itiel]

Attachment: Inspector - Browser Toolbox

Comment 2

[Itiel]

Attachment: Inspector - DevTools

Comment 3

[Itiel]

Attachment: Memory - Browser Toolbox

Comment 4

[Itiel]

Attachment: Memory - DevTools

Comment 5

[Julien Wajsberg [:julienw]]

It took me some time to understand the problem so let me rephrase it here: in the browser toolbox, the panels' contents aren't reversed as they correctly are in the in-browser devtools interface.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:pbro, could you have a look please?

Comment 7

[Patrick Brosset <:pbro>]

Hey Julian, do you think this has anything to do with the host window types we use? I know we've done some work on loading our panels in content iframes, which sounded like it might have an impact on how RTL was handled.

To be very clear, here's what's happening using an RTL version of Firefox:

• in the normal toolbox, the inspector is displayed in RTL mode (the 3 panels are displayed in this order: Layout|Rules|Markup, and various text fields and buttons are RTL too)
• in the browser toolbox, the inspector displays the 3 panels in this order instead Markup|Rules|Layout and nothing is RTL'd really.

Comment 8

[Julian Descottes [:jdescottes]]

We have not landed any of the changes to the frame type.

Browser toolbox launches a separate Firefox instance (and uses a separate profile).
My guess is that the following method doesn't detect RTL in this special instance of Firefox:

  loader.lazyGetter(this, "direction", () => {
    // Get the direction from browser.xul document
    const top = this.topWindow;
    const topDocEl = top.document.documentElement;
    const isRtl = top.getComputedStyle(topDocEl).direction === "rtl";
    return isRtl ? "rtl" : "ltr";
  });

searchfox

Comment 9

[Julian Descottes [:jdescottes]]

If you want to test this locally, you can try to force pref("intl.uidirection", 1); in modules/libpref/init/all.js to force your build to use RTL.

I added logs in loader.lazyGetter(this, "direction",. As suspected it returns a different value for the BrowserToolbox than for the regular toolbox.

We use a dedicated profile for the Browser Toolbox. Therefore I checked the value of the preference "intl.uidirection". But it is correctly set even in the BrowserToolbox profile. For some reason the topmost window of the Browser Toolbox is not "rtl". But I don't know why. The process for the BrowserToolbox is started at devtools/client/framework/ToolboxProcess.js

The method here uses the "top" window of the toolbox. If instead we use the actual toolbox window "rtl" will work on the Browser Toolbox:

  loader.lazyGetter(this, "direction", () => {
    const isRtl = this.win.getComputedStyle(this.doc.documentElement).direction === "rtl";
    return isRtl ? "rtl" : "ltr";
  });

We started using the topmost window to compute "direction" as early as Bug 1305007. I don't think there was a strong reason to pick this one rather than the toolbox window. Let's change it to use the toolbox window.

Note on the priority, I think it should rather be P3. Browser Toolbox is normally not used for anything except Firefox development. Extension development was the last external use case for the Browser Toolbox, but we switched it to regular toolboxes in FF68. It's not a recent regression (as I said, probably here since Bug 1305007).

Comment 10

[Patrick Brosset <:pbro>]

Thank you so much Julian for the investigation and describing a solution here.
I'll change this to P3 as you suggested. I think that makes sense.
I'm going to mark you as mentor on this bug in order to get it on some medium time contributor's radars, but feel free to reset that flag if you don't want or have the bandwidth to mentor.

Comment 11

[Julian Descottes [:jdescottes]]

Attachment: Bug 1543093 - Fix RTL for DevTools in Browser Toolbox;r=nchevobbe

Using the toolbox window to read the "direction" seems to fix the issue.
Added a mochitest, had to create a dedicated browser ini file in order to setup RTL properly.

Comment 12

[Julian Descottes [:jdescottes]]

(sorry, started writing a patch afterall :) )

Comment 13

[Julian Descottes [:jdescottes]]

try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=164a7969d79d330317edf55d20c1e3bc5abab39f

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b2bb8867a1b5
Fix RTL for DevTools in Browser Toolbox;r=nchevobbe

Comment 15

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/b2bb8867a1b5

Comment 16

[Itiel]

Pretty much perfect, thank you!