674443 - Make select element dropdown menu look more native

Status: RESOLVED WORKSFORME

(Core :: Layout: Form Controls, defect)

Description

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Basically, this patch tries to make the select element dropdown menu looks like any other menu in the UI. That's how it should look like on MacOS X, GTK and Windows Vista/7. I'm not sure Windows XP is expecting that but I do not have a Windows XP VM around.

I marked this bug as blocking bug 402625 because it seems that MacOS X dropdown menus do not look native with this patch. I will use bug 402625 to fix that for MacOS X.

I will need people to confirm that it looks correct on other GTK theme than mine and on Windows XP/Vista/7 (if you are in CC and you wonder why, that might be the reason ;))

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

A part of another part came with the previously attached one...

Comment 2

[Joe Drew (not getting mail)]

Can you attach before/after screenshots?

Comment 3

[Mounir Lamouri (:mounir)]

(In reply to comment #2)
> Can you attach before/after screenshots?

I will but I've just seen an unexpected side effect on the select element height which gets even worse given that the text doesn't re-align.

Comment 4

[Mounir Lamouri (:mounir)]

CCing bz in case of he has an idea of what causes the behavior described in comment 3.

Comment 5

[Mounir Lamouri (:mounir)]

Attachment: Screenshot: before

Comment 6

[Mounir Lamouri (:mounir)]

Attachment: Screenshot: after

Comment 7

[Mounir Lamouri (:mounir)]

Unfortunately, on Mac, instead of just not being native, the dropdown menu is quite broken :(

Hope stuff don't get worse on Windows :)

Comment 8

[Boris Zbarsky [:bzbarsky]]

You mean the height of the actual combobox increases?

That's quite purposeful.  See nsComboboxDisplayFrame::Reflow and the comment at the start of nsComboboxControlFrame::Reflow.  Basically, we want to be able to show any option in the display area without the options being cut off and without having to resize the combobox when the selected option changes.  So we size the display area to the size of the options.  Your patch is changing the size of the options, so changes the size of the display area....

That said, the current code is assuming that the height of options is due solely to the text in them.  With this change, I bet that's no longer true due to them ending up with nonzero padding or something.  So it would makes sense to account for that when computing the display area height.

Comment 9

[Boris Zbarsky [:bzbarsky]]

One possible option is to stop worrying about the actual option text when computing the combobox height and just use the font-size of the combobox....

Comment 10

[henryfhchan]

can this be fixed? this minor bug is extremely annoying compared to the whole design of firefox

Comment 11

[Sierk Bornemann]

(In reply to henry.fai.hang.chan from comment #10)
> can this be fixed? this minor bug is extremely annoying compared to the
> whole design of firefox

+1

Comment 12

[Sierk Bornemann]

Any progress in this issue? :)

Comment 13

[Mounir Lamouri (:mounir)]

Attachment: Patch with GTK support only

This patch makes the dropdown look native for GTK. I had to modify some widget code so I will likely have to modify other widgets to enable the feature on other platforms.

Comment 14

[Mounir Lamouri (:mounir)]

Attachment: Part 3 - Fix Cocoa backend

Comment 15

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - Update forms.css

Comment 16

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - Fix GTK backend

Comment 17

[Mounir Lamouri (:mounir)]

Attachment: Part 4 - Fix Windows backend

Comment 18

[Markus Stange [:mstange]]

Comment on attachment 791358 [details] [diff] [review]
Part 3 - Fix Cocoa backend

You can move nsNativeThemeWin::IsMenuActive to xpwidgets/nsNativeTheme, make your changes there, and use that from the Cocoa backend instead.

Comment 19

[Karl Tomlinson (:karlt)]

Comment on attachment 791363 [details] [diff] [review]
Part 2 - Fix GTK backend

>+        // We want the item to look like hovered if it is checked too so the
>+        // user can see the currently selected option easily.
>+        aState->inHover |= GetContentState(aFrame, aWidgetType).
>+                             HasState(NS_EVENT_STATE_CHECKED);

Is there any time when NS_EVENT_STATE_HOVER will be set here but not NS_EVENT_STATE_CHECKED?
Having the highlight state set for both flags suggests there might be 2 options highlighted at one time.
I assume that won't happen?
Would replacing "|=" with "=" here behave as intended?

I assume you only need my review of the widget/gtk2 changes.

Comment 20

[Jim Mathies [:jimm]]

Comment on attachment 791367 [details] [diff] [review]
Part 4 - Fix Windows backend

Review of attachment 791367 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsNativeThemeWin.cpp
@@ +1420,5 @@
>  
> +        if (isHover ||
> +            (aFrame->GetContent()->IsHTML() &&
> +             eventState.HasAtLeastOneOfStates(NS_EVENT_STATE_HOVER |
> +                                              NS_EVENT_STATE_CHECKED))) {

Can you break this out into a helper similar to IsMenuActive for html elements?

Comment 21

[Sebastian Zartner [:sebo]]

The last patch for this lies three years back. Mounir, do you still intend to fix this or should we unassign the bug for now?

Sebastian

Comment 22

[Mounir Lamouri (:mounir)]

Fair to say I will not fix this :)

Comment 23

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

We probably did a bunch of this when we switched to e10s selects.

Comment 24

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 12 votes.
:emilio, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 25

[Emilio Cobos Álvarez (:emilio)]

(In reply to David Baron :dbaron: from comment #23)

We probably did a bunch of this when we switched to e10s selects.

Indeed