Closed Bug 1304243 Opened 3 years ago Closed 3 years ago

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

Categories

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

50 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
e10s + ---
firefox49 --- unaffected
firefox50 + fixed
firefox51 + fixed
firefox52 + fixed

People

(Reporter: over68, Assigned: enndeakin)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

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
Flags: needinfo?(alice0775)
suspect: Bug 1253975
Blocks: 1253975
Status: UNCONFIRMED → NEW
tracking-e10s: --- → ?
Ever confirmed: true
Flags: needinfo?(alice0775)
Keywords: regression
Version: 52 Branch → 50 Branch
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.
[Tracking Requested - why for this release]:
Regression in 50 related to select popups.
Tracking 52+ for this regression related to popups.
Track for 51+ as this is a regression related to select popups.
New regression in Fx50, tracked.
Severity: normal → critical
Priority: -- → P1
See Also: → 1305017
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
OS: Windows 7 → All
Hardware: x86_64 → All
Doesn't hurt to scroll the current item into view after opening the popup.
Add some tests. Kirk offered to try having a go at reviewing this.
Attachment #8798844 - Flags: review?(ksteuber)
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
Attachment #8798844 - Flags: review?(ksteuber) → review+
Attachment #8798042 - Attachment is obsolete: true
(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.
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
https://hg.mozilla.org/mozilla-central/rev/232b53f80893
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Neil, this is tracked as a new regression in 50, if you think this can be uplifted would you mind filling the uplift request?
Flags: needinfo?(enndeakin)
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
Flags: needinfo?(enndeakin)
Attachment #8798844 - Flags: approval-mozilla-beta?
Attachment #8798844 - Flags: approval-mozilla-aurora?
Hello blinky, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(over68)
Comment on attachment 8798844 [details] [diff] [review]
Scroll current item in menulist into view when opening

Fixes a new regression in Fx50, Aurora51+, Beta50+
Attachment #8798844 - Flags: approval-mozilla-beta?
Attachment #8798844 - Flags: approval-mozilla-beta+
Attachment #8798844 - Flags: approval-mozilla-aurora?
Attachment #8798844 - Flags: approval-mozilla-aurora+
(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.
Flags: needinfo?(over68)
I'm hitting a conflict applying this to beta. Could we get a rebased patch?
Flags: needinfo?(enndeakin)
Flags: needinfo?(enndeakin)
You need to log in before you can comment on or make changes to this bug.