Closed Bug 1053981 Opened 10 years ago Closed 10 years ago

[e10s] Wrong <select> dropdown widget on Windows and Linux

Categories

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

x86
All
defect

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
e10s m4+ ---

People

(Reporter: mconley, Assigned: ally)

References

(Blocks 1 open bug)

Details

Attachments

(8 files, 1 obsolete file)

Attached image Reference for Windows 7 (deleted) —
We're using a non-native widget for the <select> HTML form input dropdowns in e10s windows. This appears to be true on Windows and Linux GTK. The widget we're using appears to be a "context menu" style popup, with scrollbuttons at the top and the bottom. To complicate things further, we do the same thing on OS X, except this appears to be a "native" approach. Safari, for example, uses such a popup with the top and bottom scrollbuttons. I talked to shorlander about OS X to get UX's take, and he said that in his opinion, it'd be better to have a scrollbar there, to give users the sense of how large the list is, and where in that list they are. These are things that the scrollbuttons do not convey at all (or very well). So, I think we should switch Windows and Linux GTK to use widgets more in-line with the non-e10s widgets. If they look more "native", great. But I think we could also accept straight-up parity with non-e10s. I'll upload a reference for Linux GTK in a few minutes.
OS: Windows 7 → All
Blocks: 1049285
Blocks: fxe10s
Attached image Reference for Ubuntu (deleted) —
So actually, the story on Ubuntu is kinda shake-y. I see a variety of different styles used by different tools. Chromium rolls their own like Firefox non-e10s. Epiphany does the scrollbuttons thing.
Hey jwatt, Ms2ger suggested you as someone I should talk to HTML form element styling. There are some folks who are suggesting that e10s is an opportunity to making the <select> popups more "native", but I worry that some "native" options are actually less usable than what we currently ship. The scrollbuttons that GTK and OS X use, for example, strike me as clumsy, and like shorlander told me, it'd hard to know where one is in a long list. Can I assume that it's probably in our best interest to just re-use the same styling and widgetry that we're using for non-e10s?
Flags: needinfo?(jwatt)
Attached image windows selects.png (deleted) —
I think we might be able to get away with a non-native look, but I think the existing widget needs to be improved first, at least on windows. The current menu feels cumbersome to interact with. Specifically: 1) the black selection dot needs to change to a highlight across the entire item 2) the menu needs a draggable scroll bar 3) alignment is broken 4) inline selects and popup selects should be more aligned visually
Attached file forms test cases (deleted) —
The fact that you're ending up with something totally different in e10s seems like a bizarre bug. While there certainly could be stylistic improvements made, using a bug as the basis for them seems like a bad idea.
Probably not that helpful to comment here, but just to have it out there - I really like that the e10s drop-downs show the currently selected item.
Depends on: 897060
Ok, jimm and I talked to felipe, and it sounds like what happened was that we just needed to stand up an implementation of <select> popups to avoid a crash. That was done, but it was done on a Mac (where the scrollbutton style popup is more native), and not a whole lot of time was put into investigating solutions for the other platforms. I think we're going to see if we can simply swap in the different widget - the one we ship in non-e10s - and see what challenges that causes us to run into.
Clearing needinfo? on jwatt - I think we have a plan going forward. I think we're going to have to decide at some point if we want to stick with the current native-y widget for OS X (despite the reduced usability), or go back to what non-e10s currently ships, but let's see if we can at least fix up Windows and Linux first.
Flags: needinfo?(jwatt)
(In reply to Mike Conley (:mconley) from comment #10) > Ok, jimm and I talked to felipe, and it sounds like what happened was that > we just needed to stand up an implementation of <select> popups to avoid a > crash. That was done, but it was done on a Mac (where the scrollbutton style > popup is more native), and not a whole lot of time was put into > investigating solutions for the other platforms. > > I think we're going to see if we can simply swap in the different widget - > the one we ship in non-e10s - and see what challenges that causes us to run > into. Also, there are three valid bugs here - this bug - dealing with the wrong widget(s) on Windows and Linux (fallout from bug 897060) bug 910022 - deals with shipping page style over from content bug 910023 - deals with proper width/height/positioning, which can be dependent on page styling I'm renoming this for triage, I think this bug is a big enough deal that we should reconsider it for m2.
Assignee: nobody → jmathies
Blocks: old-e10s-m2
Priority: -- → P3
(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) (away/busy Aug 27-Sep 11) from comment #7) > The fact that you're ending up with something totally different in e10s > seems like a bizarre bug. While there certainly could be stylistic > improvements made, using a bug as the basis for them seems like a bad idea. I guess I should take this back -- it's not bizarre given that bug 897060 gave us an entirely separate implementation of the dropdown for e10s. (And, actually, now that I look at the native controls, on Linux, the dropdown with e10s actually seems a lot closer to native than without.)
(In reply to Mike Conley (:mconley) from comment #11) > we're going to have to decide at some point if we want to stick with the > current native-y widget for OS X (despite the reduced usability), or go back > to what non-e10s currently ships, but let's see if we can at least fix up > Windows and Linux first. IMHO, the form controls should always follow the native style for platform consistency. I don't know if you can actually use the native widgets (vs re-creating them visually) but ideally it'd be desirable since this should give consistency and a few other things (like proper voice-over) for free. Otherwise, please recreate the look & feel as close to the native style as possible! On a more personal note, I vastly perfer the native look & feel of dropdowns on OS X than the rather ugly Windows-y style from the 90ies ...
Blocks: 1010732
Priority: P3 → P2
Status: NEW → ASSIGNED
Blocks: 1058881
Moving old M2 P2 bugs to M4.
Move old M2's low-priority bugs to M6 milestone.
oops: old M2 P2 bugs should have been moved to new M4 milestone, not M6.
Blocks: 1069463
Blocks: 1069355
Status: ASSIGNED → NEW
A good fix has been a bit of a pita to find here. Our current implementation simulates the select drop down by intercepting events and forwarding them over to the parent, where we create a xul popup with menuitems in it for the list. There are a couple issues with this implementation - our built-in combobox implementation uses a listbox vs. the e10s menupop. This was pretty easy to fix up. The second problem is with styling - since we create the popup in xul, we get xul styling for the listbox and listitems. In html the styling of select/option elements (including the drop down) is completely different. So even though we're using the right container, the css is still off. I've been tweaking xul css to try and match html css as a work around. This all seems rather hackish to me. Maybe I'm missing a better approach. Gavin, any ideas here?
Flags: needinfo?(gavin.sharp)
Note, earlier on I did look at trying to remote the drop down window from within toolkit. That might be worth revisiting, although there are similar issues with getting the right widget created and rendered properly on the parent side.
Attached image listbox wip (deleted) —
Spoke with blassey about this today, he suggested chatting with neil. Also it was suggested I might be able to get html styling by using the html namespace for elements. Will experiment around with that. Neil, any suggestions here? We are in need of a drop down styled like a combobox drop down, but managed by the browser.
Flags: needinfo?(gavin.sharp) → needinfo?(enndeakin)
If your goal is to make it look and behave like the current <select> element, I'd imagine the simplest approach would be to just create an <html:select> element and then change it such that the button area has no size and doesn't draw anything. (You will likely need to change nsListBoxFrame.cpp to do this). Then you'll need to have some way to programmatically show the popup. Some specific behaviours you may not be able to emulate easily, such as pressing the mouse to open and dragging to select an item in this case, but this route would give the most compatibility. The select element implementation is fairly complex so if you wanted to continue to use a menulist you would be sacrificing quite a bit of behaviour.
Flags: needinfo?(enndeakin)
Blocks: 1082510
> (You will likely need to change nsListBoxFrame.cpp to do this). I meant nsListControlFrame.cpp of course.
(In reply to Neil Deakin from comment #26) > If your goal is to make it look and behave like the current <select> > element, I'd imagine the simplest approach would be to just create an > <html:select> element and then change it such that the button area has no > size and doesn't draw anything. (You will likely need to change > nsListBoxFrame.cpp to do this). > > [...] > > The select element implementation is fairly complex so if you wanted to > continue to use a menulist you would be sacrificing quite a bit of behaviour. Please make sure this doesn't break OS X – the currently (e10s) implemented select is displayed correctly here (it used to have a non-native look).
Attached patch 1053981-select (deleted) — Splinter Review
This is the wip I had to get this window showing up better, but there were a lot of remaining issues.
Attachment #8523988 - Attachment is patch: true
(In reply to Florian Bender from comment #29) > (In reply to Neil Deakin from comment #26) > > If your goal is to make it look and behave like the current <select> > > element, I'd imagine the simplest approach would be to just create an > > <html:select> element and then change it such that the button area has no > > size and doesn't draw anything. (You will likely need to change > > nsListBoxFrame.cpp to do this). > > > > [...] > > > > The select element implementation is fairly complex so if you wanted to > > continue to use a menulist you would be sacrificing quite a bit of behaviour. > > Please make sure this doesn't break OS X – the currently (e10s) implemented > select is displayed correctly here (it used to have a non-native look). Actually we'll be going back to the non-native look. If you don't like that, you'll have to file a general bug in core about it. In this bug our goal to get things looking exactly like non-e10s.
Assignee: jmathies → ally
Any chance to get bug 1091592 fixed as part of this? Our select dropdowns have been ugly for too long.
Flags: needinfo?(ally)
(In reply to Philipp Sackl [:phlsa] from comment #33) > Any chance to get bug 1091592 fixed as part of this? > Our select dropdowns have been ugly for too long. You'll need to convince :jimm or :blassey to work that in. Otherwise the marching orders in Comment 31 stand.
Flags: needinfo?(ally)
Blocks: 1090602
Hey ally - I'm curious to know how this is going to get architected. Specifically, are you thinking that we'll keep the SelectParentHelper.jsm/SelectContentHelper.jsm team to do communications between the parent and child for this widget, or are we planning on making a new IPDL and doing communications directly over IPC that way? Asking because bug 1090602 relies on that information. If we're somehow able to do the communications directly over IPC, we might be able to get the event serializations defined in nsGUIEventIPC.h for free, which would be nice. Otherwise, we'll need to write event serializers / deserializers in those JSM's for all intrinsic events on those <option> elements. Curious to hear how you're thinking of proceeding on that front.
Flags: needinfo?(ally)
discussed on irc. We'll keep the the jsms for now, until there is a compelling need to switch. Minimum viable patch & all that.
Flags: needinfo?(ally)
Blocks: 1103635
¡Hola señor Conley! Is this why the drop-down options don't use the entire width of the selector or shall I file a new bug for that? ¡Gracias! Alex
Flags: needinfo?(mconley)
@alex_mayorga: ¡Hola Yes, this is likely related to that. :) So the fix, or at least, putting us in a better state, might be as easy as swapping out the binding for the popup. I'm still waiting for my build to finish on Windows to test the behaviour there with this patch, but this should move us from using a scrollarrow popup to using one with a proper scrollbar.
Flags: needinfo?(mconley)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #42) > @alex_mayorga: ¡Hola Yes, this is likely related to that. :) > > So the fix, or at least, putting us in a better state, might be as easy as > swapping out the binding for the popup. I'm still waiting for my build to > finish on Windows to test the behaviour there with this patch, but this > should move us from using a scrollarrow popup to using one with a proper > scrollbar. This looks and works much better on Windows, although it's not a perfect match with the non-e10s dropdown. I think we should go ahead and land this since it's an improvement on the current dropdown. Depending on feedback / issues we find, we might be able to roll out with this.
Comment on attachment 8532643 [details] [diff] [review] use the popup-scrollbars binding for <select> dropdowns in e10s windows. r=? As a short-term bandaid fix, I think this is probably better than what we've currently got. What I think we should spend time doing is adding a menulist-menupopup class that lets us apply the rules for menulist > menupopup (and the menuitems underneath it) with a class instead of just the menulist > menupopup child requirement. That'll take a little bit of work to get right (not too too bad though), but we can probably take this patch now and follow-up with the other stuff after.
Attachment #8532643 - Flags: review?(ally)
Comment on attachment 8532643 [details] [diff] [review] use the popup-scrollbars binding for <select> dropdowns in e10s windows. r=? Review of attachment 8532643 [details] [diff] [review]: ----------------------------------------------------------------- it's a 1.0 and a hellva improvement. r+ provided you file said followup bug with a brain dump so we can pick it up later if need be.
Attachment #8532643 - Flags: review?(ally) → review+
mconley has been struck with the mozplague remote: https://hg.mozilla.org/integration/fx-team/rev/dfef8b5c91e1
Thanks for landing, ally! I guess we can keep doing the follow-up work in here. The follow-up work, in my mind, is as follows: 1) Open DOM Inspector 2) Disable in-content prefs (browser.preferences.inContent to false) 3) In the Preferences dialog, examine the "Default font" menupopup element (and its children) to get a sense of what rules are being applied to the items inside the menupopup when under a menulist 4) Augment the current set of rules to also allow a menulist-menupopup class as well as the menulist > menupopup relationship 5) Apply the menulist-menupopup class to the ContentSelectDropdown menupopup node in browser.xul, and remove the binding rule we added in comment 46. 6) \o/ ? I've got a partial patch I started while on a plane back to Toronto, but I only got a little part of the way on Windows only, and then I passed out. I'll post that now.
Whiteboard: [leave-open]
ally - did you want to pick up where I left off with this patch (comment 48)? I don't think bug 1090602 is really blocked (conceptually) on this bug anymore, so I think I can get out of your hair now.
Flags: needinfo?(ally)
Comment on attachment 8533304 [details] [diff] [review] WIP patch to add menulist-menupopup class (Windows only at the moment) I've filed bug 1108761 for the follow-up work that we'll need in Toolkit, and I'll move the patch over there as well.
Attachment #8533304 - Attachment is obsolete: true
Since we're not doing the follow-up work in here, I'll remove the [leave-open].
Whiteboard: [leave-open]
Flags: needinfo?(ally)
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Depends on: 1108761
Blocks: 1108761
No longer depends on: 1108761
Depends on: 1112248
Depends on: 1112243
No longer depends on: 1112248
Depends on: 1112381
Blocks: 1112646
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: