Closed Bug 430745 Opened 16 years ago Closed 8 years ago

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

Categories

(Core :: Layout: Form Controls, defect)

Product:
Core
Component:
Layout: Form Controls
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: gautam.newalkar, Assigned: beachjar)

References

(Depends on 1 open bug,
URL
)

Details

(Keywords: platform-parity, polish, uiwanted)

Attachments

(2 files, 1 obsolete file)

Bug 430745 - Select dropdown popups will anchor on a selected element for Mac
58 bytes, text/x-review-board-request
jaws
: review+
Details
Bug 430745 - Select dropdown popups will anchor on a selected element for Mac
58 bytes, text/x-review-board-request
jaws
: review+
Details
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.
Summary: select dropdown appears inconsistent with OS native look and feel → <select> dropdown appears inconsistent with Mac OS X native look and feel
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?
I don't really know much about the html widgets no.
Hardware: PowerPC → x86
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?
(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.
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.
(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.
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.
Keywords: uiwanted
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? :-)
I side with Steven Michaud and ask: Any progress in this issue? *nudge* :-)
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?
Assignee: adw → nobody
Status: ASSIGNED → NEW
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.
Assigning to Jared as he's working on this as part of a student project.
Assignee: nobody → beachjar
Status: NEW → ASSIGNED
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 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-
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 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+
Attachment #8807242 - Attachment is obsolete: true
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
https://hg.mozilla.org/mozilla-central/rev/d67c6dcba478
Status: ASSIGNED → RESOLVED
Closed: 8 years 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
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 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+
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)
https://hg.mozilla.org/mozilla-central/rev/875479c06e72
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years 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...
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)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: