use small native scrollbars for HTML form elements

RESOLVED FIXED

Status

()

Core
Widget: Cocoa
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Josh Aas, Assigned: Josh Aas)

Tracking

Trunk
PowerPC
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

(Assignee)

Description

11 years ago
We should use small native scrollbars for HTML form elements. That would be more efficient space-wise and it'd look better.
(Assignee)

Comment 1

11 years ago
Created attachment 249865 [details] [diff] [review]
fix v0.2

This works for Firefox only. It isn't the right way to do it, but it is a start.
(Assignee)

Comment 2

11 years ago
Created attachment 249866 [details]
screenshot comparison w/v0.2

Screenshots of v0.2 patched vs. unpatched.
(Assignee)

Updated

11 years ago
Attachment #249866 - Attachment is patch: false
Attachment #249866 - Attachment mime type: text/plain → image/jpeg
(Assignee)

Comment 3

11 years ago
Created attachment 249880 [details] [diff] [review]
fix v1.0

This should work for all products, including Camino.
Attachment #249865 - Attachment is obsolete: true
Attachment #249880 - Flags: review?
(Assignee)

Updated

11 years ago
Attachment #249880 - Flags: review? → review?(stuart.morgan)

Comment 4

11 years ago
Comment on attachment 249880 [details] [diff] [review]
fix v1.0

Sexy.
r=smorgan.
Attachment #249880 - Flags: review?(stuart.morgan) → review+
(Assignee)

Updated

11 years ago
Attachment #249880 - Flags: superreview?(dbaron)
(Assignee)

Comment 5

11 years ago
+  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.

Comment 6

11 years ago
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'.

Comment 7

11 years ago
Created attachment 249901 [details]
screenshot for comment 6

Camino (on top) and Minefield.
This fixes bug 311454, right?
(Assignee)

Comment 9

11 years ago
Created attachment 249933 [details] [diff] [review]
fix v1.1

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)
Created attachment 249951 [details]
Select multiples without borders

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
Duplicate of this bug: 311454

Comment 12

11 years ago
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?

Comment 13

11 years ago
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....)

Updated

11 years ago
Attachment #249951 - Attachment is patch: false
Attachment #249951 - Attachment mime type: text/plain → image/png

Comment 15

11 years ago
It seems that the text in the popup-style selects is also shifted up by 1px with the patch.
(Assignee)

Comment 16

11 years ago
Comment on attachment 249933 [details] [diff] [review]
fix v1.1

I'll post an updated patch soon.
Attachment #249933 - Flags: superreview?(dbaron)
(Assignee)

Comment 17

11 years ago
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.
(Assignee)

Comment 18

11 years ago
Created attachment 250194 [details] [diff] [review]
fix v1.1.1

Fixes stuff mentioned in comments. Thanks for all the help!
Attachment #249933 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #250194 - Flags: superreview?(dbaron)

Updated

11 years ago
Blocks: 298111

Comment 19

11 years ago
(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.

Comment 20

11 years ago
(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).

(Assignee)

Updated

11 years ago
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?
(Assignee)

Comment 22

11 years ago
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+
(Assignee)

Comment 24

11 years ago
landed on trunk
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Filed bug 367571 as a followup.
You need to log in before you can comment on or make changes to this bug.