1304243 - [e10s] The selected value does not appear when open the <select> popup

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P1)

Description

[blinky]

Steps to reproduce:

1. Go to https://dl.dropboxusercontent.com/u/95157096/85f61cf7/qozqxd7g72.html.
2. Open the <select> element.


Actual results:

The selected value does not appear when open the <select> popup.

Screenshot https://dl.dropboxusercontent.com/u/95157096/85f61cf7/uufncl2zhw.png

Comment 1

[blinky]

Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7cd48e5cb14c647f86faa98628301959b86393cb&tochange=9b361e455e6ab4c37c9368d5b9c914b22997efb7

Comment 2

[Alice0775 White]

suspect: Bug 1253975

Comment 3

[Neil Deakin]

The issue here is that the scroll positioned is assigned during the popupshowing event, before the popup has been clipped to the screen and/or window bounds.

One easy solution is to use the upcoming popuppositioned event (bug 1206133) instead which fires after the popup's position and size has been assigned. It might cause some minor delay in opening the popup however.

Comment 4

[Jim Mathies [:jimm]]

[Tracking Requested - why for this release]:
Regression in 50 related to select popups.

Comment 5

[Marcia Knous [:marcia]]

Tracking 52+ for this regression related to popups.

Comment 6

[Gerry Chang [:gchang]]

Track for 51+ as this is a regression related to select popups.

Comment 7

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

New regression in Fx50, tracked.

Comment 8

[blinky]

This happens here too https://dl.dropboxusercontent.com/u/95157096/85f61cf7/483nx2q1ck.htm.

Screenshot https://dl.dropboxusercontent.com/u/95157096/85f61cf7/alot2vjfag.png

Comment 9

[Neil Deakin]

Attachment: Scroll current item in menulist into view when opening

Doesn't hurt to scroll the current item into view after opening the popup.

Comment 10

[Neil Deakin]

Attachment: Scroll current item in menulist into view when opening

Add some tests. Kirk offered to try having a go at reviewing this.

Comment 11

[Robin Steuber (she/her) [:bytesized]]

Comment on attachment 8798844 [details] [diff] [review]
Scroll current item in menulist into view when opening

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

Looks good! Just a few little things I wanted to mention.

::: browser/base/content/test/general/browser_selectpopup.js
@@ +63,5 @@
>    "        src='data:text/html,<select id=select autofocus><option>he he he</option><option>boo boo</option><option>baz baz</option></select>'" +
>    "</iframe>" +
>    "</div></body></html>";
>  
> +function openSelectPopup(selectPopup, withMouse, selector = "select",  win =window)

Formatting nit: Should be |win = window|.

::: layout/xul/nsMenuPopupFrame.cpp
@@ +536,5 @@
>  
> +    // Make sure the current selection in a menulist is visible.
> +    if (IsMenuList() && mCurrentMenu) {
> +      EnsureMenuItemIsVisible(mCurrentMenu);
> +    }

Since we are doing this here, do we want to remove the code that used to do this?
http://searchfox.org/mozilla-central/rev/fd672b97f13aa83af5f04caa7b61bd443fb623e9/layout/xul/nsMenuFrame.cpp#554

Comment 12

[Neil Deakin]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=60321d1c0794

Comment 13

[Neil Deakin]

(In reply to Kirk Steuber [:bytesized] from comment #11)
> 
> Since we are doing this here, do we want to remove the code that used to do
> this?
> http://searchfox.org/mozilla-central/rev/
> fd672b97f13aa83af5f04caa7b61bd443fb623e9/layout/xul/nsMenuFrame.cpp#554

No, it would just add more complexity and that code would still be needed for non-menulists.

Comment 14

[Neil Deakin]

https://hg.mozilla.org/integration/mozilla-inbound/rev/232b53f8089353e749954fb094ee84783283207e
Bug 1304243, scroll the current item into view when opening a menulist after it gets positioned so that it scrolls properly if the popup gets cropped, r=ksteuber

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/232b53f80893

Comment 16

[Julien Cristau [:jcristau]]

Neil, this is tracked as a new regression in 50, if you think this can be uplifted would you mind filling the uplift request?

Comment 17

[Neil Deakin]

Comment on attachment 8798844 [details] [diff] [review]
Scroll current item in menulist into view when opening

Approval Request Comment
[Feature/regressing bug #]: 1253975
[User impact if declined]: the selected item in a <select> is not visible for large lists
[Describe test coverage new/current, TreeHerder]: tests included
[Risks and why]: minimal, the patch just adds a scroll into view call
[String/UUID change made/needed]: none

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello blinky, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8798844 [details] [diff] [review]
Scroll current item in menulist into view when opening

Fixes a new regression in Fx50, Aurora51+, Beta50+

Comment 20

[blinky]

(In reply to Ritu Kothari (:ritu) from comment #18)
> Hello blinky, could you please verify this issue is fixed as expected on a
> latest Nightly build? Thanks!

I can not reproduce this bug with the latest Nightly build.

Comment 21

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

I'm hitting a conflict applying this to beta. Could we get a rebased patch?

Comment 22

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/59bf767c449d

Comment 23

[Neil Deakin]

Attachment: Beta version of patch

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2ae1db552424