869314 - [regression] drop-downs with background no longer display arrows

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[matthias koplenig [:metasieben]]

If there is any background specified drop-downs no longer show the arrow(s) to the right.

Could be related to the OSX-scrollbar(Bug 636564) changes.

Comment 1

[matthias koplenig [:metasieben]]

according to mozilla-central builds:

working: 70e0955ccc87
broken: b35170667a2f

Comment 2

[Loic]

Fresh regression:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=70e0955ccc87&tochange=b35170667a2f

Comment 4

[Boris Zbarsky [:bzbarsky]]

Stephen, nsComboboxControlFrame uses the scrollbar width to determine the width of the dropmarker.  I expect bug 636564 broke that somehow...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Oh, and the reason the background is needed is to drop the native theming.   You could just do "-moz-appearance:none" too.

Comment 6

[Stephen A Pohl [:spohl]]

Attachment: Patch

Overlay scrollbars are using negative margins, so we shouldn't add margins when we ask for desired scrollbar sizes.

Submitted to try to make sure that I didn't overlook something. I'll ask for a review once this is confirmed to be green:
https://tbpl.mozilla.org/?tree=Try&rev=0b06d53a46a0

Comment 7

[Stephen A Pohl [:spohl]]

Comment on attachment 750570 [details] [diff] [review]
Patch

This is starting to look green on all platforms, so asking for a review.

roc, this is similar to what I addressed in bug 636564 comment 209. Although the margins don't drop below 0 here, the negative margins used with overlay scrollbars will result in size.width and size.height to be 0 after adding margins. I believe it is reasonable to not add margins for overlay scrollbars here.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't think this patch is correct in general. The truth is that generally for disappearing scrollbars, the width allocated to scrollbars is zero.

The basic problem here (as I understand it) is that the layout of the combobox <select> has always depended on scrollbars having some width, and making the dropdown arrow have that width so it lines up above the scrollbar when the dropdown is opened and (in unconstrained-width situations) the scrollbar doesn't overlap any of the options in the dropdown. That just doesn't make sense with disappearing scrollbars.

If that analysis is correct, then one way to fix this bug would be to say that in combobox dropdowns only, we should allocate width for the vertical scrollbar so it doesn't overlap the options and use that width for the dropdown arrow. How does that sound?

Comment 10

[Stephen A Pohl [:spohl]]

Attachment: Patch

Trying to incorporate the feedback.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 750886 [details] [diff] [review]
Patch

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

::: layout/forms/nsComboboxControlFrame.cpp
@@ +741,5 @@
>    if (mListControlFrame) {
>      nsIScrollableFrame* scrollable = do_QueryFrame(mListControlFrame);
>      NS_ASSERTION(scrollable, "List must be a scrollable frame");
> +    if (isUsingOverlayScrollbars) {
> +      scrollbarWidth = scrollable->GetLegacyScrollbarWidth();

Let's call it GetNondisappearingScrollbarWidth.

@@ +747,3 @@
>      scrollbarWidth =
> +      scrollable->GetDesiredScrollbarSizes(presContext,
> +                                           aRenderingContext).LeftRight();

Fix indent.

::: layout/generic/nsGfxScrollFrame.h
@@ +486,5 @@
>      nsBoxLayoutState bls(aPresContext, aRC, 0);
>      return GetDesiredScrollbarSizes(&bls);
>    }
> +  virtual nscoord GetLegacyScrollbarWidth() MOZ_OVERRIDE {
> +    return nsPresContext::CSSPixelsToAppUnits(15);

We shouldn't be hardcoding this here! We need to call into theme code like we do to get the width of normal scrollbars!

Comment 12

[Stephen A Pohl [:spohl]]

Attachment: Patch

Addressed feedback.

Comment 13

[Stephen A Pohl [:spohl]]

Attachment: Patch

Improved nsGfxScrollFrameInner::GetNondisappearingScrollbarWidth.

Comment 14

[Stephen A Pohl [:spohl]]

Attachment: Patch

... and removed unused variable.

Comment 15

[Stephen A Pohl [:spohl]]

Try run is looking green:
https://tbpl.mozilla.org/?tree=Try&rev=143c4fdeb29e

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 751395 [details] [diff] [review]
Patch

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

::: layout/style/nsCSSKeywordList.h
@@ +568,5 @@
>  CSS_KEY(spinner, spinner)
>  CSS_KEY(spinner-upbutton, spinner_upbutton)
>  CSS_KEY(spinner-downbutton, spinner_downbutton)
>  CSS_KEY(spinner-textfield, spinner_textfield)
> +CSS_KEY(scrollbar-non-disappearing, scrollbar_non_disappearing)

I don't think we need this.

::: layout/style/nsCSSProps.cpp
@@ +563,5 @@
>    eCSSKeyword_scrollbartrack_horizontal,    NS_THEME_SCROLLBAR_TRACK_HORIZONTAL,
>    eCSSKeyword_scrollbartrack_vertical,      NS_THEME_SCROLLBAR_TRACK_VERTICAL,
>    eCSSKeyword_scrollbarthumb_horizontal,    NS_THEME_SCROLLBAR_THUMB_HORIZONTAL,
>    eCSSKeyword_scrollbarthumb_vertical,      NS_THEME_SCROLLBAR_THUMB_VERTICAL,
> +  eCSSKeyword_scrollbar_non_disappearing,   NS_THEME_SCROLLBAR_NON_DISAPPEARING,

or this

Comment 17

[Stephen A Pohl [:spohl]]

Attachment: Patch

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> Comment on attachment 751395 [details] [diff] [review]
> Patch
> 
> Review of attachment 751395 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/nsCSSKeywordList.h
> @@ +568,5 @@
> >  CSS_KEY(spinner, spinner)
> >  CSS_KEY(spinner-upbutton, spinner_upbutton)
> >  CSS_KEY(spinner-downbutton, spinner_downbutton)
> >  CSS_KEY(spinner-textfield, spinner_textfield)
> > +CSS_KEY(scrollbar-non-disappearing, scrollbar_non_disappearing)
> 
> I don't think we need this.

Ah, right! Removed both instances.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

BTW we should be able to come up with some kind of mochitest here that tests the width of a combobox is "wide enough".

Comment 19

[Stephen A Pohl [:spohl]]

Attachment: Test

I'm not very familiar with writing mochitests yet, but maybe this will do?

I confirmed that this test fails prior to the patch in this bug. Once applied, the test succeeds. It is also green on all platforms that don't support overlay scrollbars:
https://tbpl.mozilla.org/?tree=Try&rev=006ebc5472b4

Comment 20

[Žilvinas]

So it is green everywhere except the Mac where the problem occurred... ?

Comment 21

[Stephen A Pohl [:spohl]]

(In reply to Žilvinas from comment #20)
> So it is green everywhere except the Mac where the problem occurred... ?

That's right, until the fix (patch) in this bug is applied. Then, all platforms are green.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 751801 [details] [diff] [review]
Test

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

::: layout/forms/test/test_bug869314.html
@@ +29,5 @@
> +  <script type="application/javascript">
> +  // The selectbox has a width of 88px with the dropMarker of 15px,
> +  // 73px without.
> +  ok(document.getElementById("selectbox").clientWidth > 85,
> +     "Non-native styled combobox does not have enough space for a dropmarker!");

This test depends on the font used and other things that we don't want the test to depend on.

We can probably fix this by making all the options use the same text, and comparing the width of the <select> to the width of a regular div containing the longest option?

Comment 24

[Stephen A Pohl [:spohl]]

Attachment: Test

I couldn't use a div, since a div's width seems to default to the full width of the page. However, native styled comboboxes have the same width as non-native styled comboboxes if the dropmarker is missing. So maybe it's enough to test that non-native comboboxes have a greater width than a native one with the same content?

Comment 25

[Stephen A Pohl [:spohl]]

Forgot to mention that another reason for not using a div might be that it seems to use a different, larger font size for its content than a combobox.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/381492b19e3f
https://hg.mozilla.org/integration/mozilla-inbound/rev/619cdf937af1

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest failures on Windows XP, Android, and B2G.
https://hg.mozilla.org/integration/mozilla-inbound/rev/48b37e68c69f

https://tbpl.mozilla.org/php/getParsedLog.php?id=23203839&tree=Mozilla-Inbound

Comment 28

[Stephen A Pohl [:spohl]]

Attachment: Test

Made test Mac-specific (since this bug only applies to Mac).

Comment 29

[Stephen A Pohl [:spohl]]

And test is confirmed to pass on all platforms now:
https://tbpl.mozilla.org/?tree=Try&rev=53bc3facc417

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why did the test fail on non-Mac platforms?

Comment 31

[Stephen A Pohl [:spohl]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #30)
> Why did the test fail on non-Mac platforms?

I kicked off a try run with some debug info:
https://tbpl.mozilla.org/?tree=Try&rev=6244f3170817

Early results show that on non-Mac platforms, the width of native and non-native styled comboboxes have equal width. Note that the width actually includes the width for the dropmarker on those platforms. Mac seems to be the only platform that doesn't include the width of the dropmarker in native-styled comboboxes.

Comment 32

[Stephen A Pohl [:spohl]]

Did comment 31 answer your question in comment 30, or is there anything else I should investigate?

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 752806 [details] [diff] [review]
Test

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

I guess we'll just do this

Comment 34

[Josiah Bruner [:jsbruner]]

Could we land this now? It would be really nice to land before the next version.

Comment 35

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/abd5a7fec823
https://hg.mozilla.org/integration/mozilla-inbound/rev/1063a022f196

Comment 36

[Stephen A Pohl [:spohl]]

Backed out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fe06eab99d0

Relanded with proper r= info:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0167b5e0a389
https://hg.mozilla.org/integration/mozilla-inbound/rev/69f39ec4f7eb

Comment 37

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0167b5e0a389
https://hg.mozilla.org/mozilla-central/rev/69f39ec4f7eb

Comment 38

[Stephen A Pohl [:spohl]]

Attachment: Patch for aurora

This combined patch includes the patch and test that landed on mozilla-central as well as the crash fix from bug 877085. This is ready to land on aurora.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 636564
User impact if declined: Drop markers would be missing in non-native styled comboboxes when overlay scrollbars are used, making the comboboxes look like text fields.
Testing completed (on m-c, etc.): Has been on m-c for several days. I've also verified that comboboxes correctly display the drop markers now.
Risk to taking this patch (and alternatives if risky): minor
String or IDL/UUID changes made by this patch: none

Comment 39

[Stephen A Pohl [:spohl]]

Requesting checkin for aurora.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/567691eca26c