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

VERIFIED FIXED in Firefox 47

Status

()

Firefox
Developer Tools: Inspector
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: bgrins, Assigned: gl)

Tracking

({regression})

Trunk
Firefox 47
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 unaffected, firefox46 unaffected, firefox47+ fixed)

Details

(URL)

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8712966 [details]
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.
(Reporter)

Comment 1

2 years ago
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

Comment 2

2 years ago
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"
Duplicate of this bug: 1244027
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)

Updated

2 years ago
Assignee: nobody → gl
Flags: needinfo?(gl)
(Reporter)

Comment 6

2 years ago
(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.

Updated

2 years ago
Duplicate of this bug: 1245418

Comment 8

2 years ago
[Tracking Requested - why for this release]: regression
status-firefox45: --- → affected
status-firefox46: --- → affected
status-firefox47: --- → affected
status-firefox-esr38: --- → unaffected
status-firefox-esr45: --- → ?
tracking-firefox45: --- → ?
(Reporter)

Comment 9

2 years ago
[Tracking Requested - why for this release]: Note that this is only affecting 47 (caused by https://hg.mozilla.org/mozilla-central/rev/2d2e19e15a1c)
status-firefox45: affected → unaffected
status-firefox46: affected → unaffected
status-firefox-esr38: unaffected → ---
status-firefox-esr45: ? → ---
tracking-firefox45: ? → ---
tracking-firefox47: --- → ?
Duplicate of this bug: 1245588
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.
(Reporter)

Comment 12

2 years ago
(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?
(Reporter)

Comment 13

2 years ago
Ah, here's a STR in normal toolbox:

Open http://www.cnn.com/
Open Inspector
Switch to Fonts Panel

Comment 14

2 years ago
(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
(Reporter)

Comment 15

2 years ago
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.
(Reporter)

Updated

2 years ago
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
(Reporter)

Comment 17

2 years ago
(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.
(Reporter)

Comment 18

2 years ago
(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?
(Reporter)

Comment 19

2 years ago
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.
(Reporter)

Comment 20

2 years ago
Created attachment 8715492 [details] [diff] [review]
sidebar-1.patch

This fixes STR in Comment 14
(Reporter)

Comment 21

2 years ago
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
(Reporter)

Comment 23

2 years ago
Created attachment 8715889 [details] [diff] [review]
sidebar-1.patch

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).
(Reporter)

Comment 28

2 years ago
(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
Created attachment 8716010 [details] [diff] [review]
Bug_1243598_-_inspector_sidebar_css_changes.diff

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.
(Reporter)

Comment 30

2 years ago
(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.
(Reporter)

Comment 31

2 years ago
(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
(Reporter)

Updated

2 years ago
Depends on: 1246010

Comment 32

2 years ago
(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.
(Reporter)

Comment 33

2 years ago
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.
Blocks: 1245996
(Reporter)

Updated

2 years ago
(Reporter)

Updated

2 years ago
Duplicate of this bug: 1247534

Comment 35

2 years ago
FYI, I tried a tentative solution in my complete themes.
https://addons.mozilla.org/firefox/addon/qute-6/
(Assignee)

Comment 36

2 years ago
Created attachment 8719955 [details] [diff] [review]
1243598.patch
Attachment #8719955 - Flags: feedback?(bgrinstead)
(Reporter)

Comment 37

2 years ago
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+
(Assignee)

Comment 38

2 years ago
Created attachment 8720132 [details] [diff] [review]
1243598.patch [1.0]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=38b5e4f04dc5
Attachment #8719955 - Attachment is obsolete: true
Attachment #8720132 - Flags: review?(bgrinstead)
(Assignee)

Updated

2 years ago
Attachment #8720132 - Flags: review?(pbrosset)
(Assignee)

Updated

2 years ago
Attachment #8720132 - Flags: review?(pbrosset)
(Reporter)

Comment 39

2 years ago
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+
(Assignee)

Comment 40

2 years ago
Created attachment 8720325 [details] [diff] [review]
1243598.patch [2.0]
Attachment #8720132 - Attachment is obsolete: true
Attachment #8720325 - Flags: review+

Comment 41

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/4f1701beb4ec
(Reporter)

Updated

2 years ago
Attachment #8715889 - Attachment is obsolete: true
(Reporter)

Updated

2 years ago
Attachment #8716010 - Attachment is obsolete: true

Comment 42

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4f1701beb4ec
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: affected → fixed
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.
tracking-firefox47: ? → +

Updated

2 years ago
Status: RESOLVED → VERIFIED
(Reporter)

Updated

2 years ago
Depends on: 1260235
See Also: → bug 1272350

Updated

2 years ago
Depends on: 1278552
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.