1222403 - Columns aren't properly aligned in the memory table widget

Status: NEW

(DevTools :: Memory, defect, P2)

Description

[Patrick Brosset <:pbro>]

Attachment: columns.png

STR:
- use Windows 10 (maybe this happens elsewhere, haven't tested)
- go to about:home
- open the memory tool and take a snapshot

The columns in the resulting table aren't aligned with their headers.
Looks like there's a 1px offset for every column, and so the 4th column is misaligned by 4px with its header.

Comment 1

[Patrick Brosset <:pbro>]

This seems to be coming from the scrollbar on the .tree element. Hiding overflows fixes the problem.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Patrick, you mention that hiding overflows fixes the problem; can you whip that up into a quick patch? It would be very appreciated!

Comment 3

[Julian Descottes [:jdescottes]]

Confirmed on OSX, Win7 and Ubuntu -> updating title and platforms.

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: bug1222403.diff

This patch uses "vw" units instead of "%" for the columns widths.

Comment 5

[Victor Porof [:vporof][:vp]]

Comment on attachment 8708238 [details] [diff] [review]
bug1222403.diff

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

I don't understand this fix, can you please explain why vw is better than %, especially since the table area doesn't fill out the entire toolbox window.

Comment 6

[Julian Descottes [:jdescottes]]

(In reply to Victor Porof [:vporof][:vp] from comment #5)
> Comment on attachment 8708238 [details] [diff] [review]
> bug1222403.diff
> 
> Review of attachment 8708238 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't understand this fix, can you please explain why vw is better than %,
> especially since the table area doesn't fill out the entire toolbox window.

The header container never has scrollbars, while the body container may have a scrollbar.
The classes modified here are applied to elements both in the header and the body of the memory table.

When the body starts having a scrollbar, the width in % will compute different values for the header elements then for the body elements. Which is not the case when using vw. However this approach is only valid as long as other elements taking up width are fixed-width. (which is the case here).

This is how the "Call Tree" of the Performance tool handles the same issue (which is why I thought it would be ok to reuse it here ?).

Comment 7

[Patrick Brosset <:pbro>]

I guess another solution would be to apply a padding-inline-end to the header with the value being winUtils.getScrollbarSize
And then using overflow-y:scroll on the body.

But that's ugly because: the body will *always* have a scrollbar, and the header padding has to be added using javascript.

Can't think of anything else right now, but there surely are CSS-only solutions.
Anyway, clearing the NI? flag since Julian is taking a look at this bug.

Comment 8

[Julian Descottes [:jdescottes]]

FWIW, using vw units, we can use calc to compute same width as would have 8% and 10% :

> width: calc((100vw - var(--sidebar-width)) * 0.1); /* 10% */
> width: calc((100vw - var(--sidebar-width)) * 0.08); /* 8% */

Not sure this is worth the extra complexity.

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #7)
> I guess another solution would be to apply a padding-inline-end to the
> header with the value being winUtils.getScrollbarSize
> And then using overflow-y:scroll on the body.
> 
> But that's ugly because: the body will *always* have a scrollbar, and the
> header padding has to be added using javascript.
> 

Actually my first solution was simply to add an overflow-y: scroll on both the header and the body.
To hide the scroll gutter in the header, we could apply a padding + negative margin. Then as you say the body always has a scrollbar, which is why I found the solution implemented by the Performance tool appealing.

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8708238 [details] [diff] [review]
bug1222403.diff

For the more complex CSS changes, I think it would be better to flag Victor or :ntim for review; CSS is (clearly) not one of my strengths :(

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

s/complex/subtle/

Comment 11

[Victor Porof [:vporof][:vp]]

Talked with Julian on IRC a bit, and my assertion was that vw units are incorrect in this specific case, because they consider the width of the entire tool, not of just the widget itself. It makes more sense to have column widths depending on the table width, not some other arbitrary external width, and assume some specific external layout.

I'll get back on this soon.

Comment 12

[Victor Porof [:vporof][:vp]]

(In reply to Victor Porof [:vporof][:vp] from comment #11)
> and assume some specific external layout.

i meant "and *not* assume"

Comment 13

[Victor Porof [:vporof][:vp]]

(In reply to Julian Descottes from comment #8)

> Actually my first solution was simply to add an overflow-y: scroll on both
> the header and the body.
> To hide the scroll gutter in the header, we could apply a padding + negative
> margin. Then as you say the body always has a scrollbar, which is why I
> found the solution implemented by the Performance tool appealing.

This doesn't sounds too horrible. I'd suggest we use this approach for now.

Additionally, why not do it on Windows only, since that's where this issue actually occurs. Not sure what the current policy is about writing platform-specific css code, but since this bug simply involves adding a few extra css rules to solve a platform-specific bug, maybe it's ok?

A better idea would be to combine the table headers with the table body, and position the header in a way that doesn't affect layout (e.g. absolute or sticky). This is a bit invasive though, so it's up to you.

Comment 14

[Patrick Brosset <:pbro>]

We now have platform-specific HTML attributes on our documents' root elements: platform="win" for example.
See this dxr search: https://dxr.mozilla.org/mozilla-central/search?q=%3Aroot[platform%3D&redirect=false&case=false

Comment 15

[Julian Descottes [:jdescottes]]

Thanks for the feedback Victor!

> Additionally, why not do it on Windows only, since that's where this issue actually occurs

It also occurs on Linux. 
And on Mac OS X 10.10 (Yosemite) when the OS switches to energy saving graphic mode -> it disables translucent scrollbars and switches to opaque scrollbars and gutters (...).
So it needs to be fixed on all platforms.

The bad news is that my fix doesn't work on Linux. The scrollbar is not rendered if there is not enough height to display it, and therefore it doesn't affect width calculations.

> A better idea would be to combine the table headers with the table body,
> and position the header in a way that doesn't affect layout (e.g. absolute or sticky).
> This is a bit invasive though, so it's up to you.

The scroll is handled by the tree component, in order to render only a subset of items. The header is outside of the tree's scope. It is composed with the tree by client/memory/heap.js 
Changing this seems a bit too much for what we want to achieve here.

With this in mind I would go for Patrick's proposal for now, setting the header's padding-right in JS.

Comment 16

[Julian Descottes [:jdescottes]]

Attachment: bug1222403.v2,diff

Comment 17

[Julian Descottes [:jdescottes]]

Comment on attachment 8709213 [details] [diff] [review]
bug1222403.v2,diff

Removing the feedback?
winUtils.getScrollbarSize does not work as I expected. I thought it would return the scrollbar width depending on the platform only. 

But it actually needs to be called on an actual window object which has a scrollbar in order to return a valid value.

Comment 18

[Tim Nguyen :ntim]

Comment on attachment 8709213 [details] [diff] [review]
bug1222403.v2,diff

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

I expect this doesn't work well with the dark theme's floating scrollbars ?
Maybe it would be a good idea to use floating scrollbars everywhere ?

::: devtools/client/memory/components/census-header.js
@@ +14,5 @@
>      return dom.div(
>        {
> +        className: "header",
> +        style: {
> +          paddingRight: getScrollbarWidth() + "px"

nit: paddingInlineEnd

::: devtools/client/memory/components/dominator-tree-header.js
@@ +14,5 @@
>      return dom.div(
>        {
> +        className: "header",
> +        style: {
> +          paddingRight: getScrollbarWidth() + "px"

Same here.

::: devtools/client/themes/memory.css
@@ +304,5 @@
>    /**
>     * Flexing to fill out remaining vertical space. @see .heap-view-panel
>     */
>    flex: 1;
> +  overflow-y: scroll;

This means we'll always have scrollbars, even if there's nothing to scroll. I don't think that's the wanted user behaviour. Maybe you could try using a hidden element instead to get the scrollbar width ?

Comment 19

[magicp]

Attachment: [e-mail]

I tested it in my theme. It maybe works good.

Comment 20

[Julian Descottes [:jdescottes]]

(In reply to Tim Nguyen [:ntim] from comment #18)
> Comment on attachment 8709213 [details] [diff] [review]

Thanks for the review ntim! 
FWIW, the attachment is marked as obsolete because winUtils.getScrollbarSize has shortcomings. We need to use another approach to calculate the platform scrollbar width (as you suggest, probably relying on hidden elements).

> nit: paddingInlineEnd

good point, thanks!

> This means we'll always have scrollbars, even if there's nothing to scroll.
> Maybe you could try using a hidden element instead to get the scrollbar width ?

The goal is to have a consistent body width, so that body columns are not resized when tree nodes are expanded.
If we remove this, the header container has to be updated when nodes are expanded/collapsed : check if the body has a scroll, and if yes apply a padding equivalent to the scrollbar width.

To be honest, I felt using vw units was a good compromise, even if it assumes a certain layout. All the solutions proposed at this point feel a bit heavy, and will require maintenance.

I might be missing a simpler approach here ?

Comment 21

[johngraciliano]

This problem seems to remain.  The simplest solution is that by magicp, which is to have a fixed column width.
In Nightly, the rule for these columns in memory.css now has:
  min-width: 19ch
This alone makes the problem disappear unless the window is wide enough for 19ch < 10%.  So the panel has to be wider than 190ch (1140px? or 11.875in?) for the problem to be seen in Nightly. Setting
width: 19ch
would make it go away (this is magicp's strategy).
  ...
Yet this approach brings a different problem.  This has to do with the poor way the tool looks when dock on the side of the window.  In Nightly or with migicp's Qute theme, if the memory tool is dock to the side, it is likely to show a horizontal scrollbar.  But that scrollbar does not affect the column headers!!!! This is certainly a huge alignment problem.  This problem is also noticeable when the tool is dock to the bottom of a narrow window.  When just using the 10% value, as in Firefox 44, this means that only part of the column contents is shown.  In the best case, it only shows the percentages, but the headers are incomplete.  However this may be considered a different bug that affects Firefox 47. Perhaps I should create it separately.

Comment 22

[johngraciliano]

Regarding the added complication introduced with Nightly that I explained in the last paragraph of my previous comment:  Adding to memory.css the rules:
#heap-view{
  overflow-x: auto;
}
#heap-view > .heap-view-panel{
  min-width: 80ch;
}
seems to solve that problem.  But I think the value I used for min-width should be higher than 80ch.
The is yet a presentation problem left when docking the tool on the side but it is certainly a different issue and I do not know if it has been reported or not.

Comment 23

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Hi John, I've filed bug 1252893 as a general side-docking-and-the-memory-panel bug.

Comment 24

[johngraciliano]

Thank you Nick! (Sorry for not taking the time to acknowledge before.)