comboboxes can render labels over button with native form controls

RESOLVED FIXED

Status

()

Core
Layout: Form Controls
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Josh Aas, Assigned: Josh Aas)

Tracking

(Depends on: 1 bug)

unspecified
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite ?
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

11 years ago
Created attachment 266102 [details]
testcase 1

Say you have a combobox who's popup window will size to 300 pixels including a label and the scrollbar space. That will cause the *label part* (non-border) of the combobox button to size to 300 pixels.

Now, say you set the width of the combobox button to 150 and then select the longest option from the list. Currently, the combobox misrenders beacuse the option can render its text over the area that was added as extra area for the scrollbar, which with native widgets is used as space for the button that gets rendered on the right.
Flags: blocking1.9+
(Assignee)

Comment 1

11 years ago
Created attachment 266103 [details]
combobox bad rendering screenshot
Didn't you hack things intentionally to avoid making (or leaving room for) the frames associated with the dropmarker?  If so, the native theme code needs to return the right amount from GetWidgetBorder (or GetWidgetPadding) to ensure that the contents leave room for the dropmarker.
(Assignee)

Comment 3

11 years ago
Created attachment 266141 [details] [diff] [review]
fix v1.0
Attachment #266141 - Flags: review?(roc)
(Assignee)

Updated

11 years ago
Assignee: joshmoz → nobody
Component: Widget: Cocoa → Layout: Form Controls
QA Contact: cocoa → layout.form-controls
(Assignee)

Updated

11 years ago
Assignee: nobody → joshmoz

Updated

11 years ago
Blocks: 154632
+  PRBool themeUsesDropmarker = IsThemed() && presContext->GetTheme()->ThemeNeedsComboboxDropmarker();

Put the result of IsThemed into a local so you only have to call it once. Also, themeUsesDropmarker is a confusing name... I think it would be clearer if you created a variable "usingDropdownButtonFrame" which is !IsThemed() || ThemeNeedsComboboxMarker.

  if (NS_LIKELY(mDropdownFrame != nsnull)) {

This isn't so likely anymore so remove NS_LIKELY here.

   if (NS_LIKELY(mDropdownFrame != nsnull)) {
     result = mDropdownFrame->GetPrefWidth(aRenderingContext);
+    if (IsThemed() && !themeUsesDropmarker)
+      result -= scrollbarWidth;

I don't understand why you're doing this.
(Assignee)

Comment 5

11 years ago
>> +    if (IsThemed() && !themeUsesDropmarker)
>> +      result -= scrollbarWidth;
> I don't understand why you're doing this.

That is the whole point of the patch.

Recall that the dropmarker width is the same as the width of the scrollbar in mDropdownFrame. mDropdownFrame's pref width, used as a baseline here, includes space for a scrollbar, which is used in the baseline width as space for the dropmarker. If there shouldn't be a dropmarker, that space needs to be subtracted off.
(Assignee)

Comment 6

11 years ago
Created attachment 266437 [details] [diff] [review]
fix v1.1
Attachment #266141 - Attachment is obsolete: true
Attachment #266437 - Flags: review?(roc)
Attachment #266141 - Flags: review?(roc)
To make this clearer, I suggest that you structure the code like this:

  nscoord displayPrefWidth = 0;
  if (NS_LIKELY(mDisplayFrame)) {
     displayPrefWidth = ...;
  }
  if (mDropdownFrame) {
     nscoord dropdownContentWidth = ... - scrollbarWidth;
     displayPrefWidth = max(dropdownContentWidth, displayPrefWidth);
  }
  return result + (usingDropdownButtonFrame ? scrollbarWidth : 0);

Does that make sense? I think that makes things a lot clearer.

I wonder though, in what cases would the dropdown's preferred width (minus scrollbar) be less than the display frame's preferred width?
(Assignee)

Comment 8

11 years ago
Created attachment 266439 [details] [diff] [review]
fix v1.2
Attachment #266437 - Attachment is obsolete: true
Attachment #266439 - Flags: review?(roc)
Attachment #266437 - Flags: review?(roc)
Attachment #266439 - Flags: superreview+
Attachment #266439 - Flags: review?(roc)
Attachment #266439 - Flags: review+
(Assignee)

Comment 9

11 years ago
landed on trunk
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Comment on attachment 266439 [details] [diff] [review]
fix v1.2

I'd like Boris to have a look at this (as I said last week on IRC), even though it's already landed.
Attachment #266439 - Flags: review?(bzbarsky)
Comment on attachment 266439 [details] [diff] [review]
fix v1.2

Looks ok, I guess.
Attachment #266439 - Flags: review?(bzbarsky) → review+
Flags: in-litmus?
Looks like this patch was wrong.  Too bad I was trying to review it while doing too many other things (like being on vacation and sleeping 3 hours a night).  See bug 404288.
Depends on: 404288
https://litmus.mozilla.org/show_test.cgi?id=4532 was added to cover the Litmus test case.
Flags: in-litmus? → in-litmus+
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.