1126278 - In-content preferences category list should lose the padding-top on low window heights

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P3)

Description

[Ian Moody [:Kwan] (UTC+1)]

Attachment: Rough patch

With the fix for bug 1036434, the category list is now scrollable when the window is short, so everyone can access Advanced.  Unfortunately the padding-top on the list sticking around looks silly.

In an ideal world, maybe something like this would work, along with some JS to set the count attribute on #category:

#categories {
  --item-height: 40px;
  --padding-top: 39px;
  --total-height: calc( attr(count) * var(--item-height));
  --height-and-padding: calc(var(--total-height) + var(--padding-top);
  padding-top: var(--padding-top);
  max-height: 100vh;
}

@media (max-height: var(--height-and-padding)) {
  #categories {
    padding-top: calc(100vh - var(--total-height));
  }
}

#categories > .category {
  height: var(--item-height);
}

Since we're not in said ideal world, this'll be a little hairier.

Comment 1

[Ian Moody [:Kwan] (UTC+1)]

Attachment: Rough patch v1.1

So now that I've actually got it working, I had a chance to go back and check over the @media-finding logic.  It was... messy.  Here's a cleaned up version

Comment 2

[:Gijs (he/him)]

Comment on attachment 8555253 [details] [diff] [review]
Rough patch v1.1

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

Promising, but needs some work... more detail below.

::: browser/components/preferences/in-content/preferences.js
@@ +58,5 @@
>  
> +  // Make sure the #categories height setting is apropos the # of categories
> +  let i = 0, rule;
> +  while (document.styleSheets[i].href != 'chrome://browser/skin/preferences/in-content/preferences.css')
> +    ++i;

Let's use some mozilla oddness:

const kStyleURL = "chrome://browser/skin/preferences/in-content/preferences.css";
let styleSheet = Array.filter(document.styleSheets, (x) => x.href == kStyleURL)[0];

let rule = Array.filter(styleSheet.cssRules, (x) => {
  return x instanceof CSSMediaRule &&
         ...;
});

The condition for finding this rule seems pretty fragile, but I don't really have a better suggestion.

@@ +67,5 @@
> +      break;
> +  }
> +  let catStyle = getComputedStyle(categories);
> +  let catNum = categories.querySelectorAll('richlistitem:not([hidden])').length;
> +  let itemHeight = Number.parseInt(catStyle.getPropertyValue('--item-height'));

Instead, if we're doing this anyway, wouldn't it make more sense to determine this by asking getBoundingClientRect() for the total height here ? That avoids the complexity I mention later on with regards to font sizes.

Note that this code is also assuming that all the categories only have one line of text, which might not be true (especially not in other locales).

::: browser/themes/shared/incontentprefs/preferences.inc.css
@@ +25,5 @@
>  
>  /* Category List */
>  
>  #categories {
> +  --item-height: 40px;

So, if we're going this route, then this should be specified in a way that respects the font size (em/rem), which should match with the font-size declared later for .category-name, so that they don't get out of sync, either. :-)

@@ +26,5 @@
>  /* Category List */
>  
>  #categories {
> +  --item-height: 40px;
> +  --padding-top: 39px;

I don't think we should use variable names that match CSS property names. In fact, I am surprised it's even legal. :-)

Comment 3

[Ian Moody [:Kwan] (UTC+1)]

(In reply to :Gijs Kruitbosch from comment #2)
> Comment on attachment 8555253 [details] [diff] [review]
> Rough patch v1.1
> 
> Review of attachment 8555253 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Promising, but needs some work... more detail below.
> 
> ::: browser/components/preferences/in-content/preferences.js
> @@ +58,5 @@
> >  
> > +  // Make sure the #categories height setting is apropos the # of categories
> > +  let i = 0, rule;
> > +  while (document.styleSheets[i].href != 'chrome://browser/skin/preferences/in-content/preferences.css')
> > +    ++i;
> 
> Let's use some mozilla oddness:
> 
> const kStyleURL =
> "chrome://browser/skin/preferences/in-content/preferences.css";
> let styleSheet = Array.filter(document.styleSheets, (x) => x.href ==
> kStyleURL)[0];
> 
> let rule = Array.filter(styleSheet.cssRules, (x) => {
>   return x instanceof CSSMediaRule &&
>          ...;
> });

Cool, done.  Nice to know about the existence of these.  Almost as good as when I found out about template strings the other week.

> The condition for finding this rule seems pretty fragile, but I don't really
> have a better suggestion.

Yeah that thought occurred to me, but I couldn't think of a better way of doing it.  Altering media queries from JS seems a bit fraught. (My original idea was, I think, to just insert/modify a <style/> in the page with the @media in it.  Maybe do that instead?)

> @@ +67,5 @@
> > +      break;
> > +  }
> > +  let catStyle = getComputedStyle(categories);
> > +  let catNum = categories.querySelectorAll('richlistitem:not([hidden])').length;
> > +  let itemHeight = Number.parseInt(catStyle.getPropertyValue('--item-height'));
> 
> Instead, if we're doing this anyway, wouldn't it make more sense to
> determine this by asking getBoundingClientRect() for the total height here ?
> That avoids the complexity I mention later on with regards to font sizes.

Okay, done that, though can't use getBoundingClientRect() on the whole list, since it can grow taller than the items.  Instead I've looped the items, like so:

  let totalHeight = Array.reduce(categories.getElementsByClassName('category'),
                                 (acc, el) => acc + el.getBoundingClientRect().height,
                                 0);

> Note that this code is also assuming that all the categories only have one
> line of text, which might not be true (especially not in other locales).

As a side note, doing some playing, I don't know how likely multi-line is.  At the moment the sidebar just gets wider with more text (both installing some different language packs, and manually editing the label).  For multi-line you'd either need to stick some <html:br/> in the entity (is that even possible?) or make the items white-space: pre-wrap, from what I can tell.

> ::: browser/themes/shared/incontentprefs/preferences.inc.css
> @@ +25,5 @@
> >  
> >  /* Category List */
> >  
> >  #categories {
> > +  --item-height: 40px;
> 
> So, if we're going this route, then this should be specified in a way that
> respects the font size (em/rem), which should match with the font-size
> declared later for .category-name, so that they don't get out of sync,
> either. :-)

Hmm, willing to do so, but not to sure what value to go for. At the moment 40px doesn't match up to an <int>em, (3em seems to be 44px), and its also getting a minimum from min-height: 40px in chrome://global/skin/in-content/common.css

> @@ +26,5 @@
> >  /* Category List */
> >  
> >  #categories {
> > +  --item-height: 40px;
> > +  --padding-top: 39px;
> 
> I don't think we should use variable names that match CSS property names. In
> fact, I am surprised it's even legal. :-)

Heh, well they are well separated, so you can't accidentally hit one instead of the other (maybe not true of the old syntax though?) but I've changed it to --top-spacing, for now at least.

Comment 4

[:Gijs (he/him)]

(In reply to Ian Moody (:Kwan) from comment #3)
> (In reply to :Gijs Kruitbosch from comment #2)
> > Comment on attachment 8555253 [details] [diff] [review]
> > Rough patch v1.1
> > 
> > Review of attachment 8555253 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Promising, but needs some work... more detail below.
> > 
> > ::: browser/components/preferences/in-content/preferences.js
> > @@ +58,5 @@
> > >  
> > > +  // Make sure the #categories height setting is apropos the # of categories
> > > +  let i = 0, rule;
> > > +  while (document.styleSheets[i].href != 'chrome://browser/skin/preferences/in-content/preferences.css')
> > > +    ++i;
> > 
> > Let's use some mozilla oddness:
> > 
> > const kStyleURL =
> > "chrome://browser/skin/preferences/in-content/preferences.css";
> > let styleSheet = Array.filter(document.styleSheets, (x) => x.href ==
> > kStyleURL)[0];
> > 
> > let rule = Array.filter(styleSheet.cssRules, (x) => {
> >   return x instanceof CSSMediaRule &&
> >          ...;
> > });
> 
> Cool, done.  Nice to know about the existence of these.  Almost as good as
> when I found out about template strings the other week.
> 
> > The condition for finding this rule seems pretty fragile, but I don't really
> > have a better suggestion.
> 
> Yeah that thought occurred to me, but I couldn't think of a better way of
> doing it.  Altering media queries from JS seems a bit fraught. (My original
> idea was, I think, to just insert/modify a <style/> in the page with the
> @media in it.  Maybe do that instead?)

That is probably cleaner, yes. I like that idea. I think we just need to insert it here, right? We run it once, and then it has the right media query. :-)

> > @@ +67,5 @@
> > > +      break;
> > > +  }
> > > +  let catStyle = getComputedStyle(categories);
> > > +  let catNum = categories.querySelectorAll('richlistitem:not([hidden])').length;
> > > +  let itemHeight = Number.parseInt(catStyle.getPropertyValue('--item-height'));
> > 
> > Instead, if we're doing this anyway, wouldn't it make more sense to
> > determine this by asking getBoundingClientRect() for the total height here ?
> > That avoids the complexity I mention later on with regards to font sizes.
> 
> Okay, done that, though can't use getBoundingClientRect() on the whole list,
> since it can grow taller than the items.  Instead I've looped the items,
> like so:
> 
>   let totalHeight =
> Array.reduce(categories.getElementsByClassName('category'),
>                                  (acc, el) => acc +
> el.getBoundingClientRect().height,
>                                  0);

Mm, but this has sad perf implications and breaks if we add margin/padding/border (whichever of those isn't in the box model in use here, I'm too lazy to check). More mozilla craziness to the rescue:

let outerBox = document.getAnonymousElementByAttribute(categories, "anonid", "main-box");
let innerBox = document.getAnonymousElementByAttribute(outerBox, "class", "box-inherit scrollbox-innerbox");
let totalHeight = innerBox.getBoundingClientRect().height;


> > Note that this code is also assuming that all the categories only have one
> > line of text, which might not be true (especially not in other locales).
> 
> As a side note, doing some playing, I don't know how likely multi-line is. 
> At the moment the sidebar just gets wider with more text (both installing
> some different language packs, and manually editing the label).  For
> multi-line you'd either need to stick some <html:br/> in the entity (is that
> even possible?) or make the items white-space: pre-wrap, from what I can
> tell.

Did you test this on a build including the fix for bug 1036434? Because when I tested that, because of the horizontal overflow fix you applied, I checked what happened for labels with a bunch of words that would be too long to fit without wrapping, and they wrapped for me... it might also depend on the window width if we set a max width on the bar (not sure offhand, but I know we hide the text completely on very narrow window sizes).

> > ::: browser/themes/shared/incontentprefs/preferences.inc.css
> > @@ +25,5 @@
> > >  
> > >  /* Category List */
> > >  
> > >  #categories {
> > > +  --item-height: 40px;
> > 
> > So, if we're going this route, then this should be specified in a way that
> > respects the font size (em/rem), which should match with the font-size
> > declared later for .category-name, so that they don't get out of sync,
> > either. :-)
> 
> Hmm, willing to do so, but not to sure what value to go for. At the moment
> 40px doesn't match up to an <int>em, (3em seems to be 44px), and its also
> getting a minimum from min-height: 40px in
> chrome://global/skin/in-content/common.css

OK, but with the getBoundingClientRect use, this bit shouldn't even be necessary, right? :-)

(The padding-top can stay in px as it doesn't contain text and was already in px - I think it's just meant to match the header text on the right-hand-side of the pref page)

Comment 5

[Ian Moody [:Kwan] (UTC+1)]

Attachment: dynamic Padding v3

(In reply to :Gijs Kruitbosch from comment #4)
> (In reply to Ian Moody (:Kwan) from comment #3)
> > (In reply to :Gijs Kruitbosch from comment #2)
> > > The condition for finding this rule seems pretty fragile, but I don't really
> > > have a better suggestion.
> > 
> > Yeah that thought occurred to me, but I couldn't think of a better way of
> > doing it.  Altering media queries from JS seems a bit fraught. (My original
> > idea was, I think, to just insert/modify a <style/> in the page with the
> > @media in it.  Maybe do that instead?)
> 
> That is probably cleaner, yes. I like that idea. I think we just need to
> insert it here, right? We run it once, and then it has the right media
> query. :-)

Cool, done.  This does rather simplify the code, and I've junked all the var()s and other preferences.inc.css changes now.

> > Okay, done that, though can't use getBoundingClientRect() on the whole list,
> > since it can grow taller than the items.  Instead I've looped the items,
> > like so:
> > 
> >   let totalHeight =
> > Array.reduce(categories.getElementsByClassName('category'),
> >                                  (acc, el) => acc +
> > el.getBoundingClientRect().height,
> >                                  0);
> 
> Mm, but this has sad perf implications and breaks if we add
> margin/padding/border (whichever of those isn't in the box model in use
> here, I'm too lazy to check). More mozilla craziness to the rescue:
> 
> let outerBox = document.getAnonymousElementByAttribute(categories, "anonid",
> "main-box");
> let innerBox = document.getAnonymousElementByAttribute(outerBox, "class",
> "box-inherit scrollbox-innerbox");
> let totalHeight = innerBox.getBoundingClientRect().height;

I did occur to me to try the anon-boxes (though I probably would have tried using querySelector() or something), however the same problem persists, since it also keeps growing vertically, so there is the same problem when the window starts tall (though it does fix the window starting small situation, since the inner box does stop shrinking).
The perf impl. did occur to me, but it should only be the once at page load, so it shouldn't be too bad should it?  The accounting for margins is more of a problem though. Border and padding are indeed fine, I checked.  Don't suppose there's a function like getBoundingClientRect() that includes the margins is there?  Or anything like jQuery's outerWidth(true)? (I suspect not, since it hasn't already been suggested/I can't find it)

> > > Note that this code is also assuming that all the categories only have one
> > > line of text, which might not be true (especially not in other locales).
> > 
> > As a side note, doing some playing, I don't know how likely multi-line is. 
> > At the moment the sidebar just gets wider with more text (both installing
> > some different language packs, and manually editing the label).  For
> > multi-line you'd either need to stick some <html:br/> in the entity (is that
> > even possible?) or make the items white-space: pre-wrap, from what I can
> > tell.
> 
> Did you test this on a build including the fix for bug 1036434? Because when
> I tested that, because of the horizontal overflow fix you applied, I checked
> what happened for labels with a bunch of words that would be too long to fit
> without wrapping, and they wrapped for me... it might also depend on the
> window width if we set a max width on the bar (not sure offhand, but I know
> we hide the text completely on very narrow window sizes).
Hmm, you know I might only have checked on my DevEdition install.  However I just re-checked on my compile with both the patch for this and bug 1036434 and the behaviour is the same, no auto-wrapping.  _However_, and this did occur before but I failed to mention it, there is wrapping only when the scrollbar created by my fix for the previous bug appears, at which point a single word will wrap.
This is triggered in a number of languages (Greek, Chinese (all), Japanese at the least).
Is there a good way to boost the width to pre-allocate space for the scrollbar to avoid that?

> > > So, if we're going this route, then this should be specified in a way that
> > > respects the font size (em/rem), which should match with the font-size
> > > declared later for .category-name, so that they don't get out of sync,
> > > either. :-)
> > 
> > Hmm, willing to do so, but not to sure what value to go for. At the moment
> > 40px doesn't match up to an <int>em, (3em seems to be 44px), and its also
> > getting a minimum from min-height: 40px in
> > chrome://global/skin/in-content/common.css
> 
> OK, but with the getBoundingClientRect use, this bit shouldn't even be
> necessary, right? :-)
Ah, good point.  I had kept it as it seemed like it should be sized in em/using var() anyway, but if that's done it should be a separate bug entirely (see bug 1047586 comment #5)

> (The padding-top can stay in px as it doesn't contain text and was already
> in px - I think it's just meant to match the header text on the
> right-hand-side of the pref page)
Okay. I was wondering why it was that value.

Comment 6

[:Gijs (he/him)]

(In reply to Ian Moody (:Kwan) from comment #5)
> Created attachment 8557637 [details] [diff] [review]
> dynamic Padding v3

> > > Okay, done that, though can't use getBoundingClientRect() on the whole list,
> > > since it can grow taller than the items.  Instead I've looped the items,
> > > like so:
> > > 
> > >   let totalHeight =
> > > Array.reduce(categories.getElementsByClassName('category'),
> > >                                  (acc, el) => acc +
> > > el.getBoundingClientRect().height,
> > >                                  0);
> > 
> > Mm, but this has sad perf implications and breaks if we add
> > margin/padding/border (whichever of those isn't in the box model in use
> > here, I'm too lazy to check). More mozilla craziness to the rescue:
> > 
> > let outerBox = document.getAnonymousElementByAttribute(categories, "anonid",
> > "main-box");
> > let innerBox = document.getAnonymousElementByAttribute(outerBox, "class",
> > "box-inherit scrollbox-innerbox");
> > let totalHeight = innerBox.getBoundingClientRect().height;
> 
> I did occur to me to try the anon-boxes (though I probably would have tried
> using querySelector() or something), however the same problem persists,
> since it also keeps growing vertically, so there is the same problem when
> the window starts tall (though it does fix the window starting small
> situation, since the inner box does stop shrinking).

The problem is that then in the 'tall window' case presumably it switches to not having the padding as soon as the window gets any smaller...

> The perf impl. did occur to me, but it should only be the once at page load,
> so it shouldn't be too bad should it?  The accounting for margins is more of
> a problem though. Border and padding are indeed fine, I checked.  Don't
> suppose there's a function like getBoundingClientRect() that includes the
> margins is there?  Or anything like jQuery's outerWidth(true)? (I suspect
> not, since it hasn't already been suggested/I can't find it)

If you look at the implementation of jQuery's outerWidth, you'll find it just uses getComputedStyle to get the margins and lumps them together. :-)

Note that that then breaks if you start adding them because of collapsing margins.

Thinking about this more: can't you just grab the last child's .bottom value (we can leave the margin and hope people update it if it's ever added) ? That should, IME, be the total value you want here (specifically, 359 in the current situation).


> > > > Note that this code is also assuming that all the categories only have one
> > > > line of text, which might not be true (especially not in other locales).
> > > 
> > > As a side note, doing some playing, I don't know how likely multi-line is. 
> > > At the moment the sidebar just gets wider with more text (both installing
> > > some different language packs, and manually editing the label).  For
> > > multi-line you'd either need to stick some <html:br/> in the entity (is that
> > > even possible?) or make the items white-space: pre-wrap, from what I can
> > > tell.
> > 
> > Did you test this on a build including the fix for bug 1036434? Because when
> > I tested that, because of the horizontal overflow fix you applied, I checked
> > what happened for labels with a bunch of words that would be too long to fit
> > without wrapping, and they wrapped for me... it might also depend on the
> > window width if we set a max width on the bar (not sure offhand, but I know
> > we hide the text completely on very narrow window sizes).
> Hmm, you know I might only have checked on my DevEdition install.  However I
> just re-checked on my compile with both the patch for this and bug 1036434
> and the behaviour is the same, no auto-wrapping.  _However_, and this did
> occur before but I failed to mention it, there is wrapping only when the
> scrollbar created by my fix for the previous bug appears, at which point a
> single word will wrap.
> This is triggered in a number of languages (Greek, Chinese (all), Japanese
> at the least).
> Is there a good way to boost the width to pre-allocate space for the
> scrollbar to avoid that?

Huh. Maybe that's what I was running into then. I don't know off-hand... you could add margins on the category list items, but I expect that that will mean they can't be clicked efficiently (there'll be dead zone at their ends), and we can't really predict the size of the scrollbar well either.

Can you make a screenshot of the worst kind of wrap you see here? Last I checked they're vertically neatly aligned with the icon and so on, so the text wrapping might not be bad, esp. if it only happens on small heights (when the width is likely also limited).

Comment 7

[Ian Moody [:Kwan] (UTC+1)]

Attachment: Wrapping category name.png

(In reply to :Gijs Kruitbosch from comment #6)
> (In reply to Ian Moody (:Kwan) from comment #5)
> > Created attachment 8557637 [details] [diff] [review]
> > dynamic Padding v3
> > 
> > I did occur to me to try the anon-boxes (though I probably would have tried
> > using querySelector() or something), however the same problem persists,
> > since it also keeps growing vertically, so there is the same problem when
> > the window starts tall (though it does fix the window starting small
> > situation, since the inner box does stop shrinking).
> 
> The problem is that then in the 'tall window' case presumably it switches to
> not having the padding as soon as the window gets any smaller...
Exactly, it starts shrinking as soon as you resize it smaller, before getting near when it actually needs to.

> > The perf impl. did occur to me, but it should only be the once at page load,
> > so it shouldn't be too bad should it?  The accounting for margins is more of
> > a problem though. Border and padding are indeed fine, I checked.  Don't
> > suppose there's a function like getBoundingClientRect() that includes the
> > margins is there?  Or anything like jQuery's outerWidth(true)? (I suspect
> > not, since it hasn't already been suggested/I can't find it)
> 
> If you look at the implementation of jQuery's outerWidth, you'll find it
> just uses getComputedStyle to get the margins and lumps them together. :-)
> 
> Note that that then breaks if you start adding them because of collapsing
> margins.
> 
> Thinking about this more: can't you just grab the last child's .bottom value
> (we can leave the margin and hope people update it if it's ever added) ?
> That should, IME, be the total value you want here (specifically, 359 in the
> current situation).
Hmm, that did occur to me (the lastChild.bottom),  I'm not sure why I didn't try it out.  It'll change the math a little (since bottom will be totalHeight + catPadding), but should work, and will handle the margins (except for a bottom margin on said lastChild, but then we aren't going to care anyway.  Only issue is what happens if said lastChild is a hidden option (like Search can be).  Will have to check...

Okay tried it out, works great. Giving it a margin-bottom there is a slight issue in that we have the same problem as the wrapping case, of the scrollbar being there while 0 < padding-top < 39px.
However it doesn't handle the last category being hidden, since that spits out bottom = 0, and since ".category:not([hidden]):last-of-type" isn't valid, I'm not sure if there is a good way to get it without walking and examining.
...or could just hack it with
#categories > .category[hidden] {
  display: -moz-box;
  visibility: collapse;
}
so its bottom is valid.

> > Hmm, you know I might only have checked on my DevEdition install.  However I
> > just re-checked on my compile with both the patch for this and bug 1036434
> > and the behaviour is the same, no auto-wrapping.  _However_, and this did
> > occur before but I failed to mention it, there is wrapping only when the
> > scrollbar created by my fix for the previous bug appears, at which point a
> > single word will wrap.
> > This is triggered in a number of languages (Greek, Chinese (all), Japanese
> > at the least).
> > Is there a good way to boost the width to pre-allocate space for the
> > scrollbar to avoid that?
> 
> Huh. Maybe that's what I was running into then. I don't know off-hand... you
> could add margins on the category list items, but I expect that that will
> mean they can't be clicked efficiently (there'll be dead zone at their
> ends), and we can't really predict the size of the scrollbar well either.
I've been trying margins on the box (didn't work), didn't think of the items... and that doesn't work either.
The other thing I tried was to get the scrollbar to always physically render but sometimes be transparent, but I'm not sure the scrollbar is a "normal" XUL one, it seems like something else.  I certainly can't get it to respond to any CSS, but maybe I'm just doing something wrong.
If it did work then overflow-y: scroll to have it always there and then making it transparent/visibility:hidden when it can't do anything would be perfect.

> Can you make a screenshot of the worst kind of wrap you see here? Last I
> checked they're vertically neatly aligned with the icon and so on, so the
> text wrapping might not be bad, esp. if it only happens on small heights
> (when the width is likely also limited).
Sure, attached.  I went for an artificial one since all the real examples I have are only two words, so they don't show how it doesn't otherwise wrap ever.  It mainly just looks weird having the scrollbar there when the padding at the top is still there.

Comment 8

[:Gijs (he/him)]

(In reply to Ian Moody (:Kwan) from comment #7)
> Created attachment 8557662 [details]
> Wrapping category name.png
> 
> (In reply to :Gijs Kruitbosch from comment #6)
> > (In reply to Ian Moody (:Kwan) from comment #5)
> > > Created attachment 8557637 [details] [diff] [review]
> > > dynamic Padding v3
> > > 
> > > I did occur to me to try the anon-boxes (though I probably would have tried
> > > using querySelector() or something), however the same problem persists,
> > > since it also keeps growing vertically, so there is the same problem when
> > > the window starts tall (though it does fix the window starting small
> > > situation, since the inner box does stop shrinking).
> > 
> > The problem is that then in the 'tall window' case presumably it switches to
> > not having the padding as soon as the window gets any smaller...
> Exactly, it starts shrinking as soon as you resize it smaller, before
> getting near when it actually needs to.
> 
> > > The perf impl. did occur to me, but it should only be the once at page load,
> > > so it shouldn't be too bad should it?  The accounting for margins is more of
> > > a problem though. Border and padding are indeed fine, I checked.  Don't
> > > suppose there's a function like getBoundingClientRect() that includes the
> > > margins is there?  Or anything like jQuery's outerWidth(true)? (I suspect
> > > not, since it hasn't already been suggested/I can't find it)
> > 
> > If you look at the implementation of jQuery's outerWidth, you'll find it
> > just uses getComputedStyle to get the margins and lumps them together. :-)
> > 
> > Note that that then breaks if you start adding them because of collapsing
> > margins.
> > 
> > Thinking about this more: can't you just grab the last child's .bottom value
> > (we can leave the margin and hope people update it if it's ever added) ?
> > That should, IME, be the total value you want here (specifically, 359 in the
> > current situation).
> Hmm, that did occur to me (the lastChild.bottom),  I'm not sure why I didn't
> try it out.  It'll change the math a little (since bottom will be
> totalHeight + catPadding), but should work, and will handle the margins
> (except for a bottom margin on said lastChild, but then we aren't going to
> care anyway.  Only issue is what happens if said lastChild is a hidden
> option (like Search can be).  Will have to check...

But Search is never a last-child... :-)

> 
> Okay tried it out, works great.

Yay! Does this route sound good to you or do you have remaining reservations beyond those addressed below?

> Giving it a margin-bottom there is a slight
> issue in that we have the same problem as the wrapping case, of the
> scrollbar being there while 0 < padding-top < 39px.

See below.

> However it doesn't handle the last category being hidden, since that spits
> out bottom = 0, and since ".category:not([hidden]):last-of-type" isn't
> valid, I'm not sure if there is a good way to get it without walking and
> examining.
> ...or could just hack it with
> #categories > .category[hidden] {
>   display: -moz-box;
>   visibility: collapse;
> }
> so its bottom is valid.

Does this actually happen? But yeah, I guess this won't hurt (significantly; don't care about the infinitesimal layout/xbl/css per-element overhead). Add a comment above it to explain why we're doing this though. :-)


> > > Hmm, you know I might only have checked on my DevEdition install.  However I
> > > just re-checked on my compile with both the patch for this and bug 1036434
> > > and the behaviour is the same, no auto-wrapping.  _However_, and this did
> > > occur before but I failed to mention it, there is wrapping only when the
> > > scrollbar created by my fix for the previous bug appears, at which point a
> > > single word will wrap.
> > > This is triggered in a number of languages (Greek, Chinese (all), Japanese
> > > at the least).
> > > Is there a good way to boost the width to pre-allocate space for the
> > > scrollbar to avoid that?
> > 
> > Huh. Maybe that's what I was running into then. I don't know off-hand... you
> > could add margins on the category list items, but I expect that that will
> > mean they can't be clicked efficiently (there'll be dead zone at their
> > ends), and we can't really predict the size of the scrollbar well either.
> I've been trying margins on the box (didn't work), didn't think of the
> items... and that doesn't work either.
> The other thing I tried was to get the scrollbar to always physically render
> but sometimes be transparent, but I'm not sure the scrollbar is a "normal"
> XUL one, it seems like something else.  I certainly can't get it to respond
> to any CSS, but maybe I'm just doing something wrong.
> If it did work then overflow-y: scroll to have it always there and then
> making it transparent/visibility:hidden when it can't do anything would be
> perfect.
> 
> > Can you make a screenshot of the worst kind of wrap you see here? Last I
> > checked they're vertically neatly aligned with the icon and so on, so the
> > text wrapping might not be bad, esp. if it only happens on small heights
> > (when the width is likely also limited).
> Sure, attached.  I went for an artificial one since all the real examples I
> have are only two words, so they don't show how it doesn't otherwise wrap
> ever.  It mainly just looks weird having the scrollbar there when the
> padding at the top is still there.

Let's leave this as-is. If people complain and file a followup bug about it we can break our heads over that one again.

Comment 9

[Ian Moody [:Kwan] (UTC+1)]

Attachment: dynamic Padding v4

(In reply to :Gijs Kruitbosch from comment #8)
> (In reply to Ian Moody (:Kwan) from comment #7)
> > Created attachment 8557662 [details]
> > Wrapping category name.png
> > 
> > Hmm, that did occur to me (the lastChild.bottom),  I'm not sure why I didn't
> > try it out.  It'll change the math a little (since bottom will be
> > totalHeight + catPadding), but should work, and will handle the margins
> > (except for a bottom margin on said lastChild, but then we aren't going to
> > care anyway.  Only issue is what happens if said lastChild is a hidden
> > option (like Search can be).  Will have to check...
> 
> But Search is never a last-child... :-)
> 
> > 
> > Okay tried it out, works great.
> 
> Yay! Does this route sound good to you or do you have remaining reservations
> beyond those addressed below?
It does, and nope. Functionally this is pretty much the same, just improved because it will account for (almost all) margins (should they ever be added), and no perf hit from reduce()ing the categories.

> > Giving it a margin-bottom there is a slight
> > issue in that we have the same problem as the wrapping case, of the
> > scrollbar being there while 0 < padding-top < 39px.
> 
> See below.
> 
> > However it doesn't handle the last category being hidden, since that spits
> > out bottom = 0, and since ".category:not([hidden]):last-of-type" isn't
> > valid, I'm not sure if there is a good way to get it without walking and
> > examining.
> > ...or could just hack it with
> > #categories > .category[hidden] {
> >   display: -moz-box;
> >   visibility: collapse;
> > }
> > so its bottom is valid.
> 
> Does this actually happen? But yeah, I guess this won't hurt (significantly;
> don't care about the infinitesimal layout/xbl/css per-element overhead). Add
> a comment above it to explain why we're doing this though. :-)
I've never seen Advanced be hidden (except deliberately during this patching), I guess I'm just being a little excessively cautious about future possibilities.  And I suppose Advanced could be hidden on customised installs possibly? (hopefully using hidden rather than display:none;)

> > I've been trying margins on the box (didn't work), didn't think of the
> > items... and that doesn't work either.
> > The other thing I tried was to get the scrollbar to always physically render
> > but sometimes be transparent, but I'm not sure the scrollbar is a "normal"
> > XUL one, it seems like something else.  I certainly can't get it to respond
> > to any CSS, but maybe I'm just doing something wrong.
> > If it did work then overflow-y: scroll to have it always there and then
> > making it transparent/visibility:hidden when it can't do anything would be
> > perfect.
> > 
> > Sure, attached.  I went for an artificial one since all the real examples I
> > have are only two words, so they don't show how it doesn't otherwise wrap
> > ever.  It mainly just looks weird having the scrollbar there when the
> > padding at the top is still there.
> 
> Let's leave this as-is. If people complain and file a followup bug about it
> we can break our heads over that one again.
Sure. Bugs me that I can't find a way to make the scrollbar invisible when needs be (or there is no value of overflow: that'll behave like such), but doesn't look like there's anything to do about it.

Comment 10

[:Gijs (he/him)]

Comment on attachment 8558235 [details] [diff] [review]
dynamic Padding v4

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

::: browser/themes/shared/incontentprefs/preferences.inc.css
@@ +37,5 @@
> + * We want the last category to always have non-0 getBoundingClientRect().bottom
> + * so we can use the value to figure out the max-height of the list in
> + * preferences.js, so use collapse instead of display: none; if it's hidden
> + */
> +#categories > .category[hidden] {

nit: hidden=true

Comment 11

[Ian Moody [:Kwan] (UTC+1)]

Attachment: dynamic Padding v4.1

Updated patch with nit addressed, and lastChild->lastElementChild as discussed on IRC.  Review carried forward.

Comment 12

[Ian Moody [:Kwan] (UTC+1)]

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dccbc149a108
Despite accidentally pushing an additional change (I clearly need to learn more about hg bookmarks) try is green with 4 unrelated intermittent oranges.
(given the date and area, I do have to wonder if bug 1125526 is my fault due to bug 972655)

Setting checkin-needed.

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/fx-team/rev/86f353eadc3a

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/86f353eadc3a

Comment 15

[:Gijs (he/him)]

(In reply to Ian Moody (:Kwan) from comment #12)
> (given the date and area, I do have to wonder if bug 1125526 is my fault due
> to bug 972655)

I looked at this, bug 972655 landed on the 21st, and 1125526 started on the 24th (a saturday, so the previous few days would have been weekdays with plenty of pushes), with several hits a day... so it seems unlikely.

Comment 16

[Ian Moody [:Kwan] (UTC+1)]

(In reply to Ian Moody (:Kwan) from comment #5) 
> Don't suppose there's a function like getBoundingClientRect() that includes the
> margins is there?  Or anything like jQuery's outerWidth(true)? (I suspect
> not, since it hasn't already been suggested/I can't find it)
Oh, look what I forgot about reading months ago:
https://hacks.mozilla.org/2014/03/introducing-the-getboxquads-api/
let fullHeight = categories.lastElementChild.getBoxQuads({box:'margin'})[0].bounds.bottom;
Shame it's not in release builds yet: bug 1107559.

(In reply to :Gijs Kruitbosch from comment #15)
> (In reply to Ian Moody (:Kwan) from comment #12)
> > (given the date and area, I do have to wonder if bug 1125526 is my fault due
> > to bug 972655)
> 
> I looked at this, bug 972655 landed on the 21st, and 1125526 started on the
> 24th (a saturday, so the previous few days would have been weekdays with
> plenty of pushes), with several hits a day... so it seems unlikely.

Ah, thanks for doing that, that's more reassuring.  I thought the dates were a bit closer.

Comment 17

[Camelia Badau [:cbadau], Desktop QA]

Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.9.5 using Firefox 38 Beta 8 (buildID: 20150427090451).