Closed Bug 1243598 Opened 8 years ago Closed 8 years ago

Inspector sidebar (rule view, font inspector, etc) fills up inspector panel in toolbox the first time it's opened if the content is wide

Categories

(DevTools :: Inspector, defect)

defect
Not set
normal

Tracking

(firefox45 unaffected, firefox46 unaffected, firefox47+ fixed)

VERIFIED FIXED
Firefox 47
Tracking Status
firefox45 --- unaffected
firefox46 --- unaffected
firefox47 + fixed

People

(Reporter: bgrins, Assigned: gl)

References

()

Details

(Keywords: regression)

Attachments

(2 files, 5 obsolete files)

Attached image rule-view.png
STR:
Clean profile
Enable chrome/remote debugging
Open Browser Toolbox

Expected:
Rule view appears as normal

Actual:
Rule view covers up almost all of markup view (see screenshot).  At first I thought that resizing the splitter didn't work but it seems to after all.  If I resize it then close and reopen BT the rule view size is remembered.
I'm blocking Bug 1238133 because it seems quite likely to be related.  Gabe, can you reproduce it?
Blocks: 1238133
Component: Developer Tools: Framework → Developer Tools: Inspector
Flags: needinfo?(gl)
Keywords: regression
I think this cause is the font inspector. font-url and font-css-code are expanded their width by online fonts, then it affects other panels.

STR:
1. Open "about:home"
2. Open the font inspector
3. Click "Show all the fonts used in the page" button
4. Navigate to "bugzilla.mozilla.org"
I can confirm this was introduced by commit 2d2e19e15a1c : https://hg.mozilla.org/mozilla-central/rev/2d2e19e15a1c
Just wanted to note this affects the regular inspector as well as the browser toolbox. 

I do not understand why the layout is so badly screwed up, but maybe we should consider a backout of bug 1238133 until we figure this out?
Assignee: nobody → gl
Flags: needinfo?(gl)
(In reply to Julian Descottes [:jdescottes] from comment #5)
> Just wanted to note this affects the regular inspector as well as the
> browser toolbox. 
> 
> I do not understand why the layout is so badly screwed up, but maybe we
> should consider a backout of bug 1238133 until we figure this out?

We knew there was a chance that Bug 1238133 cause cause some issues, which is why we waited until after the merge day to land so it could bake on nightly for a full release cycle.  I think as long as this is actively being looked into we don't need to back out.
[Tracking Requested - why for this release]: regression
[Tracking Requested - why for this release]: Note that this is only affecting 47 (caused by https://hg.mozilla.org/mozilla-central/rev/2d2e19e15a1c)
We're starting to see multiple mentions of this every day, and people have been filing duplicates. So we should fix this very soon.
Gabriel said he'd look at it today. Gabriel, please un-assign yourself if you don't have the time right now, we need to get this in before the next branch point.
(In reply to Julian Descottes [:jdescottes] from comment #5)
> Just wanted to note this affects the regular inspector as well as the
> browser toolbox. 

I haven't been able to reproduce in a regular inspector (at least on a clean profile).  Maybe it's also related to the page that it's opened in as in Comment 2?
Ah, here's a STR in normal toolbox:

Open http://www.cnn.com/
Open Inspector
Switch to Fonts Panel
(In reply to Julian Descottes [:jdescottes] from comment #5)
> I do not understand why the layout is so badly screwed up
Well probably I do. Until the 1st resize, the sidebar expands itself to display the longest string in sidebar w/o breaking the line. Try these STR:

0. Create new profile
1. Open   data:text/html,<body style="background:linear-gradient(transparent, transparent);"><br>
2. Open devtools -> Inspector
3. Click <body> in markup to focus it
4. Click <br> in markup to focus it

AR:  After Step 3 sidebar is way too wide,
     After Step 4 sidebar is "normal"
ER:  The same width
So on a clean profile no width is set here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/sidebar.js#74.  But the weird thing is even if the width is set (via [width=N]) it doesn't seem to clamp it.
(In reply to Brian Grinstead [:bgrins] from comment #15)
> So on a clean profile no width is set here:
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/
> sidebar.js#74.  But the weird thing is even if the width is set (via
> [width=N]) it doesn't seem to clamp it.
Not sure how this attribute works. But I think we should be setting the default width of the <tabbox> element via CSS anyway. I've had problems in the past where setting tabbox.width to something wouldn't work properly.
I'm guessing we didn't have the problem before with iframes because they have a default width, and "contained" their markup and forced wrapping.
Summary: Rule view fills up window in browser toolbox the first time it's opened → Inspector sidebar (rule view, font inspector, etc) fills up inspector panel in toolbox the first time it's opened if the content is wide
(In reply to arni2033 from comment #14)
> (In reply to Julian Descottes [:jdescottes] from comment #5)
> > I do not understand why the layout is so badly screwed up
> Well probably I do. Until the 1st resize, the sidebar expands itself to
> display the longest string in sidebar w/o breaking the line. Try these STR:
> 
> 0. Create new profile
> 1. Open   data:text/html,<body
> style="background:linear-gradient(transparent, transparent);"><br>
> 2. Open devtools -> Inspector
> 3. Click <body> in markup to focus it
> 4. Click <br> in markup to focus it
> 
> AR:  After Step 3 sidebar is way too wide,
>      After Step 4 sidebar is "normal"
> ER:  The same width

Yeah, that is the easiest way to repro on a clean profile, thanks!  The Font Inspector one in Comment 13 even bites profiles where there was an earlier width set I think because of the pre whitespace.

Even setting width on the inspector-sidebar tabbox doesn't seem to prevent this problem, may be something with css flex or xul flex.
(In reply to Brian Grinstead [:bgrins] from comment #15)
> So on a clean profile no width is set here:
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/
> sidebar.js#74.  But the weird thing is even if the width is set (via
> [width=N]) it doesn't seem to clamp it.

I think we should have a default pref for the inspector sidebar's width, or handle a missing pref in the sidebar and set it to a constant (like 300 or so).  I don't know how it used to determine the width before on a clean profile - I guess from the default width of an <iframe> in UA styles?
Setting overflow: hidden; max-width: 374px; (or whatever the current width on the sidebar is) on the #font-container element seems to fix the font inspector's issue.  Would be nice if we could find a solution that works for all panels.
Attached patch sidebar-1.patch (obsolete) — Splinter Review
This fixes STR in Comment 14
As pointed out on IRC, this also is a problem when editing a long selector i.e. the big one on <body> in https://www.mozilla.org/en-US/firefox/nightly/firstrun/
I'd just like to mention that, differently from comment 0, I definitely cannot resize the rules view through the splitter
Attached patch sidebar-1.patch (obsolete) — Splinter Review
Let's kick off some reviews to at least improve the situation
Attachment #8715492 - Attachment is obsolete: true
Attachment #8715889 - Flags: review?(pbrosset)
Comment on attachment 8715889 [details] [diff] [review]
sidebar-1.patch

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

::: devtools/client/framework/sidebar.js
@@ +13,5 @@
>  var EventEmitter = require("devtools/shared/event-emitter");
>  var Telemetry = require("devtools/client/shared/telemetry");
>  
>  const XULNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> +const DEFAULT_WIDTH = exports.DEFAULT_WIDTH = 300;

If I clear pref devtools.toolsidebar-width.inspector on aurora, open the inspector, and then measure the size of the default sidebar using the browser toolbox, I get 405px. So I think DEFAULT_WIDTH should be 400 rather than 300.
Attachment #8715889 - Flags: review?(pbrosset) → review+
What we need is a way to contain the sidebar content. Iframes do that by default. I think what we'd need is CSS layout containment now (contain: layout): https://drafts.csswg.org/css-containment/#contain-property
Unfortunately, that's not yet implemented in gecko (see bug 1150081). Only contain:paint seems implemented, and it's behind a pref.
This gets us a little closer to layout containment:
- width attribute (as in Brian's patch) and display:block on the <tabbox> element
- width:100% on the <stack> element below that
- width:100% on the <tabpanels> element

Now I'm trying to find a way to have the <tabpanels> element occupy the full available height, which it does not for now (because of the display:block on the parent).

This is very much a hack, but to be honest, the only other solution I can think of right now is to introduce an iframe again.
height: calc(100vh - <height-of-stack>); 
on the <tabpanels> seems to work. But that requires to know the height of the <stack> element.
2em seems to work nicely. But it'd be better to set it to a fixed value instead. And by the way, I think we should, this would get rid of a bug we have where the reporter was complaining about the tabs getting really big when the toolbox closes (bug 1195727).
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #27)
> I think we should, this would get rid of a bug we
> have where the reporter was complaining about the tabs getting really big
> when the toolbox closes (bug 1195727).

That'd be nice
Just to illustrate what I had in mind. This includes Brian's changes too.
I don't think this is the right solution though, but hopefully this gives some food for thought.
With this applied, resizing the sidebar is janky ... not sure why.
Also, the netmonitor has a sidebar too, and it has no iframes. So we should be able to study how things are done there.
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #27)
> height: calc(100vh - <height-of-stack>); 
> on the <tabpanels> seems to work. But that requires to know the height of
> the <stack> element.
> 2em seems to work nicely. But it'd be better to set it to a fixed value
> instead. And by the way, I think we should, this would get rid of a bug we
> have where the reporter was complaining about the tabs getting really big
> when the toolbox closes (bug 1195727).

If we go that route then 24px is the correct size, which matches devtools-toolbar.  With your patch applied with that number it seems to work pretty well but I notice a scrolling issue on Computed panel on http://www.cnn.com/ when scrolling right the zebra striping doesn't continue.
(In reply to Brian Grinstead [:bgrins] from comment #30)
> (In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #27)
> > height: calc(100vh - <height-of-stack>); 
> > on the <tabpanels> seems to work. But that requires to know the height of
> > the <stack> element.
> > 2em seems to work nicely. But it'd be better to set it to a fixed value
> > instead. And by the way, I think we should, this would get rid of a bug we
> > have where the reporter was complaining about the tabs getting really big
> > when the toolbox closes (bug 1195727).
> 
> If we go that route then 24px is the correct size, which matches
> devtools-toolbar.  With your patch applied with that number it seems to work
> pretty well but I notice a scrolling issue on Computed panel on
> http://www.cnn.com/ when scrolling right the zebra striping doesn't continue.

Looks like on Dev Edition we add ellipses to property values wider than the viewport and in this case, which might be fixable
Depends on: 1246010
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #29)
> Also, the netmonitor has a sidebar too, and it has no iframes. So we should
> be able to study how things are done there.

The netmonitor sidebar doesn't use a mix of XUL flexbox and CSS flexbox.
Sadly, moving the tools back into iframes might be the safest option.  We'd want to undo this once the main inspector frame is converted to HTML, so hopefully we could keep some of the changes that landed in bug 1238133 like:
* Having the tool initialization handled by the inspector frame
* Keep the CSS added to container elements per-tool instead of on the html/body elements
* Test refactors

Another direction we could go is to proceed with converting the inspector frame to html (I have a WIP patch that is doing this).  But it seems risky to tie that work up with this bug, since there's a lot of unknowns still in the html conversion.
FYI, I tried a tentative solution in my complete themes.
https://addons.mozilla.org/firefox/addon/qute-6/
Attached patch 1243598.patch (obsolete) — Splinter Review
Attachment #8719955 - Flags: feedback?(bgrinstead)
Comment on attachment 8719955 [details] [diff] [review]
1243598.patch

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

This fixes the issue for me.  Please request review from Patrick though

::: devtools/client/themes/computed.css
@@ +6,5 @@
>  #sidebar-panel-computedview {
>    margin: 0;
>    display : flex;
>    flex-direction: column;
> +  height: calc(100% - 24px);

It'd be worth adding a comment on these height lines pointing to this bug
Attachment #8719955 - Flags: feedback?(bgrinstead) → feedback+
Attached patch 1243598.patch [1.0] (obsolete) — Splinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=38b5e4f04dc5
Attachment #8719955 - Attachment is obsolete: true
Attachment #8720132 - Flags: review?(bgrinstead)
Attachment #8720132 - Flags: review?(pbrosset)
Attachment #8720132 - Flags: review?(pbrosset)
Comment on attachment 8720132 [details] [diff] [review]
1243598.patch [1.0]

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

::: devtools/client/themes/computed.css
@@ +9,2 @@
>    width: 100%;
> +  /* Bug 1243598 - reduce the height of the filter toolbar because of the

This isn't really reducing the height of the toolbar (in this and the other comments), but rather the container itself to make room for the tabs above, right?
Attachment #8720132 - Flags: review?(bgrinstead) → review+
Attachment #8720132 - Attachment is obsolete: true
Attachment #8720325 - Flags: review+
Attachment #8715889 - Attachment is obsolete: true
Attachment #8716010 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/4f1701beb4ec
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Depends on: 1250892
Brian, could you please verify the fix? Thanks!
Flags: needinfo?(bgrinstead)
:bgrins is on PTO this week, let's try :pbro.
Flags: needinfo?(bgrinstead) → needinfo?(pbrosset)
Verified as fixed in FF47/windows.
Flags: needinfo?(pbrosset)
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #45)
> Verified as fixed in FF47/windows.

Great! Thanks.
Depends on: 1260235
See Also: → 1272350
Depends on: 1278552
Version: unspecified → Trunk
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: