All users were logged out of Bugzilla on October 13th, 2018

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

RESOLVED FIXED in Firefox 50

Status

()

P1
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: arni2033, Assigned: enndeakin)

Tracking

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

Trunk
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox47 affected, firefox50 fixed)

Details

(Whiteboard: e10st?)

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

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

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

ER:
 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
Comment hidden (spam)
Comment hidden (spam)
(Reporter)

Comment 3

3 years ago
str
(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

Comment 4

3 years ago
Created attachment 8727368 [details]
screenshot 2 - [e10s] vs [non-e10s].png

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?
(Reporter)

Comment 5

3 years ago
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.

Updated

3 years ago
Blocks: 1154677

Comment 7

3 years ago
Created attachment 8728997 [details]
selection comparison

Comment 8

3 years ago
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
tracking-e10s: --- → +

Updated

2 years ago
Whiteboard: e10st?
(Assignee)

Comment 10

2 years ago
Created attachment 8770652 [details] [diff] [review]
Update scroll position properly

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
Status: NEW → ASSIGNED
(Assignee)

Comment 11

2 years ago
The tests in this patch depend on 1128156. Both patches together improve scrolling.
Depends on: 1128156
(Assignee)

Comment 12

2 years ago
Created attachment 8771201 [details] [diff] [review]
Update scroll position properly, with null check
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+
(Assignee)

Comment 14

2 years ago
> 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.
(Assignee)

Comment 15

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/59ab86516175cf922f7de494d2a628cc8719b480
Bug 1253975, don't reset the scroll position of a menulist when it opens as it should scroll to its selection instead, r=mconley

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/59ab86516175
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50

Updated

2 years ago
Depends on: 1304243
(Reporter)

Updated

2 years ago
Depends on: 1326736
You need to log in before you can comment on or make changes to this bug.