Closed Bug 365243 Opened 18 years ago Closed 17 years ago

use small native scrollbars for HTML form elements

Categories

(Core :: Widget: Cocoa, defect)

PowerPC
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jaas, Assigned: jaas)

References

Details

Attachments

(4 files, 3 obsolete files)

We should use small native scrollbars for HTML form elements. That would be more efficient space-wise and it'd look better.
Attached patch fix v0.2 (obsolete) — Splinter Review
This works for Firefox only. It isn't the right way to do it, but it is a start.
Screenshots of v0.2 patched vs. unpatched.
Attachment #249866 - Attachment is patch: false
Attachment #249866 - Attachment mime type: text/plain → image/jpeg
Attached patch fix v1.0 (obsolete) — Splinter Review
This should work for all products, including Camino.
Attachment #249865 - Attachment is obsolete: true
Attachment #249880 - Flags: review?
Attachment #249880 - Flags: review? → review?(stuart.morgan)
Comment on attachment 249880 [details] [diff] [review]
fix v1.0

Sexy.
r=smorgan.
Attachment #249880 - Flags: review?(stuart.morgan) → review+
Attachment #249880 - Flags: superreview?(dbaron)
+  if ((SInt32)frameRect.size.width == smallScrollbarWidth ||
+      (SInt32)frameRect.size.height == smallScrollbarWidth) {
+    [self setControlSize:NSSmallControlSize];
+  }

I want to add an else clause to that if statement that sets the scrollbar to normal size if neither dimension matches the small scrollbar width. That will fix the unlikely case in which we incorrectly make the native scrollbar small at some point before its final size is set.

The reason we can get away with setting small size like this is that even if we have a large vertical scrollbar with a height of 16, that isn't a reasonable size so it doesn't matter if we incorrectly set the native scrollbar to small.
It is sexy :-)
One problem I see with patch rv.1.0: The buttons on select widgets (dropdown) are squeezed to the width of the scrollbar. In Minefield, that makes for a odd looking button; in Camino, it results in the text overlapping the blue 'button'.
Camino (on top) and Minefield.
Attached patch fix v1.1 (obsolete) — Splinter Review
Adds the fix I mentioned before and restricts the small scrollbars to listboxes, popup menus are not affected now.
Attachment #249880 - Attachment is obsolete: true
Attachment #249933 - Flags: superreview?(dbaron)
Attachment #249880 - Flags: superreview?(dbaron)
Josh, with patch 1.1, it looks like select multiples lose their borders.  Screenshot with today's 1.8branch nightly on top, a today's trunk+patch v1.1 in middle, and yesterday's trunk nightly on the bottom.

Hyatt's form controls testcase: https://bugzilla.mozilla.org/attachment.cgi?id=99911&action=view
Comment on attachment 249933 [details] [diff] [review]
fix v1.1

>+html|select[size],
>+html|select[multiple],
>+html|select[size][multiple] > scrollbar {
Shouldn't this say:
html|select[size] > scrollbar,
html|select[multiple] > scrollbar {
(<select multiple size=4> will match both rules of course)

Do you need to make any changes to the @media print block?
1/
>+html|select[size],
>+html|select[multiple],
>+html|select[size][multiple] > scrollbar

surely you mean
html|select[size] > scrollbar,
html|select[multiple] > scrollbar,
html|select[size][multiple] > scrollbar {

What you select is: a select widget with the attribute [size], a select widget with the attribute [multiple] and a scrollbar that is the child of a select with the attributes [size] and [multiple].
That explains the display problems mentioned by Smokey.

2/
html|select[size] > scrollbar will also select some pop ups: <select size="0"> and <select size="1">.

html|select[size]:not([size="0"]):not([size="1"]) would work on trunk.

Not sure about Branch 1.8 (for porting to Camino 1.2 eventually). I remember having problems with that selector, an obscure parsing bug when there is a namespace declaration in the stylesheet.
The browser ending up reading the selector as 
html|*select[size]:not([size="0"]):not([size="1"])
note the * before 'select'.

With both those changed on my side, that works fine on my Camino build here. Not yet tested with Minefield.
> Not sure about Branch 1.8 (for porting to Camino 1.2 eventually). 

There's no hope of this landing on the 1.8 branch, unfortunately :(  (Besides touching shared Core files, it depends on files that don't exist on the branch....)
Attachment #249951 - Attachment is patch: false
Attachment #249951 - Attachment mime type: text/plain → image/png
It seems that the text in the popup-style selects is also shifted up by 1px with the patch.
Comment on attachment 249933 [details] [diff] [review]
fix v1.1

I'll post an updated patch soon.
Attachment #249933 - Flags: superreview?(dbaron)
I used the extra CSS selector html|select[size][multiple] because that is done in layout/style/forms.css:

158 select[size],
159 select[multiple],
160 select[size][multiple] {

If it is true that it is unnecessary, then we might want to fix that in forms.css as well.
Attached patch fix v1.1.1Splinter Review
Fixes stuff mentioned in comments. Thanks for all the help!
Attachment #249933 - Attachment is obsolete: true
Attachment #250194 - Flags: superreview?(dbaron)
Blocks: 298111
(In reply to comment #17)
>I used the extra CSS selector html|select[size][multiple] because that is done
>in layout/style/forms.css:
So, in that case the select[size][multiple] appears to be there to be more specific than the subsequent select[size="0"] and select[size="1"] rules.
Your use of :not seems as if it should have the same effect.
(In reply to comment #18)
> Created an attachment (id=250194) [details]
> fix v1.1.1
> 
> Fixes stuff mentioned in comments. Thanks for all the help!
> 
This works nicely on both Camino and Minefield. Not tested with SeaMonkey.

(In reply to comment #19)
>>I used the extra CSS selector html|select[size][multiple] because that is done
>>in layout/style/forms.css:
>So, in that case the select[size][multiple] appears to be there to be more
>specific than the subsequent select[size="0"] and select[size="1"] rules.
>Your use of :not seems as if it should have the same effect.

I'm not sure about the history of that particular selector in forms.css. It is kind of redundant in that context, I think. And it selects different elements than the subsequent select[size="0"] and select[size="1"] selectors.
The use of the html|select[size]:not([size="0"]):not([size="1"]) in this particular case is to prevent 'small scrollbars' to be applied to select[size="0"] and select[size="1"].
Anyway, the problem of forms.css belongs to another bug I think (and I'll run some tests to check if it is needed or not).

Attachment #250194 - Flags: superreview?(dbaron) → superreview?(roc)
+  if ((SInt32)frameRect.size.width == smallScrollbarWidth ||
+      (SInt32)frameRect.size.height == smallScrollbarWidth) {
+    [self setControlSize:NSSmallControlSize];
+  }
+  else {
+    [self setControlSize:NSRegularControlSize];
+  }

Is this safe? What if we have a normal-sized scrollbar that is unusually short? Is it safe because a normal-sized scrollbar must be bigger than smallScrollbarWidth in both directions?
From comment #5

"The reason we can get away with setting small size like this is that even if we have a large vertical scrollbar with a height of 16, that isn't a reasonable size so it doesn't matter if we incorrectly set the native scrollbar to small."
Comment on attachment 250194 [details] [diff] [review]
fix v1.1.1

ok I guess
Attachment #250194 - Flags: superreview?(roc) → superreview+
landed on trunk
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: