Closed Bug 1594489 Opened 6 years ago Closed 21 days ago

Inspector does not respect panel layout

Categories

(DevTools :: Inspector, enhancement, P2)

71 Branch
enhancement

Tracking

(firefox157 fixed)

RESOLVED FIXED
157 Branch
Tracking Status
firefox157 --- fixed

People

(Reporter: shane, Assigned: oconnerrev)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

Attached image devtools.jpg —

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:71.0) Gecko/20100101 Firefox/71.0

Steps to reproduce:

Dock the inspector to the right with html and css open. They are split vertically (html on top and css below it) - when the panel is resized at a certain width the css splits to the right of the html instead of staying below it.

Actual results:

The html and css did not stay split vertically (html on top and css below it). They instead became to very small columns side by side instead of staying in a single column split horizontally - html on top and css below it.

Expected results:

The HTML should always stay on top and the css should always stay below it.
Toggle Option would be nice to change how panels splits and resize. The vertical 2-column split makes the rules very hard to read and is disorienting when trying to look at a wide view of html and having the css "snap-in" next to it in a column, effectively making the html view smaller and impossible to read.

The image attached shows what is happening (left) and what should happen regardless of devtools width (right).

An option or toggle would be nice ("Lock devtools panel layout" or "Single column with horizontal split panels")

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Component: Untriaged → Inspector
Product: Firefox → DevTools

Thanks for filing. I've heard the same feedback before. It's interesting because the logic right now is such that the inspector tries to adapt to whatever real estate is available and make the most of it. That's why it decide on its own to either stack things vertically or horizontally. Related to this: in 3-panel mode, we're making changes right now so it would switch from 3 rows (if very narrow), to 1 row on top and 2 columns below (if somewhat narrow), all the way to 3 columns (if very wide). But what you're saying is, on contrary, that you do not necessarily want things to change on their own while you are resizing.
And I have to admit I've myself also been frustrated by this when I'm used to a certain layout (say everything in columns) and I just want to make devtools bigger for some time but keep that layout. Well after some point things jump to another layout and I have to get used to the way things look now.
So an option or toggle might indeed make sense.

Martin: maybe some new food for thoughts to consider while we're working on this 3-panel layout.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(mbalfanz)

That's a good idea. We've struggled quite a bit to find a good solution, so giving the user the option to control the layout sounds like a good idea to me 👍

Flags: needinfo?(mbalfanz)
Priority: -- → P2
Severity: normal → S3
Duplicate of this bug: 1806248
Attached image vertical-monitor.png —

me bumped into this as i switched to vertical monitor.

Attached image zoomed-in.png —

i found a temporary solution for now, if i zoom in a few times, the panel moves to the bottom

Hey so this has been bugging me forever, I dove into browser code because of this.

this is the solution:
I am not sure if its okay to put the edited files here so I am writing the snippets needed to make it happen.
It's my first time here so apologies if I am not supposed to do this.

devtools\client\locales\en-US\inspector.properties

# LOCALIZATION NOTE (inspector.splitOrientation.tooltip): Tooltip for the
button that toggles the inspector splitter between landscape and portrait.
inspector.splitOrientation.tooltip=Toggle split orientation

devtools\client\inspector\inspector.js

const THREE_PANE_ENABLED_PREF = "devtools.inspector.three-pane-enabled";
const THREE_PANE_CHROME_ENABLED_PREF =
  "devtools.inspector.chrome.three-pane-enabled";
// Forces the splitter orientation: "landscape", "portrait", or "auto" to
// keep the width-threshold behavior.
const SPLIT_ORIENTATION_PREF = "devtools.inspector.split-orientation";
const DEFAULT_COLOR_UNIT_PREF = "devtools.defaultColorUnit";
  /**
   * Check if the inspector should use the landscape mode.
   *
   * @return {boolean} true if the inspector should be in landscape mode.
   */
  #useLandscapeMode() {
    if (!this.panelDoc) {
      return true;
    }

    const orientation = Services.prefs.getCharPref(
      SPLIT_ORIENTATION_PREF,
      "auto"
    );
    if (orientation === "landscape" || orientation === "portrait") {
      return orientation === "landscape";
    }

    const splitterBox = this.panelDoc.getElementById("inspector-splitter-box");
    const width = splitterBox.clientWidth;
  #onLazyPanelResize = debounce(
    () => {
      // We can be called on a closed window or destroyed toolbox because of the deferred task.
      if (
        this.panelWin?.closed ||
        this.#destroyed ||
        this.#toolbox.currentToolId !== "inspector"
      ) {
        return;
      }

      this.splitBox.setState({ vert: this.#useLandscapeMode() });
      this.emit("inspector-resize");
    },
    LAZY_RESIZE_INTERVAL_MS,
    this
  );

  onSplitOrientationButtonClicked = () => {
    const useLandscape = !this.#useLandscapeMode();
    Services.prefs.setCharPref(
      SPLIT_ORIENTATION_PREF,
      useLandscape ? "landscape" : "portrait"
    );
    this.splitBox.setState({ vert: useLandscape });
    this.splitOrientationButton?.setAttribute("aria-pressed", useLandscape);
  };
  async #setupToolbar() {
    this.#teardownToolbar();

    // Setup the add-node button.
    this.addNodeButton = this.panelDoc.getElementById(
      "inspector-element-add-button"
    );
    this.addNodeButton.addEventListener("click", this.addNode);

    // Setup the split orientation toggle button.
    this.splitOrientationButton = this.panelDoc.getElementById(
      "inspector-split-orientation-toggle"
    );
    this.splitOrientationButton.setAttribute(
      "aria-pressed",
      this.#useLandscapeMode()
    );
    this.splitOrientationButton.addEventListener(
      "click",
      this.onSplitOrientationButtonClicked
    );
  #teardownToolbar() {
    if (this.addNodeButton) {
      this.addNodeButton.removeEventListener("click", this.addNode);
      this.addNodeButton = null;
    }

    if (this.eyeDropperButton) {
      this.eyeDropperButton.removeEventListener(
        "click",
        this.onEyeDropperButtonClicked
      );
      this.eyeDropperButton = null;
    }

    if (this.splitOrientationButton) {
      this.splitOrientationButton.removeEventListener(
        "click",
        this.onSplitOrientationButtonClicked
      );
      this.splitOrientationButton = null;
    }
  }

\devtools\client\inspector\index.xhtml

          <button
            id="inspector-eyedropper-toggle"
            class="devtools-button"
          ></button>
          <button
            id="inspector-split-orientation-toggle"
            class="devtools-button"
            aria-pressed="false"
            data-localization="title=inspector.splitOrientation.tooltip"
          ></button>
        </div>

devtools\client\themes\inspector.css

/* Split orientation toolbar button */

#inspector-split-orientation-toggle::before {
  background-image: url(images/dock-bottom.svg);
  background-position: center;
  background-size: 14px;
}

#inspector-split-orientation-toggle[aria-pressed="true"]::before {
  background-image: url(images/dock-side-right.svg);
}

browser\app\profile\firefox.js

// Enable the 3 pane mode in the inspector
pref("devtools.inspector.three-pane-enabled", true);
// Enable the 3 pane mode in the chrome inspector
pref("devtools.inspector.chrome.three-pane-enabled", false);
// Splitter orientation: "landscape", "portrait", or "auto" (width-based)
pref("devtools.inspector.split-orientation", "auto");
// Collapse pseudo-elements by default in the rule-view
pref("devtools.inspector.show_pseudo_elements", false);
Attached image image.png —

(In reply to Rév O'Conner from comment #8)

Hey so this has been bugging me forever, I dove into browser code because of this.

this is the solution:
I am not sure if its okay to put the edited files here so I am writing the snippets needed to make it happen.
It's my first time here so apologies if I am not supposed to do this.

Hello Rév, thanks a lot for the patch!
Can you submit it following https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html#to-submit-a-patch so we can start the review process ?
Let me know if you run into any issue

Flags: needinfo?(oconnerrev)

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #10)

(In reply to Rév O'Conner from comment #8)

Hey so this has been bugging me forever, I dove into browser code because of this.

this is the solution:
I am not sure if its okay to put the edited files here so I am writing the snippets needed to make it happen.
It's my first time here so apologies if I am not supposed to do this.

Hello Rév, thanks a lot for the patch!
Can you submit it following https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html#to-submit-a-patch so we can start the review process ?
Let me know if you run into any issue

Thanks for responding. I am a bit busy this month, so I expect to hop on it maybe by second week of August. I checked out the process, and seemed straightforward

Flags: needinfo?(oconnerrev)

(In reply to Rév O'Conner from comment #11)

Thanks for responding. I am a bit busy this month, so I expect to hop on it maybe by second week of August. I checked out the process, and seemed straightforward

Hello Rév, I wanted to check if that's something you're still interested in?

Flags: needinfo?(oconnerrev)

Hey! thanks for checking in. I will start on this from Monday, just been busy job hunting. A man has to eat and what not!

Flags: needinfo?(oconnerrev)

(In reply to Rév O'Conner from comment #13)

Hey! thanks for checking in. I will start on this from Monday, just been busy job hunting. A man has to eat and what not!

sure, no worries, I don't want this to become a burden, do it if you want and have the time to, otherwise I'll take over (but I'd likely take inspiration from the investigation you did here, so I want to make sure you have a chance to get proper credit for it by doing the commit yourself :) )

Adds a meatball-style menu to the Inspector toolbar with three radio
options: Automatic (the existing width-based behavior, default), Side by
side, and Stacked. The choice is stored in the new
devtools.inspector.split-orientation pref. A pref observer keeps the
layout and the menu in sync when the pref changes externally.

Also adds the menuitemradio role to the shared menu button styling in
tooltips.css, since MenuItem supports it but the stylesheet only covered
menuitem and menuitemcheckbox.

Assignee: nobody → oconnerrev
Status: NEW → ASSIGNED

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #14)

(In reply to Rév O'Conner from comment #13)

Hey! thanks for checking in. I will start on this from Monday, just been busy job hunting. A man has to eat and what not!

sure, no worries, I don't want this to become a burden, do it if you want and have the time to, otherwise I'll take over (but I'd likely take inspiration from the investigation you did here, so I want to make sure you have a chance to get proper credit for it by doing the commit yourself :) )

Hey, so I had some time on my hand and submitted the fix for this. I had to take some help from AI since I have never worked with phab before, and just overall making sure I was sticking to the process. So if there something I missed or did wrong please let me know. The AI also said to comment on the phabricator page for the bug but I cannot find any comment box or anything over here. Hah! So I am assuming it's just hallucinated that part.

I changed a few things from the code written above, since that was an adaptation from a userchrome.js script I run on my daily use firefox. I realised it would be confusing in terms of UX for anyone else. I adapted the three dot meatball menu on top "Customize developer tools and get help" with an auto which is the current auto layout, and side by side and stacked as menuitems. The styling was done with the help of browser toolbox, hopefully I have no added any duplicate CSS rules here.

I am attaching a screen recording of it in play here https://drive.google.com/file/d/1HVvoad7tCeI9GXgo4WIXzeDigNs_piLu/view?usp=sharing (no idea how to natively attach video here, so bear with me )

The test passed locally for me as well as linting and I ran tests for all areas of the browser touched.
Tests passed:

  • the (NEW, included) browser_inspector_split-orientation.js
  • browser_toolbox_meatball.js,
  • eslint, stylelint and fluent-lint.

@nchevobbe submitted the revision. Do i need to comment here or how does the bugzilla and phabricator link up. A bit confused still about the platforms, please excuse me.

(In reply to Rév O'Conner from comment #17)

@nchevobbe submitted the revision. Do i need to comment here or how does the bugzilla and phabricator link up. A bit confused still about the platforms, please excuse me.

hello Rev, thanks for the update. I'm just back from PTO, so I'll have a look shortly (probably tomorrow)

Pushed by nchevobbe@mozilla.com: https://github.com/mozilla-firefox/firefox/commit/e4dad5ece2c3 https://hg.mozilla.org/integration/autoland/rev/919a86959b91 Add a menu to control the Inspector split orientation. r=nchevobbe,fluent-reviewers,devtools-reviewers,bolsson
Status: ASSIGNED → RESOLVED
Closed: 21 days ago
Resolution: --- → FIXED
Target Milestone: --- → 157 Branch
You need to log in before you can comment on or make changes to this bug.