Closed Bug 1253975 Opened 7 years ago Closed 7 years ago

[e10s] <select>'s drop-down list scrolls as close as possible to the end, not beginning


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




Tracking Status
e10s + ---
firefox47 --- affected
firefox50 --- fixed


(Reporter: arni2033, Assigned: enndeakin)


(Depends on 1 open bug, Blocks 1 open bug)


(Whiteboard: e10st?)


(5 files, 1 obsolete file)

>>>   My Info:   Win7_64, Nightly 47, 32bit, ID 20160229030448
1. Open "testcase 1"
2. Click the <select>'s dropmarker

 The drop-down list is scrolled as close to the end as possible. Selected option is visible

 The drop-down list should be scrolled as close to the beginning as possible, because it matches
 (1) non-e10s,  (2) menulists in about:preferences#content -> Advanced,  (3) GoogleChrome
(4) because there may be some <optgroup>s like in attachment 8672344 [details]
On that page, user can't see the name of oprgroup when <select> is just opened
I investigated them using the Browser Toolbox (remote debugging).
In case of non-e10s, elements are structured by <select><option>. But in case of e10s, elements are structured by <menulist><menupopup><menuitem>. So, e10s is affected by active theme. Is this correct?
I see that difference too, but I don't understand what did you mean by "e10s is affected by active theme". Because, yes, CSS related to XUL menulists applies to those "options" too. But this bug is about initial scroll position, I just don't see how styling issues are related
Just in case: bug 910022 will allow styling of those menuitems in multi-process mode.

Also, (2) is incorrect. Those menulists just scroll to the very beginning when I open it. Huh.
In my opinion, all are handling incorrectly.  Ideally, the scroll position of a pre-selected item should be as close to center of the list view, as possible, not shifted to top or bottom of the list view. In my opinion this would better show context of the selected item within the list.

However, to be in parody with non-e10s and Chrome, e10s should shift the selected item to the top of the list view.
Blocks: e10s-select
Attached image selection comparison
jeff, does this block?
Flags: needinfo?(jgriffiths)
(In reply to Jim Mathies [:jimm] from comment #8)
> jeff, does this block?

No, the web content is not broken, it is not even effectively different ( selected option is preserved correctly ) it is instead a visual discrepancy.

I would reconsider if there was an example where an interaction model ( mouse, kb, a11y, whatever ) was degraded in usefulness significantly.
Flags: needinfo?(jgriffiths)
Priority: -- → P1
Whiteboard: e10st?
Attached patch Update scroll position properly (obsolete) — Splinter Review
For menulists, don't reset the scroll position to 0 when they open. This ensures that the later call to nsMenuPopupFrame::EnsureMenuItemIsVisible isn't overridden.

This means that the call to scrollIntoView isn't needed anymore.
Assignee: nobody → enndeakin
The tests in this patch depend on 1128156. Both patches together improve scrolling.
Depends on: 1128156
Attachment #8770652 - Attachment is obsolete: true
Attachment #8771201 - Flags: review?(mconley)
Comment on attachment 8771201 [details] [diff] [review]
Update scroll position properly, with null check

Review of attachment 8771201 [details] [diff] [review]:

::: browser/base/content/test/general/browser_selectpopup.js
@@ +395,5 @@
> +      let bpBottom = parseFloat(cs.paddingBottom) + parseFloat(cs.borderBottomWidth);
> +
> +      is(selectPopup.childNodes[60].getBoundingClientRect().bottom,
> +         selectPopup.getBoundingClientRect().bottom - bpBottom,
> +         "Popup scroll at correct position " + bpBottom);

We're using top to calculate positioning in test_menulist_paging.xul - is there a reason why we're using bottom over here?

::: toolkit/content/tests/chrome/test_menulist_paging.xul
@@ +125,5 @@
> +  let cs = window.getComputedStyle(menulist.menupopup);
> +  let bpTop = parseFloat(cs.paddingTop) + parseFloat(cs.borderTopWidth);
> +
> +  // Skip menulist3 as it has a label that scrolling doesn't need normally deal with.

Can you go into more detail about what "scroll" means?\ in the tests array schema? Like, it means the index of a child that we expect to be at the top of the menulist, right?
Attachment #8771201 - Flags: review?(mconley) → review+
> We're using top to calculate positioning in test_menulist_paging.xul - is
> there a reason why we're using bottom over here?

The menulist scrolls such that the bottom of the item is aligned with the bottom of the popup (as the bug title suggests). I could do the same for the other tests I guess, but why not test both top and bottom anyway.
Bug 1253975, don't reset the scroll position of a menulist when it opens as it should scroll to its selection instead, r=mconley
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1304243
Depends on: 1326736
You need to log in before you can comment on or make changes to this bug.