<select> combobox dropdown menu should anchor at the selected item on MacOSX

RESOLVED FIXED in Firefox 53

Status

()

Core
Layout: Form Controls
RESOLVED FIXED
9 years ago
2 months ago

People

(Reporter: gav, Assigned: Jared Beach)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {polish, pp, uiwanted})

unspecified
mozilla53
x86
Mac OS X
polish, pp, uiwanted
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

(URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_5_2; en-us) AppleWebKit/525.18 (KHTML, like Gecko) Version/3.1.1 Safari/525.18
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008042404 Minefield/3.0pre

Although the <select> html element looks consistent with the native UI, when you click on it the listing of <option> elements is very non-native.

Reproducible: Always

Steps to Reproduce:
1.  Open any page with a select dropdown box (example URL given).
2.  Click on the dropdown box.
Actual Results:  
When a <select> html element is clicked, the <option> elements are displayed in a custom, non-native way:  square box,  white background, black border on right and bottom, placed directly under the <select> box.

Expected Results:  
When a <select> html element is clicked, it should display <option> elements the native way.  In Leopard this is a rounded box, pearl coloured, hovered right above the <select> box.

All other form elements (text-boxes, checkbxes, etc) seem normal.

Updated

9 years ago
Summary: select dropdown appears inconsistent with OS native look and feel → <select> dropdown appears inconsistent with Mac OS X native look and feel

Updated

9 years ago
Status: UNCONFIRMED → NEW
Component: OS Integration → Layout: Form Controls
Ever confirmed: true
Keywords: polish, pp
Product: Firefox → Core
QA Contact: os.integration → layout.form-controls
I'm pretty sure we have a bug on this already.

And "OS integration" might have been write; I don't believe the native theming code to do the dropdown is in place.
Whiteboard: DUPEME
Er, might have been right.  As in, I don't think this is a form controls bug.
It's a little bit of both, the positioning (and height considerations)
is in layout.  I tried to find a bug on that but couldn't find one.
We have a bug on the appearance though: bug 402625, so let's make this
bug be about the position/height thing.
Summary: <select> dropdown appears inconsistent with Mac OS X native look and feel → <select> combobox dropdown menu should anchor at the selected item on MacOSX
Whiteboard: DUPEME
Enn, do you have any idea how to fix this?

Updated

8 years ago
Duplicate of this bug: 478439

Comment 6

8 years ago
I don't really know much about the html widgets no.
(Reporter)

Updated

8 years ago
Hardware: PowerPC → x86
Blocks: 402625
I've thought about this a little. Two things occured to me:
 1. Fixing XUL menulists seems a lot easier than fixing <select>s (in fact I've
    already got them mostly working).
 2. Fennec uses XUL popups for <select>s.

So if we took Fennec's approach to <select>s and fixed XUL menulists, we could basically get <select>s fixed for free, no?

Comment 8

8 years ago
(In reply to comment #7)
>  2. Fennec uses XUL popups for <select>s.

Are you sure? That doesn't sound like something that could be implemented easily.
Well, they're not XUL popups after all, just normal XUL elements (vbox, scrollbox, hbox). Here's the code that copies the list items from the <select> and puts them into Fennec's XUL:
http://mxr.mozilla.org/mobile-browser/source/chrome/content/browser-ui.js#1410

Maybe implementing such a thing wasn't easy, but to me it seems easier than teaching nsCSSFrameConstructor::InitializeSelectFrame how to play well with arrowscrollboxes.
OK. This is still slow, buggy and incomplete, but you get the idea:
https://build.mozilla.org/tryserver-builds/mstange@themasta.com-try-b323bf724600/try-b323bf724600-macosx.dmg
(Reporter)

Comment 11

8 years ago
Hi Markus,

I just tried the build you posted.  It seems to work well in most pages but behaves a little strange in some.

For example, the bugzilla search page works beautifully:
https://bugzilla.mozilla.org/query.cgi

But with this page the select box jumps to the top left corner of the screen:
http://www.w3schools.com/TAGS/tryit.asp?filename=tryhtml_option
Markus, that looks pretty good. A few notes after a quick testing, on 10.6.2:

1. select widgets nested in frames/iframes: the dropdown is anchored at the top-left of the window (noted in previous comment)
2. text-size: smaller than the OS default ? could be my eyes…
   (although Safari has the same font-size, I think)
3. doesn't respect minimum font-size setting
   (preferences > Content > fonts & colors - advanced)
   Safari does, to some extend
4. <option> is not styled according to the author stylesheet
  (testcase: http://dev.l-c-n.com/form-controls/select-styled_a.html)
5. no indication of <optgroup>: optgroup label has disappeared,
   all options are displayed as one group (same test case, top left)

In case of (4), I don't object, mind you :-). I find author styled form controls ugly beyond words.

Comment 13

8 years ago
(In reply to comment #9)
> Maybe implementing such a thing wasn't easy, but to me it seems easier than
> teaching nsCSSFrameConstructor::InitializeSelectFrame how to play well with
> arrowscrollboxes.

The number of regressions it would cause though isn't worth it just to fix this bug.
(In reply to comment #13)
> The number of regressions it would cause though isn't worth it just to fix this
> bug.

What regressions do you have in mind? Are you talking about loss of author-control over the styling of the popup?
In my opinion fixing this bug is important enough to accept some regressions in that area.

Comment 15

8 years ago
Maybe I misunderstood, but your description implies above that you want to reimplement the select element using different XUL popup elements. Large changes like that will cause regressions.

It would be fine to do that work to remove a lot of the specialized code in the <select> frame classes and move into, say, xbl2, but that shouldn't be this bug.

> In my opinion fixing this bug is important enough to accept some regressions in
> that area.

Not on Windows or Linux though.

A quick glance implies that this bug could be much more easily be fixed by just adjusting the vertical position in nsComboboxControlFrame::AbsolutelyPositionDropDown.
(In reply to comment #15)
> Maybe I misunderstood, but your description implies above that you want to
> reimplement the select element using different XUL popup elements.

This is what I want to do:
 - Have a popupshowing listener on the tabbrowser element in browser.xul.
 - In this handler, if we're on Mac and event.target is a HTMLSelectElement,
   preventDefault the event so that the traditional <select> popup doesn't
   open. (When <select>s are opened, they fire a popupshowing event. The code
   that does this doesn't respect preventDefault yet, but that's easy to
   change).
 - Iterate over all the options / optgroups in the select and add them to
   a menupopup, using code that's very similar to the Fennec code
http://mxr.mozilla.org/mobile-browser/source/chrome/content/browser-ui.js#1410
 - Maybe copy some of the computed font style over, so that we don't discard
   all of the author's styling.
 - Open and anchor the menupopup in the same way that a XUL <menulist> would.
 - When a menuitem is clicked, update the <select>'s selectedIndex and fire a
   change event.

> Large changes like that will cause regressions.

Ah, so you're just mentioning the general regression risk that one always has to anticipate. OK.

> > In my opinion fixing this bug is important enough to accept some regressions in
> > that area.
> 
> Not on Windows or Linux though.

Sure. On Windows we wouldn't cancel the popupshowing event and we wouldn't open our own menupopup, so the traditional <select> dropdown would open, and the traditional code paths wouldn't be impacted.

> A quick glance implies that this bug could be much more easily be fixed by just
> adjusting the vertical position in
> nsComboboxControlFrame::AbsolutelyPositionDropDown.

Well, the idea is to make these dropdowns look completely like native ones eventually. Making them anchor at the selected item is just one step on the way there. There are lots of other things that need to be fixed for the full native look / feel: The scrollbar needs to go and be replaced by arrowbuttons at the top and bottom, clicked menu items should blink, the list should fade when it's rolled up, it needs rounded corners and the correct shadow, a checkmark in front of selected item, different alignment etc.

Fixing all these things in nsComboboxControlFrame and friends would duplicate the work we're doing to fix them in XUL menus.
(In reply to comment #16)
> > Not on Windows or Linux though.
> 
> Sure. On Windows we wouldn't cancel the popupshowing event

... and probably on Linux too, at least in the beginning.
Bug 468734 suggests that native Linux behaviour is pretty Mac-like here.
Note that I'm happy to help in whatever way you like with the frame construction end of things if it would be useful (starting with cleaning it up to be sane, if you'd like).

I'm not entirely convinced that moving this basic HTML UI effectively out of Gecko and into something every single Gecko consumer would have to reimplement is desirable.  It's an ok solution for Firefox, but not an acceptable solution for Core.  In my humble form controls wish-I-weren't-owner role, that is...

I don't have a priori objections to using a XUL something to implement the combobox dropdown, though the approach described sounds like a performance nightmare for long select boxes to me, at first blush.  Have you tested it on a combobox with 10-20 thousand options?  How long does opening such a combobox take?  How long does it take on Fennec?
(In reply to comment #18)
> I'm not entirely convinced that moving this basic HTML UI effectively out of
> Gecko and into something every single Gecko consumer would have to reimplement
> is desirable.  It's an ok solution for Firefox, but not an acceptable solution
> for Core.  In my humble form controls wish-I-weren't-owner role, that is...

I agree that that's far from optimal. But at the moment I don't see a different solution that avoids implementing everything twice.
And it's not like we're breaking anything for non-Firefox users of Gecko... we're just not fixing this bug for them. The old dropdown continues to work.

> I don't have a priori objections to using a XUL something to implement the
> combobox dropdown, though the approach described sounds like a performance
> nightmare for long select boxes to me, at first blush.  Have you tested it on a
> combobox with 10-20 thousand options?  How long does opening such a combobox
> take?  How long does it take on Fennec?

20000 options take about 12 seconds in a Fennec nightly on my Mac. In my Firefox build it takes a lot longer because it's a debug build...
So yeah, performance is really bad. But I don't see anything wrong with falling back to the old dropdown for long lists. Long lists are much easier to navigate using a scrollbar anyway.
I'd want ui-review on that sort of inconsistent UI...

And generally speaking, I'd prefer a core fix for core bugs.  If we think it's important enough to fix for Firefox, then it's important enough to fix for non-Firefox consumers.

What are the impediments to implementing things in core here?
GTK also wants something much more like a menu than the current dropdown.

I would be in favour of offering an alternative (native) implementation of the combobox dropdown that uses an nsMenuPopupFrmae instead of an nsListBoxFrame.
Or better still, maybe we could figure out how to style a XUL menu on Windows so that it looks like the current dropdown, e.g. hiding the up/down arrows and showing a scrollbar instead.
Fwiw, at the UI level, Opera 10.5 pre alpha for OS X now has an OS X style dropdown that closely matches what Camino 2.0/2.1a1pre currently does and what the tryserver build in comment 10 does. Quite nice.

Updated

7 years ago
Duplicate of this bug: 541634

Updated

6 years ago
Keywords: uiwanted

Comment 25

5 years ago
I'm noodling around with this and bug 402625.  If someone who knows what they're doing is actively working on it, please feel free to take it.
Assignee: nobody → adw
Status: NEW → ASSIGNED
Any progress? :-)

Comment 27

5 years ago
I side with Steven Michaud and ask: Any progress in this issue? *nudge* :-)

Comment 28

4 years ago
Looking at Comment #25 and this recent comment here: https://bugzilla.mozilla.org/show_bug.cgi?id=402625#c27

Any progress? Anybody there who is able to finally fix that bug after 6 years of being filed and non-fixed?

Updated

4 years ago
Assignee: adw → nobody
Status: ASSIGNED → NEW

Comment 29

4 years ago
Shouldn't this Bug be DUPEd to Bug 402625? I guess there's not much of extra work that should be separated from making SELECT look native to justify this "extra" bug.

Updated

11 months ago
Duplicate of this bug: 1173111

Updated

7 months ago
Duplicate of this bug: 1309941
Blocks: 1091592
Assigning to Jared as he's working on this as part of a student project.
Assignee: nobody → beachjar
Status: NEW → ASSIGNED

Comment 33

6 months ago
For e10s this is pretty much already implemented now that 52106 is done.

Three changes would be needed.

1. Change 'after_start' to 'selection' in the call to popup open call for Mac only.
2. If needed, tweak the margins to make the popup align horizontally.
3. Large popups which scroll/get cropped won't work right. Some adjustments will need to be made for this.

I'm assuming due to 1300784 that we won't worry about non-e10s.
(In reply to Neil Deakin from comment #33)
> For e10s this is pretty much already implemented now that 52106 is done.
> Three changes would be needed.
> 
> 1. Change 'after_start' to 'selection' in the call to popup open call for
> Mac only.
> 2. If needed, tweak the margins to make the popup align horizontally.
> 3. Large popups which scroll/get cropped won't work right. Some adjustments
> will need to be made for this.

That is awesome! Thank you for mentioning this.
 
> I'm assuming due to 1300784 that we won't worry about non-e10s.

Yes, that is right. We should only focus on the e10s implementation here.
Comment hidden (mozreview-request)

Comment 36

6 months ago
mozreview-review
Comment on attachment 8807242 [details]
Bug 430745 - Select dropdown popups will anchor on a selected element for Mac

https://reviewboard.mozilla.org/r/90470/#review90246

Please make sure that browser_selectpopup.js still passes with these changes. You can run that test by running:
`mach test browser/base/content/test/general/browser_selectpopup.js`

::: toolkit/modules/SelectParentHelper.jsm:11
(Diff revision 1)
> +const {classes: Cc, utils: Cu, interfaces: Ci} = Components;
> +const { devtools } = Cu.import("resource://devtools/shared/Loader.jsm", {});

These two are unused and should be removed.

::: toolkit/modules/SelectParentHelper.jsm:64
(Diff revision 1)
>      let constraintRect = browser.getBoundingClientRect();
>      constraintRect = new win.DOMRect(constraintRect.left + win.mozInnerScreenX,
>                                       constraintRect.top + win.mozInnerScreenY,
>                                       constraintRect.width, constraintRect.height);
>      menupopup.setConstraintRect(constraintRect);
> -    menupopup.openPopupAtScreenRect("after_start", rect.left, rect.top, rect.width, rect.height, false, false);
> +    menupopup.openPopupAtScreenRect((AppConstants.platform == "macosx") ? "selection" : "after_start", rect.left, rect.top, rect.width, rect.height, false, false);

The parentheses around `AppConstants.platform == "macosx"` are not needed and can be removed.
Attachment #8807242 - Flags: review?(jaws) → review-
Attachment #8807242 - Flags: review?(mconley)

Comment 37

6 months ago
mozreview-review
Comment on attachment 8807242 [details]
Bug 430745 - Select dropdown popups will anchor on a selected element for Mac

https://reviewboard.mozilla.org/r/90470/#review90248

::: toolkit/modules/SelectParentHelper.jsm:13
(Diff revision 1)
>    "SelectParentHelper"
>  ];
>  
> +const {classes: Cc, utils: Cu, interfaces: Ci} = Components;
> +const { devtools } = Cu.import("resource://devtools/shared/Loader.jsm", {});
> +const { AppConstants } = devtools.require("resource://gre/modules/AppConstants.jsm");

You should load this with `Cu.import(..., {})` directly so you can avoid the DevTools loader.
Thanks jryans, you have better eyes than me! I didn't notice AppConstants was being loaded with the devtools loader. Yeah, Cu.import directly is the way to go here.
Comment hidden (mozreview-request)

Comment 40

6 months ago
mozreview-review
Comment on attachment 8808355 [details]
Bug 430745 - Select dropdown popups will anchor on a selected element for Mac

https://reviewboard.mozilla.org/r/91176/#review90988

Looks good. These two patches should get merged. It looks like this one is built on top of the first. Might be easiest to just `hg strip` these two changesets from your tree and recreate it from scratch since the patch is so small. Once you do that, then you can repush this to mozreview and we can land from there.
Attachment #8808355 - Flags: review?(jaws) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

6 months ago
Attachment #8807242 - Attachment is obsolete: true
Attachment #8808355 - Flags: review?(mconley)

Comment 42

6 months ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d67c6dcba478
Select dropdown popups will anchor on a selected element for Mac r=jaws

Comment 43

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d67c6dcba478
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Depends on: 1316991
There's a good chance this patch caused bug 1316991.
Depends on: 1316597

Comment 45

5 months ago
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/mozilla-central/rev/feddafb5cb54
Backed out changeset d67c6dcba478 on request from jaws. r=backout
also backed out from aurora in https://hg.mozilla.org/releases/mozilla-aurora/rev/8cdff71808a0e11361bca7f218658c4343c730ca on request from jaws
Status: RESOLVED → REOPENED
status-firefox52: fixed → affected
Flags: needinfo?(jaws)
Resolution: FIXED → ---
This was backed out due to bug 1316597. We'll have to get bug 1316597 fixed before we can re-land this patch.
status-firefox52: affected → ---
Flags: needinfo?(jaws)
Target Milestone: mozilla52 → ---
I'm going to request re-landing now that bug 1316597 is set to autoland.
Comment hidden (mozreview-request)

Comment 50

5 months ago
mozreview-review
Comment on attachment 8813247 [details]
Bug 430745 - Select dropdown popups will anchor on a selected element for Mac

https://reviewboard.mozilla.org/r/94722/#review94936

r=me, rebased beachjar's previous patch.
Attachment #8813247 - Flags: review?(jaws) → review+

Comment 51

5 months ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/875479c06e72
Select dropdown popups will anchor on a selected element for Mac r=jaws
Attachment #8813247 - Flags: review?(mconley)

Comment 52

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/875479c06e72
Status: REOPENED → RESOLVED
Last Resolved: 6 months ago5 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
This doesn't look fixed to me.  Simple testcase:

  data:text/html,<select><option>a<option selected>b

Am I missing something?

It's also not fixed in non-e10s for the even simpler testcase:

  data:text/html,<select><option>a<option>b
Flags: needinfo?(jaws)
Flags: needinfo?(beachjar)
I guess the non-e10s case is handled by bug 1300784, but the problem with the basic "not the first option is selected" case is still a problem...

Comment 55

5 months ago
The popup is constrained to not opening outside the content area to prevent spoofing (which would be more relevant to e10s if we ever fix 910022). If it would appear over chrome UI, it instead appears aligned along the popup edge, much like it would have done before this change.
I believe Neil answered the needinfo in comment #55. I don't have anything to add here and I don't think beachjar knows either.
Flags: needinfo?(jaws)
Flags: needinfo?(beachjar)
Comment hidden (mozreview-request)
Attachment #8808355 - Flags: review?(mconley)
Depends on: 1342398
You need to log in before you can comment on or make changes to this bug.