Various layout problems when the inspector sidebar is very small

RESOLVED INVALID

Status

P2
normal
RESOLVED INVALID
2 years ago
2 months ago

People

(Reporter: pbro, Assigned: jdescottes)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

This might need to be a meta bug if it's easier to fix these issues separately.

STR:
- Open about:home
- Open the inspector panel
- Grab the splitter and make the sidebar really small so you start seeing problems (at its minimum, 50px, the sidebar is so small that you don't see anything anymore, so make it a bit bigger)
- Go through the various sidebar panels and notice these layout problems:

1) computed-view

In the box-model widget, the various labels "margin", "padding", ... overlap the values
Also the various boxes (the purple padding box for instance) overflow out of the padding and margin boxes while they should stay inside

The Browser Styles checkbox also becomes incorrectly vertically aligned and overflows its container.

2) Animations

The "All Animations" label also overflows its container.

When there are animations displayed in the panel, only the list of DOM nodes is visible, not the animations themselves, and there's no horizontal scrollbar to get to them. There should be a min-width that causes a scrollbar to appear so the whole content of the panel is visible.

3) Fonts

The "Show all fonts used" label overflows its container.
Not a regression, but this originally comes from bug 1262443 and one of its follow-ups since we now can make the sidebar much smaller than we used to.
Comment hidden (mozreview-request)
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Comment on attachment 8797582 [details]
Bug 1307408 - part1: fix layout issues for small widths in inspector sidebar;

Just a first improvement here, making sure no scrollbars are displayed when the inspector has a small width. 

Patrick: can you give it a try and let me know if you'd be ok to land this first and then address improvements in a follow up (such as adding horizontal scrollbars).

Note this also should address Bug 1307404.
Attachment #8797582 - Flags: feedback?(pbrosset)
added Helen in cc, as we discussed this on IRC today.

Here is a try push with builds for windows/osx/linux if anyone wants to try. https://treeherder.mozilla.org/#/jobs?repo=try&revision=c77c31b7f97b10f6002e69e737b1deb5a8d7536b
Comment on attachment 8797582 [details]
Bug 1307408 - part1: fix layout issues for small widths in inspector sidebar;

I've tested this quickly, it looks really good. Thanks for these changes. Yes, I do think we should land this first and then tackle other individual problems later.

Just one weird thing I noticed: if you drag the splitter to the left quickly, then the markup-view may scroll within its frame. I hope it's visible in the GIF:
https://dl.dropboxusercontent.com/u/714210/inspector-slides-left.gif
Attachment #8797582 - Flags: feedback?(pbrosset) → feedback+
Comment hidden (mozreview-request)
(In reply to Patrick Brosset <:pbro> from comment #5)
> Comment on attachment 8797582 [details]
> Bug 1307408 - part1: fix layout issues for small widths in inspector sidebar;
> 
> I've tested this quickly, it looks really good. Thanks for these changes.
> Yes, I do think we should land this first and then tackle other individual
> problems later.
> 
> Just one weird thing I noticed: if you drag the splitter to the left
> quickly, then the markup-view may scroll within its frame. I hope it's
> visible in the GIF:
> https://dl.dropboxusercontent.com/u/714210/inspector-slides-left.gif

Thanks for checking. I investigated the issue your GIF illustrates. It comes from the breadcrumbs widget. When they overflow, the "chevron" buttons are added, and we scrollIntoView to make sure the selected breadcrumb item is visible. Which in this case will make the whole container scroll. 

Not sure how to fix that just yet, but I would like to go forward and land the first batch of improvements.
Keywords: leave-open
Inspector bug triage (filter on CLIMBING SHOES).
Priority: -- → P2

Comment 10

2 years ago
mozreview-review
Comment on attachment 8797582 [details]
Bug 1307408 - part1: fix layout issues for small widths in inspector sidebar;

https://reviewboard.mozilla.org/r/83258/#review83144

Looks good to me and all the issues mentioned in comment #0 are fixed for me.

R+, assuming my comment gets resolved.

Honza

::: devtools/client/themes/inspector.css:39
(Diff revision 2)
> -.devtools-main-content {
> +#inspector-main-content {
>    /* Subtract 1 pixel from the panel height. It's puzzling why this
>      is needed, but if not presented the entire Inspector panel
>      content jumps 1 pixel up when the Toolbox is opened. */
>    height: calc(100% - 1px);
> +  min-width: 125px;

Is this needed? I don't see any difference when disabling this (using Browser Toolbox).

Honza
Attachment #8797582 - Flags: review?(odvarko) → review+
(Assignee)

Comment 11

2 years ago
mozreview-review-reply
Comment on attachment 8797582 [details]
Bug 1307408 - part1: fix layout issues for small widths in inspector sidebar;

https://reviewboard.mozilla.org/r/83258/#review83144

Thanks for the review, see my reply below.

> Is this needed? I don't see any difference when disabling this (using Browser Toolbox).
> 
> Honza

This allows to mitigate the glitch described by Patrick and discussed in the previous comments. If you reduce the splitter size quickly the scrollIntoView call will force the container to scroll and will show blank space in the panel, which really feels buggy. With this min-width, even if the container scrolls, it will not look empty.

125px is kind of an arbitrary value though, it only has to be over 100px, which is the effective min-height of the #inspector-toolbar element (which is the element that would force the overflow in the absence of a greater min-width set on the container).

So overall I'd be in favor of keeping this min-width.
Created attachment 8799421 [details]
bug1307408-min-width-reason.png

See my previous comment for the min-width: 125px, and above a screenshot of what happens without it (top) and with it (bottom) if you quickly resize the splitter.

I can of course add a comment above the value to explain what I just said here.
Flags: needinfo?(odvarko)
(In reply to Julian Descottes [:jdescottes] from comment #11)
> Comment on attachment 8797582 [details]
> This allows to mitigate the glitch described by Patrick and discussed in the
> previous comments. If you reduce the splitter size quickly the
> scrollIntoView call will force the container to scroll and will show blank
> space in the panel, which really feels buggy. With this min-width, even if
> the container scrolls, it will not look empty.
Ah, I see it now.

> 125px is kind of an arbitrary value though, it only has to be over 100px,
> which is the effective min-height of the #inspector-toolbar element (which
> is the element that would force the overflow in the absence of a greater
> min-width set on the container).
> 
> So overall I'd be in favor of keeping this min-width.
Agree

> I can of course add a comment above the value to explain what I just said here.
Yes, please!

Thanks!
Honza
Flags: needinfo?(odvarko)
Comment hidden (mozreview-request)

Comment 16

2 years ago
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a38df6f51b67
part1: fix layout issues for small widths in inspector sidebar;r=Honza
Enough fixes done here that it doesn't need to block the inspector-top-bugs bug anymore. Thanks!
No longer blocks: 1264624
Patrick I propose to close this as invalid. There's no clear small width issue in the inspector at the moment and the bug is not actionable.
Flags: needinfo?(pbrosset)
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
Flags: needinfo?(pbrosset)
Resolution: --- → INVALID
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open

Updated

2 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.