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

Status: RESOLVED FIXED

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

Description

[Mike Conley (:mconley) (:⚙️)]

Attachment: Reference for Windows 7

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.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: Reference for Ubuntu

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.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

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?

Comment 5

[Jim Mathies [:jimm]]

Attachment: windows selects.png

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

Comment 6

[Jim Mathies [:jimm]]

Attachment: forms test cases

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 9

[Chris Lord [:cwiiis]]

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.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 12

[Jim Mathies [:jimm]]

(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.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.)

Comment 14

[Florian Bender]

(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 ...

Comment 15

[Chris Peterson [:cpeterson]]

Moving old M2 P2 bugs to M4.

Comment 16

[Chris Peterson [:cpeterson]]

Move old M2's low-priority bugs to M6 milestone.

Comment 17

[Chris Peterson [:cpeterson]]

oops: old M2 P2 bugs should have been moved to new M4 milestone, not M6.

Comment 22

[Jim Mathies [:jimm]]

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?

Comment 23

[Jim Mathies [:jimm]]

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.

Comment 24

[Jim Mathies [:jimm]]

Attachment: listbox wip

Comment 25

[Jim Mathies [:jimm]]

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.

Comment 26

[Neil Deakin]

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.

Comment 27

[Neil Deakin]

> (You will likely need to change nsListBoxFrame.cpp to do this).

I meant nsListControlFrame.cpp of course.

Comment 29

[Florian Bender]

(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).

Comment 30

[Jim Mathies [:jimm]]

Attachment: 1053981-select

This is the wip I had to get this window showing up better, but there were a lot of remaining issues.

Comment 31

[Jim Mathies [:jimm]]

(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.

Comment 33

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Any chance to get bug 1091592 fixed as part of this?
Our select dropdowns have been ugly for too long.

Comment 34

[Allison Naaktgeboren :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.

Comment 37

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 38

[Allison Naaktgeboren :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.

Comment 40

[alex_mayorga]

Attachment: The drop-down options don't use the entire width.

¡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

Comment 41

[Mike Conley (:mconley) (:⚙️)]

Attachment: use the popup-scrollbars binding for <select> dropdowns in e10s windows. r=?

Comment 42

[Mike Conley (: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.

Comment 43

[Jim Mathies [:jimm]]

(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 44

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 45

[Allison Naaktgeboren :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.

Comment 46

[Allison Naaktgeboren :ally]

mconley has been struck with the mozplague

remote:   https://hg.mozilla.org/integration/fx-team/rev/dfef8b5c91e1

Comment 47

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 48

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP patch to add menulist-menupopup class (Windows only at the moment)

Comment 49

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 50

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 51

[Mike Conley (:mconley) (:⚙️)]

Since we're not doing the follow-up work in here, I'll remove the [leave-open].

Comment 52

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dfef8b5c91e1