Alice0775 White Reporter
	Description • 9 years ago

Steps to reproduce
1. open e10s window
2. open attached
3. Zoom-in and/or Zoom-out

Actual Results:
<select> dropdown list does not zoom

Expected Results:
<select> dropdown list should zoom

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 1 • 9 years ago

So this applies the scale() transform to the menupopup, which works, but this brings back the issue of the anchoring element being too narrow causing text to be invisible. Basically, it seems that the preferred width of the popup is being calculated pre-transform, so the popup ends up being made the width of the anchoring element which can be too narrow (same as bug 1168212 but this affects the zoomed case only).

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 2 • 9 years ago

I think this is a better approach, given the limitations of calculating the preferred size when using CSS transforms. As a <select> can only contain text this should be fine.

	Neil Deakin
	Comment 3 • 9 years ago

OK, maybe we can revisit the transform option, once the width computation is done better.

	Mike Conley (:mconley) (:⚙️)
	Comment 4 • 9 years ago

Comment on attachment 8616071 [details] [diff] [review]
0001-Bug-1128162-Scale-the-select-popup-s-text-to-match-t.patch

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

::: toolkit/modules/SelectParentHelper.jsm
@@ +72,5 @@
> +
> +  // 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");

I suspect that for a <select> with many optgroups, this patch will cause us to do a synchronous reflow for each optgroup, since we're getting layout to compute a CSS value for us (font-size) after mucking about and adding elements.

Or perhaps layout is smart enough to realize that a child element can't affect the menupopup's font-size. I'm not sure.

I think what I'd prefer, if possible, is to get the textSize once, and pass it into the recursive function. Or, even better - get the textSize once, and set it once on the entire popup instead of each element. Have you tried that?

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 5 • 9 years ago

Updated to pass the adjustedTextSize through when recursing on populateChildren. Setting the fontSize style on the menulist or the menupopup doesn't work for this.

	Mike Conley (:mconley) (:⚙️)
	Comment 6 • 9 years ago

Comment on attachment 8616812 [details] [diff] [review]
0001-Bug-1128162-Scale-the-select-popup-s-text-to-match-t.patch

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

::: toolkit/modules/SelectParentHelper.jsm
@@ +70,4 @@
>    let index = startIndex;
>    let element = menulist.menupopup;
>  
> +  if (adjustedTextSize == -1) {

Ugh. Alright - mind putting in a comment noting that this hack is just so that we compute this only the first time?

Recursion. :/

@@ +97,4 @@
>      }
>  
>      if (isOptGroup) {
> +      index = populateChildren(menulist, option.children, selectedIndex, zoom, 

Nit: whitespace

	Pulsebot
	Comment 7 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/f49c57b57b43

	Phil Ringnalda (:philor)
	Comment 8 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/f49c57b57b43

	Md. Rahimul Islam
	Comment 9 • 9 years ago

I have successfully reproduced the bug in firefox Nightly 38.0a1 (2015-01-31) with windows 7 (32 bit) 

Verified as fixed with 43.0a1 as comment "0"

Build ID:   20150820030202
Mozilla/5.0 (Windows NT 6.1; rv:43.0) Gecko/20100101 Firefox/43.0

[testday-20150821]