Bug 28900 – <select> controls doesn't render size=2 or size=3 properly

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Dirk Pranke]

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.

[Dirk Pranke]

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.

[Gérard Talbot (no longer involved)]

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

[Naoki Takano]

Created attachment 103169 [details]
Patch

[Naoki Takano]

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.

[Alexey Proskuryakov]

*** Bug 17648 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

http://code.google.com/p/chromium/issues/detail?id=4579

[Alexey Proskuryakov]

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.

[Naoki Takano]

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.

[Alexey Proskuryakov]

>             - 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?

[Naoki Takano]

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?

[Naoki Takano]

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?

[Alexey Proskuryakov]

> 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.

[Naoki Takano]

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.

[Alexey Proskuryakov]

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.

[Naoki Takano]

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.

[Alexey Proskuryakov]

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.

[Naoki Takano]

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.

[Alexey Proskuryakov]

wkScrollbarMinimumThumbLength() returns the minimum thumb height that can possibly be drawn with the system theme. Are you saying that its result is incorrect? Why?

[Eric Seidel (no email)]

Comment on attachment 103169 [details]
Patch

Seems reasonable.  WHy was this 4 in teh first place?  Do other ports want this minimum of 4?

[Eric Seidel (no email)]

I'm not seeking to override ap here.  If ap has objections to this patch he should feel encouraged to r- it.

[Darin Adler]

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.

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

*** Bug 126515 has been marked as a duplicate of this bug. ***

[ChangSeok Oh]

(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.

[Alexey Proskuryakov]

(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).

[ChangSeok Oh]

(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

[ChangSeok Oh]

Created attachment 220749 [details]
screenshot on mavericks

I attached test result on mavericks.

[ChangSeok Oh]

Created attachment 220756 [details]
Proposed patch

[Radar WebKit Bug Importer]

<rdar://problem/15786308>

[ChangSeok Oh]

I don't know why the bots don't stop running tests. :(

[ChangSeok Oh]

Created attachment 220841 [details]
Updated patch

expected png results for mac are updated.

[Darin Adler]

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?

[Alexey Proskuryakov]

(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>

[ChangSeok Oh]

(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.

[ChangSeok Oh]

(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?

[ChangSeok Oh]

Created attachment 220974 [details]
Updated patch

RenderListBox should manage positive integers for its size.

[Alexey Proskuryakov]

> 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.

[ChangSeok Oh]

Created attachment 221243 [details]
Updated patch

update expected results for mac port.

[ChangSeok Oh]

Created attachment 221469 [details]
Update patch

Updated listbox-bidi-align-expected.txt for mac-mountainlion

[ChangSeok Oh]

>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.

[Alexey Proskuryakov]

Comment on attachment 221469 [details]
Update patch

This still makes fast/replaced/replaced-breaking.html fail on Mac.

[ChangSeok Oh]

Created attachment 222007 [details]
Updated patch

Resolved hash mismatch. Hope this to be final..

[Alexey Proskuryakov]

Still failing:

Regressions: Unexpected text-only failures (1)
  fast/replaced/replaced-breaking.html [ Failure ]

[ChangSeok Oh]

(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?

[ChangSeok Oh]

(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?

[ChangSeok Oh]

Created attachment 222125 [details]
Updated patch

[ChangSeok Oh]

Created attachment 222126 [details]
Updated patch

[ChangSeok Oh]

Created attachment 222194 [details]
Updated patch

Update the mountainlion result

[ChangSeok Oh]

Created attachment 222206 [details]
Updated patch

Rebased the patch

[ChangSeok Oh]

Created attachment 222327 [details]
Updated patch

Rebase EFL & GTK results.

[ChangSeok Oh]

Comment on attachment 222327 [details]
Updated patch

All greens!

[ChangSeok Oh]

Review please? or any concern?

[Michael Catanzaro]

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.

[Ahmad Saleem]

https://github.com/WebKit/WebKit/pull/4450

[EWS]

Committed 254698@main (bef8deafdcd3): <https://commits.webkit.org/254698@main>

Reviewed commits have been landed. Closing PR #4450 and removing active labels.

[Ahmad Saleem]

*** Bug 21871 has been marked as a duplicate of this bug. ***