Closed Bug 869314 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Layout: Form Controls, defect, major)

23 Branch
x86
macOS
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla24
Tracking Status
firefox21 --- unaffected
firefox22 --- unaffected
firefox23 + fixed
firefox24 --- fixed
firefox-esr17 --- unaffected

People

(Reporter: metasieben, Assigned: spohl)

References

()

Details

(Keywords: regression, Whiteboard: [lion-scrollbars+])

Attachments

(3 files, 7 obsolete files)

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.
according to mozilla-central builds:

working: 70e0955ccc87
broken: b35170667a2f
Duplicate of this bug: 870050
Status: UNCONFIRMED → NEW
Ever confirmed: true
Stephen, nsComboboxControlFrame uses the scrollbar width to determine the width of the dropmarker.  I expect bug 636564 broke that somehow...
Assignee: nobody → spohl.mozilla.bugs
Component: General → Layout: Form Controls
Product: Firefox → Core
Oh, and the reason the background is needed is to drop the native theming.   You could just do "-moz-appearance:none" too.
Severity: normal → major
Attached patch Patch (obsolete) — Splinter Review
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 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.
Attachment #750570 - Flags: review?(roc)
Duplicate of this bug: 872674
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?
Attached patch Patch (obsolete) — Splinter Review
Trying to incorporate the feedback.
Attachment #750570 - Attachment is obsolete: true
Attachment #750570 - Flags: review?(roc)
Attachment #750886 - Flags: review?(roc)
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!
Attached patch Patch (obsolete) — Splinter Review
Addressed feedback.
Attachment #750886 - Attachment is obsolete: true
Attachment #750886 - Flags: review?(roc)
Attachment #751131 - Flags: review?(roc)
Attached patch Patch (obsolete) — Splinter Review
Improved nsGfxScrollFrameInner::GetNondisappearingScrollbarWidth.
Attachment #751131 - Attachment is obsolete: true
Attachment #751131 - Flags: review?(roc)
Attachment #751391 - Flags: review?(roc)
Attached patch Patch (obsolete) — Splinter Review
... and removed unused variable.
Attachment #751391 - Attachment is obsolete: true
Attachment #751391 - Flags: review?(roc)
Attachment #751395 - Flags: review?(roc)
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
Attached patch PatchSplinter Review
(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.
Attachment #751395 - Attachment is obsolete: true
Attachment #751395 - Flags: review?(roc)
Attachment #751479 - Flags: review?(roc)
BTW we should be able to come up with some kind of mochitest here that tests the width of a combobox is "wide enough".
Attached patch Test (obsolete) — Splinter Review
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
Attachment #751801 - Flags: review?(roc)
So it is green everywhere except the Mac where the problem occurred... ?
(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.
Duplicate of this bug: 868420
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?
Attached patch Test (obsolete) — Splinter Review
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?
Attachment #751801 - Attachment is obsolete: true
Attachment #751801 - Flags: review?(roc)
Attachment #752031 - Flags: review?(roc)
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.
Keywords: checkin-needed
Attached patch TestSplinter Review
Made test Mac-specific (since this bug only applies to Mac).
Attachment #752031 - Attachment is obsolete: true
Attachment #752806 - Flags: review?(roc)
And test is confirmed to pass on all platforms now:
https://tbpl.mozilla.org/?tree=Try&rev=53bc3facc417
Why did the test fail on non-Mac platforms?
(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.
Did comment 31 answer your question in comment 30, or is there anything else I should investigate?
Flags: needinfo?(roc)
Comment on attachment 752806 [details] [diff] [review]
Test

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

I guess we'll just do this
Attachment #752806 - Flags: review?(roc) → review+
Flags: needinfo?(roc)
Could we land this now? It would be really nice to land before the next version.
https://hg.mozilla.org/mozilla-central/rev/0167b5e0a389
https://hg.mozilla.org/mozilla-central/rev/69f39ec4f7eb
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Depends on: 877085
Attached patch Patch for auroraSplinter Review
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
Attachment #758065 - Flags: approval-mozilla-aurora?
Attachment #758065 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Requesting checkin for aurora.
Keywords: checkin-needed
Blocks: 940798
You need to log in before you can comment on or make changes to this bug.