Closed Bug 971959 Opened 10 years ago Closed 10 years ago

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

Categories

(DevTools :: Inspector, defect)

30 Branch
defect
Not set
normal

Tracking

(firefox28 affected, firefox29 affected, firefox30- affected, firefox34 verified)

VERIFIED FIXED
Firefox 34
Tracking Status
firefox28 --- affected
firefox29 --- affected
firefox30 - affected
firefox34 --- verified

People

(Reporter: danemacmillan, Assigned: kemenaran, Mentored)

References

Details

(Whiteboard: [lang=html][good first bug][lang=css])

Attachments

(3 files, 7 obsolete files)

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.
Version: unspecified → 30 Branch
Also true in Firefox 28 and 29.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
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.
Flags: needinfo?(dcamp)
Blocking the larger theme bug with this.  Sounds like it is an issue with all of the sidebar tabs (including network panel).
Blocks: 916766
Flags: needinfo?(dcamp)
OS: Mac OS X → All
Hardware: x86_64 → All
Summary: Resizing Inspector's right panel to fill all available horizontal space results in breadcrumb z-index issue. → DevTools Themes: Resizing Inspector's right panel to fill all available horizontal space results in breadcrumb z-index issue.
Based on Feedback from DevTools on IRC we won't be tracking this.
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.
(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?
(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.
Whiteboard: [mentor=bgrins][lang=html][good first bug][lang=css]
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.
Is there any way I could develop with github (since it has a client app) ? The Mercurial stuff is complicated to me.
(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.
Assignee: nobody → ntim007
Status: NEW → ASSIGNED
Attachment #8395281 - Flags: review?(bgrinstead)
Comment on attachment 8395281 [details] [diff] [review]
Patch 1 : Inspector and NetMonitor

Canceling review, to address IRC comments.
Attachment #8395281 - Flags: review?(bgrinstead)
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #8395281 - Attachment is obsolete: true
Attachment #8396522 - Flags: review?(bgrinstead)
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
Attachment #8396522 - Flags: review?(bgrinstead)
(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.
Attachment #8396522 - Attachment is obsolete: true
Attachment #8395281 - Attachment description: bug_971959.patch → Patch 1 : Inspector and NetMonitor
Attachment #8395281 - Attachment is obsolete: false
Attachment #8395281 - Flags: review?(bgrinstead)
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.
Attachment #8395281 - Flags: review?(bgrinstead)
Assignee: ntim007 → nobody
Status: ASSIGNED → NEW
Mentor: bgrinstead
Whiteboard: [mentor=bgrins][lang=html][good first bug][lang=css] → [lang=html][good first bug][lang=css]
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 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 ?
Attachment #8444075 - Flags: review?(bgrinstead)
Attachment #8395281 - Attachment is obsolete: true
Assignee: nobody → kemenaran
Status: NEW → ASSIGNED
Attachment #8444075 - Attachment is obsolete: true
Attachment #8444075 - Flags: review?(bgrinstead)
Thanks for your comments Tim. Here is a new version of the patch that applies the .devtools-main-content class on all tools.
(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.
Flags: needinfo?(bgrinstead)
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.
Flags: needinfo?(bgrinstead)
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.
Attachment #8444100 - Attachment is obsolete: true
Attachment #8446624 - Flags: review?(bgrinstead)
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
Attachment #8446624 - Flags: review?(bgrinstead)
(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.
(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
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
Attachment #8446624 - Attachment is obsolete: true
Attachment #8447663 - Flags: review?(bgrinstead)
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
Attachment #8447663 - Flags: review?(bgrinstead)
How smart from me ; I forgot to commit some files. Sorry for this :)
Attachment #8447663 - Attachment is obsolete: true
Attachment #8448252 - Flags: review?(bgrinstead)
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) */
Attachment #8448252 - Flags: review?(bgrinstead)
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.
Flags: needinfo?(kemenaran)
Sure; here is a new patch that includes the suggestions from Comment 33.
Attachment #8448252 - Attachment is obsolete: true
Flags: needinfo?(kemenaran)
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
Attachment #8466767 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/fe004e9051ab
Keywords: checkin-needed
Whiteboard: [lang=html][good first bug][lang=css] → [lang=html][good first bug][lang=css][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/fe004e9051ab
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [lang=html][good first bug][lang=css][fixed-in-fx-team] → [lang=html][good first bug][lang=css]
Target Milestone: --- → Firefox 34
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.
QA Whiteboard: [qa+]
Already verified by Karthikeyan in comment 39.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa+] → [qa!]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.