Created attachment 38907 [details] test case <select> controls seem to have a minimum size of 4 (or either 1 or 4 if it is not a multi-select). FF 3.5, IE 8, and Opera 10 all support sizes of 2 and 3, but no WebKit-based browser (Safari 4, Chrome nightlies) seem to.
I'm not working on this now nor do I expect to be in the near future, so I am disclaiming ownership in case someone else wants to pick this up.
Dirk, I confirm your finding and your bug report. Additional testcase coming from HTML 4 test suite conformance: HTML4 Test Suite on select and size attribute http://www.w3.org/MarkUp/Test/HTML401/current/tests/sec17_6-BF-01.html This bug has also been reported at KDE: Size attribute for select incorrectly rendered [HTML 4 conformance, HTML form] https://bugs.kde.org/show_bug.cgi?id=237438 regards, Gérard
Created attachment 103169 [details] Patch
Uploaded the patch. Please review it. The change is very small but I wonder if the test is OK. If it's Ok, I'll make win, mac version.
*** Bug 17648 has been marked as a duplicate of this bug. ***
http://code.google.com/p/chromium/issues/detail?id=4579
As it's apparent from the code code being changed, this was an intentional choice at some point. Could you please find out what the original reasons for this choice were using svn annotate? I'd like to see screenshots of how short selects look on other platforms. I suspect that some may be broken, making this change infeasible.
According to git blame, the following change added minSize=4. commit a9b3e5324c977c9838694d5761d8b8051d8220d1 Author: darin <darin@268f45cc-cd09-0410-ab3c-d52691b4dbfc> Date: Fri Jan 19 23:12:20 2007 +0000 LayoutTests: Reviewed by Adele. - test for html4.css problem where we had a missing semicolon and thus missed parsing a style rule Now I'm trying to make image on Win/Mac. (In reply to comment #7) > As it's apparent from the code code being changed, this was an intentional choice at some point. Could you please find out what the original reasons for this choice were using svn annotate? > > I'd like to see screenshots of how short selects look on other platforms. I suspect that some may be broken, making this change infeasible.
> - test for html4.css problem where we had a missing semicolon and thus missed > parsing a style rule This is surprising, I suspect that this is not the right change. What's the svn revision number?
It looks r18989. (In reply to comment #9) > > - test for html4.css problem where we had a missing semicolon and thus missed > > parsing a style rule > > This is surprising, I suspect that this is not the right change. What's the svn revision number?
Hmm... The fix affects Mac result. I'm looking into it... (In reply to comment #10) > It looks r18989. > > (In reply to comment #9) > > > - test for html4.css problem where we had a missing semicolon and thus missed > > > parsing a style rule > > > > This is surprising, I suspect that this is not the right change. What's the svn revision number?
> It looks r18989. The constant was moved in this revision, but it already existed. I tracked it down to r16663 (Initial implementation of engine-based list box control), but it might have been taken from earlier Cocoa-based version. This is likely related to bug 5005.
I was looking into the source code, and I found where I should change the code. But I'm not sure how to fix it yet. Because that part is using HIThemeTrackDrawInfo which is Carbon API. I cannot find the doc even in Apple site, but do you know where we can get it? Thanks, (In reply to comment #12) > > It looks r18989. > > The constant was moved in this revision, but it already existed. I tracked it down to r16663 (Initial implementation of engine-based list box control), but it might have been taken from earlier Cocoa-based version. > > This is likely related to bug 5005.
Could you tell me what specifically you are looking for? I don't know if there is any documentation besides what is in header file, but I can try to find an answer for a specific question.
I see. I want to know how we can set the up/down arrow buttons and thumb. I believe we can specify the height with HIThemeTrackDrawInfo in ScrollbarThemeChromiumMac::paint(). So I want to know which member can change the height in the struct. Do you have the idea? Thanks, (In reply to comment #14) > Could you tell me what specifically you are looking for? I don't know if there is any documentation besides what is in header file, but I can try to find an answer for a specific question.
I don't think that it's possible to change the size of up/down buttons - how would they be rendered if resized? And the size of thumb is controlled by WebKit, so there should be code showing ho to do that. See also: bug 66311.
Sorry, my explanation was confusing. And thank you for your suggestion. I know we cannot change the size of up/down buttons. As you pointed out, I think wkScrollbarMinimumThumbLength() returns too big number. That is the problem. But I also use to change it like Bug 66311, still the minimum size is too big. Can we shrink the minimum size of thumb? It looks the thumb size is completely controlled by Cocoa(or Carbon?) API. Also I looked into WebKit side and they also do the same thing. Please see http://www.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/platform/mac/ScrollbarThemeMac.mm&type=cs&q=minimumThumbLength&exact_package=chromium&l=419 Thanks, (In reply to comment #16) > I don't think that it's possible to change the size of up/down buttons - how would they be rendered if resized? And the size of thumb is controlled by WebKit, so there should be code showing ho to do that. > > See also: bug 66311.
wkScrollbarMinimumThumbLength() returns the minimum thumb height that can possibly be drawn with the system theme. Are you saying that its result is incorrect? Why?
Comment on attachment 103169 [details] Patch Seems reasonable. WHy was this 4 in teh first place? Do other ports want this minimum of 4?
I'm not seeking to override ap here. If ap has objections to this patch he should feel encouraged to r- it.
The minimum of 4 was to make Mac scroll bars work properly. To preserve that we probably need to allow themes to supply a minimum height, or continue to do so if they are already.
Comment on attachment 103169 [details] Patch Since the question in comment 18 hasn't been answered at all, r- seems appropriate. Changing to that until there is an understanding why this change is OK.
*** Bug 126515 has been marked as a duplicate of this bug. ***
(In reply to comment #23) > *** Bug 126515 has been marked as a duplicate of this bug. *** I'd like to continue this bug. As I said in 126515, current situation is changed. I doubt if we should continuously stick 4 for the minSize . I couldn't find any problem for Macport running on Marvericks and Lion. The scrollbar shows up and automatically hide after few seconds on the platforms. And also I don't think GTK port has any problem for size=2/3, though GTK+ 3.0 doesn't paint scrollbar for such a short list, but it presents more lists remained by using up/down arrow indicators. I propose to change minSize from 4 to 2. Updated change is coming.
(In reply to comment #24) > The scrollbar shows up and automatically hide after few seconds on the platforms. This is the behavior with a trackpad. When using a mouse, a different scroll bar style is used by default, does that look right too? You can test both scroll bar styles by changing system preferences (General preference pane).
(In reply to comment #25) > (In reply to comment #24) > > The scrollbar shows up and automatically hide after few seconds on the platforms. > > This is the behavior with a trackpad. When using a mouse, a different scroll bar style is used by default, does that look right too? > > You can test both scroll bar styles by changing system preferences (General preference pane). Well. I'm using a magic mouse. I re-tested the scroll bar with the option "Preference>General>Show scroll bars>Always" on Mavericks. I think no problem there. Is there another option to change scrollbar style except that? ''a
Created attachment 220749 [details] screenshot on mavericks I attached test result on mavericks.
Created attachment 220756 [details] Proposed patch
<rdar://problem/15786308>
I don't know why the bots don't stop running tests. :(
Created attachment 220841 [details] Updated patch expected png results for mac are updated.
Comment on attachment 220841 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=220841&action=review > Source/WebCore/rendering/RenderListBox.cpp:72 > -// The minSize constant was originally defined to render scrollbars correctly. > -// This might vary for different platforms. > -const int minSize = 4; > +const int minSize = 2; Why 2 rather than 3 or 1?
(In reply to comment #30) > I don't know why the bots don't stop running tests. :( It's hitting errors like this: Use of uninitialized value $ENV{"PLATFORM_NAME"} in string eq at WebCore/bindings/scripts/CodeGeneratorObjC.pm line 114. WebCore/bindings/objc/PublicDOMInterfaces.h:29:10: fatal error: 'wtf/Platform.h' file not found #include <wtf/Platform.h>
(In reply to comment #32) > (From update of attachment 220841 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=220841&action=review > > > Source/WebCore/rendering/RenderListBox.cpp:72 > > -// The minSize constant was originally defined to render scrollbars correctly. > > -// This might vary for different platforms. > > -const int minSize = 4; > > +const int minSize = 2; > > Why 2 rather than 3 or 1? Good point. I missed the case multiple=true so I thought RenderListBox couldn't have size=1. But it's not true. If select have multiple=true, RenderListBox can have the size=1. Theoretically RenderListBox should manage all positive integers for the size attribute. Both FireFox and IE allow the case, size=1 & multiple=true.
(In reply to comment #33) > (In reply to comment #30) > > I don't know why the bots don't stop running tests. :( > > It's hitting errors like this: > > Use of uninitialized value $ENV{"PLATFORM_NAME"} in string eq at WebCore/bindings/scripts/CodeGeneratorObjC.pm line 114. > WebCore/bindings/objc/PublicDOMInterfaces.h:29:10: fatal error: 'wtf/Platform.h' file not found > #include <wtf/Platform.h> Hrm.. I've never seen this errors before. Is this really related with this patch?
Created attachment 220974 [details] Updated patch RenderListBox should manage positive integers for its size.
> Hrm.. I've never seen this errors before. Is this really related with this patch? Indeed, this error is not caused by this patch. Filed bug 126873 about it. I'm not sure why EWS fails to report the results. It sees these regressions from this patch: Regressions: Unexpected text-only failures (2) fast/forms/listbox-bidi-align.html [ Failure ] fast/replaced/replaced-breaking.html [ Failure ] Also, style bot hits bug 126702.
Created attachment 221243 [details] Updated patch update expected results for mac port.
Created attachment 221469 [details] Update patch Updated listbox-bidi-align-expected.txt for mac-mountainlion
>Sharding tests ... >Starting 1 worker ... > fast/replaced/replaced-breaking.html -> pixel hash failed (but diff passed) >[1/1] fast/replaced/replaced-breaking.html failed unexpectedly (text diff) Weird. the test passed on my marvericks.. pixel hash failed, but diff passed?? What does this mean..? sigh.
Comment on attachment 221469 [details] Update patch This still makes fast/replaced/replaced-breaking.html fail on Mac.
Created attachment 222007 [details] Updated patch Resolved hash mismatch. Hope this to be final..
Still failing: Regressions: Unexpected text-only failures (1) fast/replaced/replaced-breaking.html [ Failure ]
(In reply to comment #43) > Still failing: > > Regressions: Unexpected text-only failures (1) > fast/replaced/replaced-breaking.html [ Failure ] :( Is there any to see or download actual test results?
(In reply to comment #43) > Still failing: > > Regressions: Unexpected text-only failures (1) > fast/replaced/replaced-breaking.html [ Failure ] :( Is there any way to see or download actual test results?
Created attachment 222125 [details] Updated patch
Created attachment 222126 [details] Updated patch
Created attachment 222194 [details] Updated patch Update the mountainlion result
Created attachment 222206 [details] Updated patch Rebased the patch
Created attachment 222327 [details] Updated patch Rebase EFL & GTK results.
Comment on attachment 222327 [details] Updated patch All greens!
Review please? or any concern?
Comment on attachment 222327 [details] Updated patch Hi, Apologies that your patch was not reviewed in a timely manner. Since it's now quite old, I am removing it from the review request queue. Please consider rebasing it on trunk and resubmitting. To increase the chances of getting a review, consider using 'Tools/Scripts/webkit-patch upload --suggest-reviewers' to CC reviewers who might be interested in this bug.
https://github.com/WebKit/WebKit/pull/4450
Committed 254698@main (bef8deafdcd3): <https://commits.webkit.org/254698@main> Reviewed commits have been landed. Closing PR #4450 and removing active labels.
*** Bug 21871 has been marked as a duplicate of this bug. ***