971959 - DevTools Themes: Resizing Inspector's right panel to fill all available horizontal space results in breadcrumb z-index issue.

Status: VERIFIED FIXED

(DevTools :: Inspector, defect)

Description

[Dane MacMillan [:danemacmillan]]

Attachment: firefox-devtools-zindex-bug.png

Resizing the Inspector's right panel so it assumes the full width of the Developer Tools does not properly hide the Inspector's node breadcrumb arrows. The arrows should have a lower z-index and disappear when the right panel is resized to become full width.

An image with the described behaviour is attached.

Comment 1

[Liz Henry (:lizzard) (relman/hg->git project)]

Also true in Firefox 28 and 29.

Comment 2

[Dane MacMillan [:danemacmillan]]

Since bug 899727 has been worked on, and the Network's right panel can now be resized beyond the original constraint, I resized it to full width, and the panel icon also appears over the Headers tab, instead of disappearing. If the purpose was to keep it visible, it's not actually clickable.

Here's an image of the issue:

http://i.imgur.com/pP5ENOi.png

Comment 3

[Benjamin Kerensa [:bkerensa]]

Dave, is there someone we can get assigned to this? I don't know that we will want to track this and potentially block release on it but would love to get your thoughts.

Comment 4

[Brian Grinstead [:bgrins]]

Blocking the larger theme bug with this.  Sounds like it is an issue with all of the sidebar tabs (including network panel).

Comment 5

[Benjamin Kerensa [:bkerensa]]

Based on Feedback from DevTools on IRC we won't be tracking this.

Comment 6

[Brian Grinstead [:bgrins]]

There should probably be a minimum width on either of the boxes to the side of the splitter.  On the network panel in particular, you can resize the details pane to full width which makes it hard to shrink it again.  If the timeline pane never was allowed to shrink below 50px or so, it would solve this problem and the issue in comment 2 as well.

Comment 7

[Benjamin Kerensa [:bkerensa]]

(In reply to Brian Grinstead [:bgrins] from comment #6)
> There should probably be a minimum width on either of the boxes to the side
> of the splitter.  On the network panel in particular, you can resize the
> details pane to full width which makes it hard to shrink it again.  If the
> timeline pane never was allowed to shrink below 50px or so, it would solve
> this problem and the issue in comment 2 as well.

Might we consider adding  [good first bug] to whiteboard and mentoring this bug?

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Benjamin Kerensa [:bkerensa] from comment #7)
> (In reply to Brian Grinstead [:bgrins] from comment #6)
> > There should probably be a minimum width on either of the boxes to the side
> > of the splitter.  On the network panel in particular, you can resize the
> > details pane to full width which makes it hard to shrink it again.  If the
> > timeline pane never was allowed to shrink below 50px or so, it would solve
> > this problem and the issue in comment 2 as well.
> 
> Might we consider adding  [good first bug] to whiteboard and mentoring this
> bug?

Sure, I think this would be a good first bug.

Comment 9

[Brian Grinstead [:bgrins]]

In this case we are talking about two elements in particular:

1) This vbox: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/inspector.xul#77

2) The #network-table element: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/netmonitor.xul#51

What I would do is add a class on both of these elements called "devtools-side-splitter-sibling".

Then inside of widgets.inc.css (https://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/widgets.inc.css), I would add a rule for this class with min-width: 50px; or so.  This way if we needed to do the same thing on other panels we could  apply this class to the sibling of the splitter.

Comment 10

[Tim Nguyen :ntim]

Is there any way I could develop with github (since it has a client app) ? The Mercurial stuff is complicated to me.

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen [:ntim] from comment #10)
> Is there any way I could develop with github (since it has a client app) ?
> The Mercurial stuff is complicated to me.

Yes, I believe so.  Take a look at https://developer.mozilla.org/en-US/docs/Git and https://etherpad.mozilla.org/moz-git-tools, specifically the 'Exporting patches for bugzilla' section in the etherpad.

Comment 12

[Tim Nguyen :ntim]

Attachment: Patch 1 : Inspector and NetMonitor

Comment 13

[Tim Nguyen :ntim]

Comment on attachment 8395281 [details] [diff] [review]
Patch 1 : Inspector and NetMonitor

Canceling review, to address IRC comments.

Comment 14

[Tim Nguyen :ntim]

Attachment: Patch v2

Comment 15

[Brian Grinstead [:bgrins]]

Comment on attachment 8396522 [details] [diff] [review]
Patch v2

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

This is working in all the panels except for webconsole and debugger.  I haven't had time to dig into exactly why, but I think maybe webconsole.xul just needs to include <?xml-stylesheet href="chrome://browser/content/devtools/widgets.css" type="text/css"?>, and debugger seems to have something going on with the variables/events panel to the right

Comment 16

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #15)
> Comment on attachment 8396522 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 8396522 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is working in all the panels except for webconsole and debugger.  I
> haven't had time to dig into exactly why, but I think maybe webconsole.xul
> just needs to include <?xml-stylesheet
> href="chrome://browser/content/devtools/widgets.css" type="text/css"?>, and
> debugger seems to have something going on with the variables/events panel to
> the right

We can land patch v1 which takes care of just the inspector and netmonitor. Then use leave-open for the other tools.

Comment 17

[Brian Grinstead [:bgrins]]

Comment on attachment 8395281 [details] [diff] [review]
Patch 1 : Inspector and NetMonitor

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

I think we can get it working on the webconsole easily (just by adding the css reference) and it may take a little bit of digging, but I'm guessing debugger shouldn't be too hard either.  As we discussed earlier, with the class name 'devtools-main-content' we should try to cover as many tools as possible.

Comment 18

[Pierre de La Morinerie]

Attachment: Fix z-ordering of DevTools toolbar buttons

Comment 19

[Pierre de La Morinerie]

Attachment: Screenshot of issues fixed by the z-ordering patch

Comment 20

[Pierre de La Morinerie]

I looked into this: it happens on all non-opaque toolbar items. The opacity changes the stacking context, and makes the item bleed through the split-view content.

We could simply increase the min-width of all split-views, but it won't prevent other translucent elements to show the same issue. Instead here is a patch that isolates the stacking context of split-bar views, so that the same issue is fixed for all toolbars varieties and length.

Comment 21

[Tim Nguyen :ntim]

Comment on attachment 8444075 [details] [diff] [review]
Fix z-ordering of DevTools toolbar buttons

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

Code looks good. But as Brian mentioned earlier, you'll need to apply the .devtools-main-content class on all tools. If that's an issue, you can just use an ID selector on the inspector vbox. Brian, what do you think ?

Comment 22

[Pierre de La Morinerie]

Attachment: v2 - Fix z-ordering of DevTools toolbar buttons.

Comment 23

[Pierre de La Morinerie]

Thanks for your comments Tim. Here is a new version of the patch that applies the .devtools-main-content class on all tools.

Comment 24

[Tim Nguyen :ntim]

(In reply to Pierre de LA MORINERIE from comment #22)
> Created attachment 8444100 [details] [diff] [review]
> v2 - Fix z-ordering of DevTools toolbar buttons.

You're applying the class on the correct elements only on the web audio editor, the inspector, and the network monitor. Also, you should do the same on the web console (and include widgets.inc.css as Brian mentioned in some early comments). See attachment 8396522 [details] [diff] [review] for more guidance (note that due to the oldness to the patch, I haven't applied the class in the canvas debugger).

I think in this case, it might be better to just add an id on the inspector vbox and add some extra css for it, but Brian might have a better idea what to do here.

Comment 25

[Brian Grinstead [:bgrins]]

I think there are two different fixes going on here:

1) The devtools-main-content class, which was meant to sort of work around this problem *and* prevent the main region in a tool from being shrunk to 0px
2) The better workaround that you found using position:relative on the sidebar tabs / split view

Let's keep both of them, but try not to combine them too much.  So first, keep the devtools-main-content on the main areas only as in Attachment 8396522 [details] [diff].  Second, add in the min-width rule `.devtools-main-content { min-width: 50px; }`.

I think once those are done, it should actually be OK to keep the position:relative on devtools-main-content as that will include the inspector tab (which is a special case for now where we need the relative positioning on a non sidebar tab).  But I'll double check after applying the updated patch.

Comment 26

[Pierre de La Morinerie]

Attachment: Fix z-ordering of DevTools toolbar buttons.

Here is a new patch that:

* Add the devtools-main-content class on all main areas
* Ensure that all side areas have the (already existing) devtools-sidebar-tabs class
* Set both devtools-main-content and devtools-sidebar-tabs to position:relative and min-width:50px

The reason we need 'min-width:50px' for both main and side content is that we want to restrict the width of the content on the left side.
But the side content may be on the left (debugger, style editor) or on the right (inspector, web-console).
So we set a default minimum width to both left and right content - anyway many tabs will override this value with a more specific (and larger) one.

Comment 27

[Brian Grinstead [:bgrins]]

Comment on attachment 8446624 [details] [diff] [review]
Fix z-ordering of DevTools toolbar buttons.

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

Looks good, but I'd like to avoid repurposing the devtools-sidebar-tabs class and instead add a new explicit class for this

::: browser/devtools/canvasdebugger/canvasdebugger.xul
@@ +16,5 @@
>    <script src="chrome://browser/content/devtools/theme-switching.js"/>
>    <script type="application/javascript" src="canvasdebugger.js"/>
>  
>    <hbox class="theme-body" flex="1">
> +    <vbox id="snapshots-pane" class="devtools-sidebar-tabs">

I don't like reusing the devtools-sidebar-tabs class on non-tabs elements.  Maybe we should have a devtools-side-content class that is put on the boxes (siblings to devtools-main-content).

This may require adding a container box for certain panels that actually have sidebar-tabs (like inspector), but for others like this you can just switch the newly added class name.

::: browser/devtools/netmonitor/netmonitor.xul
@@ +50,5 @@
>      <vbox id="network-inspector-view" flex="1">
>        <hbox id="network-table-and-sidebar"
>              class="devtools-responsive-container"
>              flex="1">
> +        <vbox id="network-table" flex="1" class="devtools-main-content">

In netmonitor, you should be able to add the devtools-side-content to #details-pane, for instance

::: browser/devtools/profiler/profiler.xul
@@ +21,5 @@
>            src="chrome://browser/content/devtools/theme-switching.js"/>
>    <script type="text/javascript" src="sidebar.js"/>
>    <box flex="1" id="profiler-chrome"
>      class="devtools-responsive-container theme-body">
> +    <vbox class="profiler-sidebar theme-sidebar devtools-sidebar-tabs">

Actually, forget changing this profiler.xul file... it is being removed in Bug 879008

::: browser/themes/shared/devtools/widgets.inc.css
@@ +34,5 @@
>    -moz-box-orient: horizontal;
>  }
>  
> +.devtools-main-content,
> +.devtools-sidebar-tabs {

replace devtools-sidebar-tabs with the new devtools-side-content class

Comment 28

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #27)
> Comment on attachment 8446624 [details] [diff] [review]
> Fix z-ordering of DevTools toolbar buttons.
> 
> Review of attachment 8446624 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, but I'd like to avoid repurposing the devtools-sidebar-tabs
> class and instead add a new explicit class for this
> 
> ::: browser/devtools/canvasdebugger/canvasdebugger.xul
> @@ +16,5 @@
> >    <script src="chrome://browser/content/devtools/theme-switching.js"/>
> >    <script type="application/javascript" src="canvasdebugger.js"/>
> >  
> >    <hbox class="theme-body" flex="1">
> > +    <vbox id="snapshots-pane" class="devtools-sidebar-tabs">
> 
> I don't like reusing the devtools-sidebar-tabs class on non-tabs elements. 
> Maybe we should have a devtools-side-content class that is put on the boxes
> (siblings to devtools-main-content).
> 
> This may require adding a container box for certain panels that actually
> have sidebar-tabs (like inspector), but for others like this you can just
> switch the newly added class name.

Or you could just apply both classes on sidebar tabs.

Comment 29

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen [:ntim] from comment #28)
> (In reply to Brian Grinstead [:bgrins] from comment #27)
> > Comment on attachment 8446624 [details] [diff] [review]
> > Fix z-ordering of DevTools toolbar buttons.
> > 
> > Review of attachment 8446624 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks good, but I'd like to avoid repurposing the devtools-sidebar-tabs
> > class and instead add a new explicit class for this
> > 
> > ::: browser/devtools/canvasdebugger/canvasdebugger.xul
> > @@ +16,5 @@
> > >    <script src="chrome://browser/content/devtools/theme-switching.js"/>
> > >    <script type="application/javascript" src="canvasdebugger.js"/>
> > >  
> > >    <hbox class="theme-body" flex="1">
> > > +    <vbox id="snapshots-pane" class="devtools-sidebar-tabs">
> > 
> > I don't like reusing the devtools-sidebar-tabs class on non-tabs elements. 
> > Maybe we should have a devtools-side-content class that is put on the boxes
> > (siblings to devtools-main-content).
> > 
> > This may require adding a container box for certain panels that actually
> > have sidebar-tabs (like inspector), but for others like this you can just
> > switch the newly added class name.
> 
> Or you could just apply both classes on sidebar tabs.

Yeah, if the sidebar tabs are the only 'side content' then I suppose doing that would require less changes

Comment 30

[Pierre de La Morinerie]

Attachment: v3 - Fix z-ordering of DevTools toolbar buttons.

Thanks for the review and advices. Here is a new version of the patch that:

* adds a 'devtools-side-content' class (instead of reusing the 'devtools-sidebar-tabs' class)
* doesn't change profiler.xul

Comment 31

[Brian Grinstead [:bgrins]]

Comment on attachment 8447663 [details] [diff] [review]
v3 - Fix z-ordering of DevTools toolbar buttons.

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

I think this may not have all the latest changes included, I'm still seeing devtools-sidebar-tabs throughout the changes instead of devtools-side-content, and profiler.xul is still here

Comment 32

[Pierre de La Morinerie]

Attachment: v4 - Fix z-ordering of DevTools toolbar buttons.

How smart from me ; I forgot to commit some files. Sorry for this :)

Comment 33

[Brian Grinstead [:bgrins]]

Comment on attachment 8448252 [details] [diff] [review]
v4 - Fix z-ordering of DevTools toolbar buttons.

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

Sorry, I know this was on my recommendation, but as it turns out setting min-width on some of these side-content elements is not good.  For instance, the debugger sidebar is now collapsible to 50px whereas before it would stop shrinking earlier.  Given this, I'd like to revise the suggestion:

1) Remove the devtools-side-content class altogether
2) Set position: relative on devtools-sidebar-tabs (will fix inspector) and devtools-main-content (will fix style editor).
3) Keep a 50px min-width on devtools-main-content only

This will let us land your fix for the overlapping elements without causing any side effects like I'm seeing with the min-width on side-content.  If there are particular cases where we need to set a min width on a sidebar we can deal with that in the future.

::: browser/themes/shared/devtools/widgets.inc.css
@@ +36,5 @@
>  
> +.devtools-main-content,
> +.devtools-side-content {
> +  min-width: 50px;
> +  /* isolate the stacking contexts of both sides

Please update the comment to be a bit more clear about the problem it is solving. Maybe something like  /* Prevent some children that should be hidden from remaining visible as this is shrunk (Bug 971959) */

Comment 34

[Brian Grinstead [:bgrins]]

Pierre, any chance you'll have time to send over an updated patch for this?  It's pretty much ready to go once the changes from Comment 33 are addressed.

Comment 35

[Pierre de La Morinerie]

Attachment: v5 - Fix z-ordering of DevTools toolbar buttons.

Sure; here is a new patch that includes the suggestions from Comment 33.

Comment 36

[Brian Grinstead [:bgrins]]

Comment on attachment 8466767 [details] [diff] [review]
v5 - Fix z-ordering of DevTools toolbar buttons.

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

Thanks!  Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=3ea05486b75a

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/fe004e9051ab

Comment 38

[Ed Morley [:emorley]]

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

Comment 39

[Karthikeyan]

Verified fixed on Windows 7 32-bit Firefox Nighty 34 [20140813]

Description: 

Sizing sidepane to full width of the screen does properly hide the inspector's node breadcumbs arrows.

Comment 40

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Already verified by Karthikeyan in comment 39.