1037859 - Applications tree column and children is not aligned

Status: NEW

(Firefox :: Settings UI, defect, P4)

Description

[Alice0775 White]

Attachment: screenshot

Applications pane of in-content preferences, tree column and children is not aligned.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: Application no scrollbar

With few elements in in the richlistbox when where is no scrollbar then it's almost correct. Only this rule needs to set to 0px: http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/preferences/applications.css#11.

But with scrollbar the width of the two hbox elements in richlistitem are recalculated and are then no more in synch with the treecols.

Somehow the treecols and richlistitems are needing to be connected together to always have the same width.

Comment 2

[Matthew N. [:MattN]]

More of a polish issue so not a hard blocker.

Comment 3

[Tiziana Sellitto [:tiziana]]

Attachment: Add CSS class when scrollbar is visible to "actionsMenu" in applications to adjust margins between the "menulist" and the header in applications panel of in-content preferences.

I've added the CSS class for Windows and OSX (I also noticed the problem there sometimes) themes. I didn't had a linux machine to check, right now.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8589860 [details] [diff] [review]
Add CSS class when scrollbar is visible to "actionsMenu" in applications to adjust margins between the "menulist" and the header in applications panel of in-content preferences.

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

Did you test that this patch worked for you? When I applied the patch I couldn't get the `if (this._list.scrollBoxObject.element.scrollTopMax > 0)` line hit by the debugger. Also, you should be able to do this._list.scrollTopMax > 0 instead of going through the scrollBoxObject.

Comment 5

[Tiziana Sellitto [:tiziana]]

Attachment: bug1037859.patch

I couldn't get the whole application.js file hit by the debugger, too. I tested it changing the richlistbox height.

Anyway, I can see that my patch doesn't work correctly on Mac because the scrollbar disappears even when visible. So I've removed the part of the patch that was for the OSX platform and I focused on Windows.

As for the this._list.scrollTopMax, I noticed that the property is always zero even when the scrollbar is visible. The 'scrollTopMax' that changes is only the one for the 'scrollBoxObject'.

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 8593702 [details] [diff] [review]
bug1037859.patch

>+/* Applied to line up margins when scrollbar is visible. */
>+.actionsMenu.scrollbar-visible {
>+  -moz-margin-start: 6px !important;
>+}

The scrollbar width varies across OSes and OS themes / settings, so this doesn't seem correct. ... unless I'm missing how this patch works.

What's causing this bug in the first place? This is only an issue for in-content preferences, not for the preferences window, is it? It would be good to know the underlying problem so we can fix that rather than adding hacks as in this patch.

Comment 7

[Tim Nguyen :ntim]

(In reply to Dão Gottwald [:dao] from comment #6)
> What's causing this bug in the first place? This is only an issue for
> in-content preferences, not for the preferences window, is it?
It is an issue in the preferences window too. It is just worse in the in-content prefs due to the large padding.

Comment 8

[Tiziana Sellitto [:tiziana]]

(In reply to Dão Gottwald [:dao] from comment #6)
> Comment on attachment 8593702 [details] [diff] [review]
> bug1037859.patch
> What's causing this bug in the first place? This is only an issue for
> in-content preferences, not for the preferences window, is it? It would be
> good to know the underlying problem so we can fix that rather than adding
> hacks as in this patch.

I've tried to figure it out before using this approach, but it's not really clear to me the origin of this bug that, as Tim said, is happening in the preferences window, too.

Any suggestion or idea on how I can try to identify the source of the bug or what it could be?

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

We do have ScrollbarSampler.jsm[1] that can be used to get the width of a scrollbar dynamically.

[1] http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/ScrollbarSampler.jsm
and one of the usages of it here:
http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/content/panelUI.js#248

Comment 10

[:Gijs (he/him)]

As a guess, the proper fix here might involve switching away from the currently used column headers to different ones. See bug 1123740 for some context about why I think that.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Tiziana, were you able to make any more progress here?

Comment 12

[Tiziana Sellitto [:tiziana]]

(In reply to Dão Gottwald [:dao] from comment #6)
> What's causing this bug in the first place? This is only an issue for
> in-content preferences, not for the preferences window, is it? It would be
> good to know the underlying problem so we can fix that rather than adding
> hacks as in this patch.

Jared, reading at comment #6 and comment #10 I made the idea that it's not really clear which should be the fix here, mainly because it's not still clear the actual source of the bug. So I'm not working on it right now.

Comment 13

[Tiziana Sellitto [:tiziana]]

I'm setting the status to NEW since I'm not currently working on this.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Closing old needinfo request...

Comment 15

[Fischer [:Fischer]]

Make no longer blocks: 718011, 752197 because this bug is not moving into in-content and already being tracked by 1271779

Comment 16

[Fischer [:Fischer]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9)
> We do have ScrollbarSampler.jsm[1] that can be used to get the width of a
> scrollbar dynamically.
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/browser/components/
> customizableui/ScrollbarSampler.jsm
> and one of the usages of it here:
> http://mxr.mozilla.org/mozilla-central/source/browser/components/
> customizableui/content/panelUI.js#248

@Jared,

It seems that ScrollbarSampler.jsm at is to get system scroll bar width. I am not sure if it is applicable in the Applications panel.

Another possible approach is to retrieve richlistitem item width dynamically.
Then set the "Content Type" treecol width based on retrieved value and let the "Action" treecol occupies the rest of space.
In this way, the header columns and the items below would have the same width.
Because user could resize window, listening to the resize event and then adjusting is required.

How do you think?

Thank you

Comment 17

[:Gijs (he/him)]

(In reply to Fischer [:Fischer] from comment #16)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9)
> > We do have ScrollbarSampler.jsm[1] that can be used to get the width of a
> > scrollbar dynamically.
> > 
> > [1]
> > http://mxr.mozilla.org/mozilla-central/source/browser/components/
> > customizableui/ScrollbarSampler.jsm
> > and one of the usages of it here:
> > http://mxr.mozilla.org/mozilla-central/source/browser/components/
> > customizableui/content/panelUI.js#248
> 
> @Jared,
> 
> It seems that ScrollbarSampler.jsm at is to get system scroll bar width. I
> am not sure if it is applicable in the Applications panel.
> 
> Another possible approach is to retrieve richlistitem item width dynamically.
> Then set the "Content Type" treecol width based on retrieved value and let
> the "Action" treecol occupies the rest of space.
> In this way, the header columns and the items below would have the same
> width.
> Because user could resize window, listening to the resize event and then
> adjusting is required.
> 
> How do you think?

I don't think we should not need to write code that complex or extensive here  - the severity of the problem does not justify that, and in any case it should be possible to solve this more simply. See in particular comment 6 and also comment #10 / bug 1123740.

Comment 18

[Fischer [:Fischer]]

Attachment: listhead_issue.png

Comment 19

[Fischer [:Fischer]]

(In reply to :Gijs Kruitbosch (away 26-29 incl.) from comment #17)
> (In reply to Fischer [:Fischer] from comment #16)
> > (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9)
> > > We do have ScrollbarSampler.jsm[1] that can be used to get the width of a
> > > scrollbar dynamically.
> > > 
> > > [1]
> > > http://mxr.mozilla.org/mozilla-central/source/browser/components/
> > > customizableui/ScrollbarSampler.jsm
> > > and one of the usages of it here:
> > > http://mxr.mozilla.org/mozilla-central/source/browser/components/
> > > customizableui/content/panelUI.js#248
> > 
> > @Jared,
> > 
> > It seems that ScrollbarSampler.jsm at is to get system scroll bar width. I
> > am not sure if it is applicable in the Applications panel.
> > 
> > Another possible approach is to retrieve richlistitem item width dynamically.
> > Then set the "Content Type" treecol width based on retrieved value and let
> > the "Action" treecol occupies the rest of space.
> > In this way, the header columns and the items below would have the same
> > width.
> > Because user could resize window, listening to the resize event and then
> > adjusting is required.
> > 
> > How do you think?
> 
> I don't think we should not need to write code that complex or extensive
> here  - the severity of the problem does not justify that, and in any case
> it should be possible to solve this more simply. See in particular comment 6
> and also comment #10 / bug 1123740.

Just try use
  <listhead>
    <listheader ... >
  </listhead>

Although semantically correct, still some issues needs to be handled.
- Set <listhead> with position: absolute so won’t scroll out of view
- Give <listheader> the width dynamically but should also consider window resize
- The first <richlistitem> (ANX file) would be covered by <listhead>
Please see listhead_issue.png for detail.

Just like Comment #6, scroll bar style varies by OS platforms. Unlike Mac OSX which the scroll bar overlays on top of content, in Linux, the scroll bar occupies space. 

Currently the headers are put outside of the scrollbox, see [1].
So when scroll bar appears and occupies some space of list items, the header columns and the list items are not aligned any more in Linux.


[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/richlistbox.xml#20

Comment 20

[Fischer [:Fischer]]

Attachment: Bug 1037859 - Applications tree column and children is not aligned

Review commit: https://reviewboard.mozilla.org/r/76920/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76920/

Comment 21

[Fischer [:Fischer]]

Comment on attachment 8788416 [details]
Bug 1037859 - Applications tree column and children is not aligned

@Jaws:

Like Comment #6, it could set the "Content Type" treecol width according to the richlistitem item width to ensure columns are aligned.

However, it shouldn't have to consider the window resize event since we only have to do the job when building the view(inside the call of _rebuildView).

Comment 22

[Fischer [:Fischer]]

(In reply to Fischer [:Fischer] from comment #21)
> Comment on attachment 8788416 [details]
> Bug 1037859 - Applications tree column and children is not aligned
> 
> @Jaws:
> 
> Like Comment #6, it could set the "Content Type" treecol width according to
> the richlistitem item width to ensure columns are aligned.
> 
> However, it shouldn't have to consider the window resize event since we only
> have to do the job when building the view(inside the call of _rebuildView).

Sorry, still need to consider the window resizing, otherwise the width would be fixed and incorrect during resizing.

Comment 23

[:Gijs (he/him)]

Why does this patch take flex off the type column?

Comment 24

[Fischer [:Fischer]]

(In reply to :Gijs Kruitbosch from comment #23)
> Why does this patch take flex off the type column?
If flex was specified, then width would be ignored. 
Maybe we could use CSS width: calc(...) to do the width calculation job then width calculation would be done automatically during resizing.