201797 - HTML SELECT control missing scrollbar if both WIDTH and STYLE tags present

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect, P2)

Description

[Stephen Moehle]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4b) Gecko/20030408
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4b) Gecko/20030412

If a SELECT control in a form has both a WIDTH tag and a STYLE tag as in

<SELECT NAME="B" WIDTH=288 STYLE="width: 2.400in">

the SELECT has no scrollbar even though it has enough items that a scrollbar is
required.  If either the WIDTH tag or the STYLE tag is removed, it works.

This is a regression.  It works as is in 2003-04-08-08 trunk and fails in
04-09-09 trunk on Windows XP.

I know I could fix this simply by removing the invalid WIDTH tag, but this HTML
comes from a commercial application over which I have no control.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Stephen Moehle]

Attachment: Testcase to demonstrate the problem. SELECT has no scrollbar.

Comment 2

[Chris Casciano]

-> All/All (OS X 2003041008)

Comment 3

[Boris Zbarsky [:bzbarsky]]

This looks like roc's fault...

Comment 4

[Eugene Savitsky]

Is bug 202296 a dupe?

Comment 5

[Sterling Bates]

From bug 203316, Daniel Glazman (comment #2):

The problem here is related to the presence of the 'width' attribute on the
'select' element in the markup. This is not a valid HTML 4.01 attribute on
a 'select' element **but** apparently gecko honours it setting, in the current
test case, the width of the element to 10 'something' where something is px or 
%, I did not test.

This is a bug and we should ignore that width attribute.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

In http://www.ljh.fi/pakettilaskin.asp there are many <select>s and
some of them are not working properly even if they do not have the width
attribute. Try for example 'Valitse AMD prosessori' and  'Valitse tuote'.

I think this should block 1.4b, or at least 1.4.

Comment 7

[kitchin]

But in http://www.ljh.fi/pakettilaskin.asp they do have the style:
<select name="AMD" onChange="laske(this.form)" style=" width: 500;">
<option value=0>Valitse AMD prosessori</option>
...
Also, I looked at <LINK REL="STYLESHEET" HREF="ljh.css"> and nothing is relevant
there.

Another URL showing this bug:
http://www.kcrw.org/show/tp
It uses:
<STYLE><!--
     .dropdown  {
     	width : 227;
     	font-family : Helvetica, Arial, sans-serif;
     }
--></STYLE>
...
<SELECT NAME="keywords" SIZE="1" CLASS="dropdown" onChange="goKeyword();">

Changing width:227 to width:300 fixes it.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: new testcase

Nothing to do with the 'width' attribute. The bug occurs if the width is
constrained by perfectly valid CSS.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, what happens here is this:
-- The basic strategy for initial reflow of comboboxes is for
nsComboboxFrame::Reflow to call nsListControlFrame::Reflow to reflow the
dropdown unconstrained. If the resulting dropdown width is greater than or equal
to combobox "item + button" width, then the dropdown is not reflowed again. In
itself, that's fine.
-- The dropdown is reflowed by nsListControlFrame::Reflow. That calls
nsGfxScrollFrame::Reflow unconstrained to get the desired width and height. Then
code around line 1220 of nsListControlFrame.cpp does some fairly arcane
calculations based on screen height and number of rows to see if the dropdown is
"too tall". If it is, then it sets "visibleHeight" to something less than the
desired height returned by nsGfxScrollFrame::Reflow. That height is returned as
the desired height by nsListControlFrame::Reflow. Here is the problem: we do not
 call nsGfxScrollFrame::Reflow again to reflow the scrollbars or the scrollport
to fit this new height.
-- There is another call to nsGfxScrollFrame::Reflow, with a comment saying "//
Do a second reflow with the adjusted width and height settings", but it's all
guarded to happen only when mPassId == 2. This "second pass reflow" only happens
when the combobox needs to widen the dropdown to be the same width as the "item
+ button", so that doesn't happen here.
-- This probably used to work because nsScrollFrame didn't have any guts that
needed to be reflowed if its actual size was different from its desired size.

Possible solutions:
-- Do an extra nsGfxScrollFrame::Reflow if the height is reduced by
nsListControlFrame::Reflow
-- Scrap the complicated height-reducing logic in nsListControlFrame::Reflow.
Compute a simple, reasonable maximum height (say, 3/8 the screen height) and
pass that in as the available height to the one and only call to
nsGfxScrollFrame::Reflow.

I think the first option is actually the easiest and least risky for now. The
height-reducing logic has a number of special cases which probably have good
reasons to be there, I just don't know what those reasons are.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

This patch causes us to do the second nsGfxScrollFrame reflow if we adjusted
the width or height. It fixes all the duplicates. Sometimes it introduces a
horizontal scrollbar which wasn't there before, but I think that's OK. I can
still see a minor bug where sometimes you can't drag the thumb on a dropdown,
but I think that's an existing bug.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This patch causes extra reflows of dropdowns so it may not be the best for Tp.
We may want to bite the bullet and go with a constrained-height reflow of
nsGfxScrollFrame.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: better fix

No need for that. This patch only does extra reflows if we really need them.
The adjustment of visibleWidth I'm deleting doesn't seem to have any effect, in
fact it seems to have always been incorrect. Getting rid of that adjustment
makes visibleWidth match scrollAreaWidth in the common case for unconstrained
reflow.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 122278 [details] [diff] [review]
better fix

Need a review on this. I've tested it with a variety of combobox-heavy
testcases and it seems good.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: listboxes/comboboxes with borders testcase

Good testcase with different kinds of combo/listboxes and padding.

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 122278 [details] [diff] [review]
better fix

Nit: isn't prevailing style in layout at least to put the || at the end of the
continued line, not at the start of the continuation?

/be

Comment 16

[[e-mail] (gone - use [e-mail] instead)]

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

Comment 17

[[e-mail] (gone - use [e-mail] instead)]

Hi Robert - you landed this, but the bug isn't marked FIXED yet.

Just making sure it's still on your radar.  It appears to have fixed bug 203316,
as well.

Comment 18

[David Brittain]

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

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This was checked in a few days ago. It caused about a 1% Tp hit :-(. Probably
just the extra reflows required to get constrained-width dropdowns with the
right scrollbars. Listbox and combobox reflow should probably revisited in the
future. In particular we could defer some of the dropdown reflow until the
dropdown is actually shown.

Comment 20

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED on build 2004-06-30-08 using
http://bugzilla.mozilla.org/attachment.cgi?id=120328&action=view as my testcase
on Windows XP.