2049244 - Split view "active" outline doesn't go around DevTools toolbox when it is docked to a side

Status: RESOLVED FIXED

(DevTools :: General, defect, P2)

Description

[Nicolas Chevobbe [:nchevobbe][PTO back September 7]]

Attachment: bottom vs side docking

When split view is enabled:

• if the devtools toolbox is docked to the bottom, the active split view outline goes around the toolbox as well
• if the devtools toolbox is docked to the side, the active split view outline only goes around the tab

Note that if a view was "inactive", interactive with its associated toolbox will mark the tab as active when the toolbox is docked to the bottom, but not when it's docked to the side

Comment 1

[Julian Descottes [:jdescottes]]

Sending to split view, probably have more expertise than devtools team on how to fix this issue?

Comment 2

[Alexandre Poirot [:ochameau]]

I'm wondering if bug 2013120's approach I submitted and https://phabricator.services.mozilla.com/D281795 may help? I know we went for simple css fix for the dialog issue, but ultimately the DOM hierarchy around dialog and devtools are not malleable enough.

Comment 3

[BugBot [:suhaib / :marco]]

The severity field is not set for this bug.
:sfoster, could you have a look please?

For more information, please visit BugBot documentation.

Comment 4

[Sam Foster [:sfoster] (he/him)]

(In reply to Julian Descottes [:jdescottes] from comment #1)

Sending to split view, probably have more expertise than devtools team on how to fix this issue?

I'm sure this is a function of the markup and how devtools' <browser> gets placed. When docked to the bottom, the devtools-toolbox browser is a sibling to the content's browser and shares the .browserContainer parent. That's the element we draw the outline on. When devtools are docked to the right, the element is a sibling of the .browserContainer.

The .browserContainer rules are complicated by the rounded corner UX requirement we had - we have to step a little carefully to avoid regressing how the outline, corners and clipping works. I don't know all the history here, but this element is a vbox, and I assume if that's why the side-docked devtools-toolbox gets moved out of there to become a child of the hbox#panel-n element. I'm not sure off the top of my head what else gets inserted into the .browserContainer which relies on the flex-direction. Find bar maybe? I think there's a prompt that can show up above the <browser>. Those preclude simply switching the flex-direction as-needed when updating the docked position of the devtools-toolbox.

So I don't see a quick fix here without adding yet another container element around the stack.browserStack. I'm interested in any better ideas.

I hope that helps. I'm worried this will end up sorting pretty low on the split-view/productivity team priorities list so I'll send it back for your triage. If you do need help, reviews etc. please need-info me or find the team on matrix/slack.

Comment 5

[Nicolas Chevobbe [:nchevobbe][PTO back September 7]]

I tried turning .browserContainer into a grid (D311100) and layout wise, it seems to be fine, but the <splitter> only seems to work within a Flex container, so that's not great (a dead end?)

Comment 6

[Nicolas Chevobbe [:nchevobbe][PTO back September 7]]

Here's the current markup when notification and "search in page" elements are displayed:

Devtools docked to bottom:

<hbox class="browserSidebarContainer">
  <vbox class="browserContainer">
    <vbox class="notificationbox-stack"></vbox>
    <stack class="browserStack"></stack>
    <findbar></findbar>
    <splitter class="devtools-horizontal-splitter"></splitter>
    <browser class="devtools-toolbox-bottom-iframe">
  </vbox>
</hbox>

Devtools docked to left:

<hbox class="browserSidebarContainer">
  <browser class="devtools-toolbox-side-iframe">
  <splitter class="devtools-side-splitter"></splitter>
  <vbox class="browserContainer">
    <vbox class="notificationbox-stack"></vbox>
    <stack class="browserStack"></stack>
    <findbar></findbar>
  </vbox>
</hbox>

Devtools docked to right:

<hbox class="browserSidebarContainer">
  <vbox class="browserContainer">
    <vbox class="notificationbox-stack"></vbox>
    <stack class="browserStack"></stack>
    <findbar></findbar>
  </vbox>
  <splitter class="devtools-side-splitter"></splitter>
  <browser class="devtools-toolbox-side-iframe">
</hbox>

As Sam hinted, if we try to always put the DevTools <browser> in .browserContainer and control its flex-direction, then the notification and the findbar are not properly displayed.

Comment 7

[Nicolas Chevobbe [:nchevobbe][PTO back September 7]]

I was thinking of moving the toolbox inside .browserStack, but here we can have <split-view-footer> as a sibling, which is absolutely positioned within .browserStack, so we'd need to change its position to avoid it overlapping the toolbox (not sure it's great).

What felt the cleanest so far was to always have the toolbox in .browserContainer that would be turned into a grid (because then the layout is quite explicit and easier to work with)
I'll try making the <splitter> to work in such case

Comment 8

[Nicolas Chevobbe [:nchevobbe][PTO back September 7]]

Attachment: Bug 2049244 - Make <splitter> handle items in Grid container. r=emilio.

Comment 9

[Nicolas Chevobbe [:nchevobbe][PTO back September 7]]

Attachment: Bug 2049244 - Turn .browserContainer into a CSS grid. r=#devtools,#tabbrowser-reviewers.

This makes the element layout more explicit and easier to work with.

Comment 10

[Nicolas Chevobbe [:nchevobbe][PTO back September 7]]

Attachment: Bug 2049244 - [devtools] Small tabbox refactor. r=#devtools.

Use Element#append and Element#remove instead of appendChild/removeChild

Comment 11

[Phabricator Automation]

Comment on attachment 9609113 [details]
Bug 2049244 - [devtools] Small tabbox refactor. r=#devtools.

Revision D312252 was moved to bug 2055840. Setting attachment 9609113 [details] to obsolete.

Comment 12

[Phabricator Automation]

Comment on attachment 9609111 [details]
Bug 2049244 - Make <splitter> handle items in Grid container. r=emilio.

Revision D312251 was moved to bug 2057178. Setting attachment 9609111 [details] to obsolete.

Comment 13

[Sam Foster [:sfoster] (he/him)]

Attachment: devtools-splitview-screenshot.png

Looks great. We've got devtools panel docked on the side and bottom in a splitview, with the screenshots button panel correctly positioned and floated on top!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/47ff72653768
https://hg.mozilla.org/integration/autoland/rev/d2ba01e204b4
Turn .browserContainer into a CSS grid. r=desktop-theme-reviewers,tabbrowser-reviewers,devtools-reviewers,sfoster,ochameau,dao,jdescottes.

Comment 15

[agoloman]

Backed out for causing dt failures.

• Backout link
• Push with failures
• Failure Log

Comment 16

[Nicolas Chevobbe [:nchevobbe][PTO back September 7]]

Looks like Nova was enabled after I push to TRY, and some tests are failing due to the new border size

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/b58cc53229fa
https://hg.mozilla.org/integration/autoland/rev/c3c8adca1893
Turn .browserContainer into a CSS grid. r=desktop-theme-reviewers,tabbrowser-reviewers,devtools-reviewers,sfoster,ochameau,dao,jdescottes.

Comment 18

[rperta]

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