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

RESOLVED FIXED in mozilla37

Status

()

Core
Layout: Form Controls
P2
normal
RESOLVED FIXED
3 years ago
10 months ago

People

(Reporter: mconley, Assigned: ally)

Tracking

(Blocks: 1 bug)

unspecified
mozilla37
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10sm4+)

Details

Attachments

(8 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Created attachment 8473205 [details]
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.
(Reporter)

Updated

3 years ago
OS: Windows 7 → All
(Reporter)

Updated

3 years ago
Blocks: 1049285
(Reporter)

Updated

3 years ago
Blocks: 653064
(Reporter)

Comment 1

3 years ago
Created attachment 8473220 [details]
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.
(Reporter)

Comment 2

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

Updated

3 years ago
Duplicate of this bug: 1002489

Updated

3 years ago
tracking-e10s: ? → +
(Reporter)

Updated

3 years ago
Duplicate of this bug: 1049285

Comment 5

3 years ago
Created attachment 8473719 [details]
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

3 years ago
Created attachment 8473720 [details]
forms test cases
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.
(Reporter)

Updated

3 years ago
Duplicate of this bug: 1012212

Comment 9

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

Updated

3 years ago
Depends on: 897060
(Reporter)

Comment 10

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

Comment 11

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

Comment 12

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

Updated

3 years ago
Assignee: nobody → jmathies
Blocks: 997462
tracking-e10s: ? → +
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.)

Comment 14

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

Updated

3 years ago
Blocks: 1010732
Priority: P3 → P2

Updated

3 years ago
Status: NEW → ASSIGNED

Updated

3 years ago
Blocks: 1058881
Moving old M2 P2 bugs to M4.
Move old M2's low-priority bugs to M6 milestone.
tracking-e10s: + → m6+
oops: old M2 P2 bugs should have been moved to new M4 milestone, not M6.
tracking-e10s: m6+ → m4+

Updated

3 years ago
Blocks: 1069463

Updated

3 years ago
Duplicate of this bug: 973534
(Reporter)

Updated

3 years ago
Duplicate of this bug: 1066773

Updated

3 years ago
Blocks: 1069355

Updated

3 years ago
Duplicate of this bug: 1069596

Updated

3 years ago
Status: ASSIGNED → NEW

Comment 21

3 years ago

Comment 22

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

Comment 23

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

3 years ago
Created attachment 8503339 [details]
listbox wip

Comment 25

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

Updated

3 years ago
Blocks: 1082510
> (You will likely need to change nsListBoxFrame.cpp to do this).

I meant nsListControlFrame.cpp of course.
See Also: → bug 1091592

Updated

3 years ago
Duplicate of this bug: 1096272

Comment 29

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

3 years ago
Created attachment 8523988 [details] [diff] [review]
1053981-select

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

Updated

3 years ago
Attachment #8523988 - Attachment is patch: true

Comment 31

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

Updated

3 years ago
Assignee: jmathies → ally

Updated

3 years ago
Duplicate of this bug: 1100072
Any chance to get bug 1091592 fixed as part of this?
Our select dropdowns have been ugly for too long.
Flags: needinfo?(ally)
(Assignee)

Comment 34

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

Updated

3 years ago
Duplicate of this bug: 1100735

Updated

3 years ago
Duplicate of this bug: 1101519
(Reporter)

Updated

3 years ago
Blocks: 1090602
Blocks: 1068626
(Reporter)

Comment 37

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

Comment 38

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

Updated

3 years ago
Duplicate of this bug: 1105294

Updated

3 years ago
Blocks: 1103635

Comment 40

3 years ago
Created attachment 8531251 [details]
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
Flags: needinfo?(mconley)
(Reporter)

Comment 41

3 years ago
Created attachment 8532643 [details] [diff] [review]
use the popup-scrollbars binding for <select> dropdowns in e10s windows. r=?
(Reporter)

Comment 42

3 years ago
@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)

Comment 43

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

Comment 44

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

Comment 45

3 years ago
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+
(Assignee)

Comment 46

3 years ago
mconley has been struck with the mozplague

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

Comment 47

3 years ago
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]
(Reporter)

Comment 48

3 years ago
Created attachment 8533304 [details] [diff] [review]
WIP patch to add menulist-menupopup class (Windows only at the moment)
(Reporter)

Comment 49

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

Comment 50

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

Comment 51

3 years ago
Since we're not doing the follow-up work in here, I'll remove the [leave-open].
Whiteboard: [leave-open]
(Assignee)

Updated

3 years ago
Flags: needinfo?(ally)
https://hg.mozilla.org/mozilla-central/rev/dfef8b5c91e1
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Depends on: 1108761

Updated

3 years ago
Blocks: 1108761
No longer depends on: 1108761

Updated

3 years ago
Depends on: 1112248
Depends on: 1112243
No longer depends on: 1112248

Updated

3 years ago
Depends on: 1112381

Updated

3 years ago
Blocks: 1112646

Updated

2 years ago
See Also: → bug 1197913
You need to log in before you can comment on or make changes to this bug.