All users were logged out of Bugzilla on October 13th, 2018
1.15 KB, text/html
61.86 KB, image/png
107.55 KB, image/png
40.61 KB, image/jpeg
8.10 KB, patch
|Details | Diff | Splinter Review|
>>> 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
(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
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?
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.
jeff, does this block?
(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.
Priority: -- → P1
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
The tests in this patch depend on 1128156. Both patches together improve scrolling.
Depends on: 1128156
Created attachment 8771201 [details] [diff] [review] Update scroll position properly, with null check
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.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.
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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.