1309938 - The padding of options and optgroups in select dropdowns should scale relatively with the font-size of the items

Status: RESOLVED DUPLICATE of bug 1316722

(Core :: Layout: Form Controls, defect)

Description

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

The select dropdown gets the computed font-size from the <select> element and applies it to the font-size of the menuitem and menucaption elements in the popup that is shown. We should adjust the padding to match the adjusted font-size as necessary.

For example, by default a menuitem has font-size 11px, top padding of 4px, and bottom padding of 4px. 

If the font-size is 14px, we should have a top and bottom padding of 5px each.

We can use the 4/11 ratio as the basis for scaling the padding-size. In the event that the device is touch-enabled, we should use 11/11 (1:1) ratio since touch-enabled uses 11px vertical padding.

Comment 1

[Mark Golbeck]

Attachment: Bug 1309938 - The padding of options and optgroups in select dropdowns should scale relatively with the font-size of the items.

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

Comment 2

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

Comment on attachment 8802712 [details]
Bug 1309938 - The padding of options and optgroups in select dropdowns should scale relatively with the font-size of the items.

https://reviewboard.mozilla.org/r/87032/#review86364

::: toolkit/modules/SelectParentHelper.jsm:151
(Diff revision 1)
>  };
>  
>  function populateChildren(menulist, options, selectedIndex, zoom,
>                            parentElement = null, isGroupDisabled = false, adjustedTextSize = -1) {
>    let element = menulist.menupopup;
> +  let paddingRatio = 4/11;

Quick feedback: this patch overall is good and in the right direction, but we shouldn't hardcode the 4 and 11 here since they might get changed in the other place but not get updated here. And themes can change the styling and then this will be out of sync.

You should be able to use getComputedStyle() to retrieve the values from the popup, though it might not work since the popup hasn't been displayed yet and styles haven't been applied.

::: toolkit/modules/SelectParentHelper.jsm:162
(Diff revision 1)
>  
>      // Grab the computed text size and multiply it by the remote browser's fullZoom to ensure
>      // the popup's text size is matched with the content's. We can't just apply a CSS transform
>      // here as the popup's preferred size is calculated pre-transform.
>      let textSize = win.getComputedStyle(element).getPropertyValue("font-size");
> -    adjustedTextSize = (zoom * parseFloat(textSize, 10)) + "px";
> +    var adjustedTextRatio = (zoom * parseFloat(textSize, 10));

note, you can't use 'let' here instead of 'var' because 'let' will scope the variable to this block. You should declare the adjustedTextRatio variable outside of this block, and then assign its value inside of the block.

let adjustedTextRadio;
if (adjustedTextSize == -1) {
 ...
 adjustedTextRatio = ...
}
...

Comment 3

[Mark Golbeck]

Comment on attachment 8802712 [details]
Bug 1309938 - The padding of options and optgroups in select dropdowns should scale relatively with the font-size of the items.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87032/diff/1-2/

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8802712 [details]
Bug 1309938 - The padding of options and optgroups in select dropdowns should scale relatively with the font-size of the items.

https://reviewboard.mozilla.org/r/87032/#review88094

::: toolkit/modules/SelectParentHelper.jsm:163
(Diff revision 2)
>  
>      // Grab the computed text size and multiply it by the remote browser's fullZoom to ensure
>      // the popup's text size is matched with the content's. We can't just apply a CSS transform
>      // here as the popup's preferred size is calculated pre-transform.
>      let textSize = win.getComputedStyle(element).getPropertyValue("font-size");
> -    var adjustedTextRatio = (zoom * parseFloat(textSize, 10));
> +    let padding = win.getComputedStyle(element).getPropertyValue("padding-top");

Is this the padding you're attempting to query for: http://searchfox.org/mozilla-central/rev/4012e61cc38758ffa1f7ce26039173f67b048853/browser/themes/windows/browser.css#2671 ?

Because "element" in this case is the menupopup, and not either a menucaption or a menuitem, which would probably explain why you're not getting the expected value. Or am I misunderstanding?

Comment 5

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

Comment on attachment 8802712 [details]
Bug 1309938 - The padding of options and optgroups in select dropdowns should scale relatively with the font-size of the items.

https://reviewboard.mozilla.org/r/87032/#review88606

mconley's review comments here are sufficient.

Comment 6

[Justin Dolske [:Dolske]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #0)

> For example, by default a menuitem has font-size 11px, top padding of 4px,
> and bottom padding of 4px. 

Random drive-by... Can we be clever here and just specify the CSS padding in em/ex units? (Picking some multiplier so that that it's equivalent to 4px when the text is 11px.) That should automagically make the padding scale with the text size, no JS needed.

Although I don't know if we're trying to match some platform style precisely, or if we need greater control.

Just a thought. :)

Comment 7

[Mark Golbeck]

Im trying to implement what Justin Dolske (:Dolske) is suggesting. His suggestion made me think of using rem units for scaling. Here is what I have so far:

#ContentSelectDropdown > menupopup > menucaption,
#ContentSelectDropdown > menupopup > menuitem {
  padding: 4px 6px;
}

#ContentSelectDropdown > menupopup > menucaption > .menu-iconic-text,
#ContentSelectDropdown > menupopup > menuitem > .menu-iconic-text {
  font: message-box;
  font-size: 0.9167rem;    /* base is 12px rem converts to 11px */
  padding-top: 0.3333rem;  /* make proportional to 4 px */ 
  padding-bottom: 0.3333rem;
}

That works in the default case. The base element is 12 px. So i scale the padding to 4px for top and bottom using rem units. I also scale the text down to 11px. However when i change the text size it doesnt scale with it. For example, if i use 14px the padding should scale to 5px but it does not. 

Any help would be appreciated. I have already asked in the irccloud channels and developers said it would be best to ask here.

Comment 8

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

Mark, please test out the patch in bug 1316722. I believe it will take care of half of this bug since the padding will now scale with the size of the font as long as full-zoom and text-zoom is used (activated by using Ctrl+ or Ctrl-).

The patch there doesn't let the popup inherit the font-size of the <select> element. That will need to be handled by 910022, but it doesn't look like bug 910022 will support font-size (only color and background-color for now).

If the patch in bug 1316722 fixes the issue here, then we may be close this bug as a duplicate of bug 1316722 and be done with this work.

Comment 9

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

This is fixed by bug 1316722 and this bug isn't different enough from bug 1316722 to warrant keeping it around separately.