1112243 - [e10s] <select> dropdowns are blank, if there are many options (e.g. at http://www.bls.gov/data/inflation_calculator.htm )

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

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

STR:

1) Go to http://www.bls.gov/data/inflation_calculator.htm
2) Click on the dropdown to select the year

ER:

Should see scrollable popup listing the years I can select

AR:

A popup appears, and it's scrollable, but it's also blank.

We might not be serializing our menupopup correctly.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Comment 3

[Daniel Holbert [:dholbert]]

Broadening summary & adding keyword & dependency from duplicate-bug 1112248.

Comment 5

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

Attachment: [e10s] Set sizetopopup="always" on ContentSelectDropdown. r=?

The XUL menulist popup that we use for displaying <html:select> dropdowns
from the content process is sometimes not wide enough to properly display
the labels inside of it - especially if there is a scrollbar. The sizetopopup
attribute allows us to make sure that the popup assumes its preferred size
and is not constrained by the size of the menulist item. This ensures that
all labels are displayed properly.

Comment 6

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

Comment on attachment 8555996 [details] [diff] [review]
[e10s] Set sizetopopup="always" on ContentSelectDropdown. r=?

The XUL menulist popup that we use for displaying <html:select> dropdowns
from the content process is sometimes not wide enough to properly display
the labels inside of it - especially if there is a scrollbar. The sizetopopup
attribute allows us to make sure that the popup assumes its preferred size
and is not constrained by the size of the menulist item. This ensures that
all labels are displayed properly.

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8555996 [details] [diff] [review]
[e10s] Set sizetopopup="always" on ContentSelectDropdown. r=?

I don't think I understand this patch. https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/menulist says:

"If the sizetopopup attribute is left out or set to none, the menu will be its preferred width and the popup may extend outside of this width, unaffected by the maximum width of the menu itself."

which is pretty much what we want, isn't it?

Comment 8

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

So I dug into this more, because you're right Dao - that documentation doesn't really make a whole lot of sense with what I'm seeing.

It looks like this sizetopopup fix is a bit of a hack that's taking advantage of the fact that at the point of laying out the menupopup, the menulist should have calculated its preferred width to match the needs of the menupopup. Then, this bit of code executes in nsMenuPopupFrame::LayoutPopup:

  // get the preferred, minimum and maximum size. If the menu is sized to the
  // popup, then the popup's width is the menu's width.
...
  if (aSizedToPopup) {
    prefSize.width = aParentMenu->GetRect().width;
  }

Which makes it so that the preferred size of the menupopup matches that of the menulist. And that's what's causing my patch to work. aSizedToPopup is only ever true in this context if sizetopopup="always" or "pref".

So, to sum, I guess this is a hack that's taking advantage of this decision to set the preferred size of the popup to match the parent when sizetopopup="always" or "pref".

Maybe the better solution would be to make it so that for long menulists (with a scrollbar) with short labels, the menupopup does a better job of calculating its preferred size so that labels are displayed[1].

Neil, does that sound right?

[1]: Because after testing it, it seems like this is true for all <xul:menulists>. Without a sizetopopup attribute set to "pref" or "always", long menulists with a scrollbar and short labels cause the labels to get squished out so that they're not visible.

Comment 9

[Neil Deakin]

What you should do is add:

    <content sizetopopup="pref">

to the menulist-popuponly binding, as the other menulist bindings have, so they always default to sizing themselves properly.

That's not actually the width you want though for the ContentSelectDropdown; you actually want the popup to be same width as the <select> element. But that's best done after 873923.

Comment 10

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

Yes, that works nicely. Thanks!

Comment 11

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

Attachment: add sizetopref to menulist-popuponly binding content. r=?

The rest of the menulist bindings have this, and is used to properly size
the menupopup when the menulist is opened. Without it, for long menulists
with a scrollbar, and short labels, the labels will get squeezed out during
layout and not be visible.

Comment 12

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

Comment on attachment 8556557 [details] [diff] [review]
add sizetopref to menulist-popuponly binding content. r=?

This works well! Asking for review from Dao since it was Enn that suggested this change in the first place.

Comment 13

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

Thanks!

remote:   https://hg.mozilla.org/integration/fx-team/rev/a465bbe65e90

Comment 14

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

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

Comment 15

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

Attachment: Bugnotes

http://people.mozilla.org/~mconley2/bugnotes/bug-1112243.html

Comment 17

[Daniel Holbert [:dholbert]]

Per dupe bug 1168247, this is not actually fixed. (The testcase attached here -- attachment 8537362 [details] -- still reproduces the bug as well.)

Reopening.

Comment 18

[Daniel Holbert [:dholbert]]

I'm seeing this bug (blank dropdown) w/ attached testcase, in latest nightly on Linux, with a fresh profile. Nightly 41.0a1 (2015-05-25)

Comment 19

[Daniel Holbert [:dholbert]]

(Looks like this was really fixed for a while, and then regressed. I tested a random Nightly between comment 14 and now (2015-02-16) and it shows the correct rendering.  For sanity's sake, I'll re-close this, since it was fixed at one point, and reclassify bug 1168247 as a regression.)