153995 - Button on table inside form bleeds over form border

Status: VERIFIED FIXED

(Core :: Layout: Tables, defect, P1)

Description

[Louis Erickson]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.1a+) Gecko/20020624
BuildID:    2002062408

When a form contains a table, and that table contains a submit button, the
button can bleed off of the edge of the table.

Table borders draw over the button and look funny.

You can't click on the button in the space that is not inside the table, even
though the button shows.  This is true if borders are visible or not.  Having
the borders visible makes it much more obvious what is going on.

This happened on a page on our private intranet; I have reduced it to a much
smaller form and removed any possibly proprietary data.  I will attach the HTML
for this form to the bug.

This HTML validates with the W3C's validator and looks fine in other browsers. 
The paint bug is strange, but the inability to click on the bottom third of the
button is particularly annoying.

Reproduced in Mozilla 1.0 release and in build 2002062408.

If there is any other information I can provide, I am happy to do so.

Reproducible: Always
Steps to Reproduce:
1. View HTML with <form><table><submit></table></form> construct.  Sample included.


Actual Results:  Form draws, but button bleeds out of form, and part of it
cannot be clicked.

Expected Results:  Form should contain all of the button, and it should all be
clickable.

I will attach a sample file containing HTML to reproduce this.

Comment 1

[Louis Erickson]

Attachment: HTML to reproduce table sizing problem

Here is the HTML I was using to reproduce this issue.  Look at the "Login"
button and try and click below the table border.

Comment 2

[Greg L.]

I can confirm that the button overlaps the table on Win2k.

Comment 3

[Mats Palmgren (inactive)]

Attachment: Testcase

Comment 4

[Rick Ju]

Attachment: simpler testcase

a simpler testcase
in my opnion, this should be a very servere problem.
is it a regression?  i can see the overflow on 1.2a both for linux and w2k.

the problem may come from mStyleHeight is shared by mHasFixedHeight and
mHasPctHeight. but i'm not sure since still can't figure out the whole
agrithom.

Comment 5

[Jerry]

remove button from simpler testcase also has problem.
Table's bug.

Comment 6

[Rick Ju]

Attachment: patch 0.1

as i mentioned in comment #4, the problem comes from a wrong assumption: 
  the pct height is always greater than fixed height. based on it,
nsTableRowFrame shared its mStyleHeight for fixed and pct height. whenever a
pct height exists, it will remember max pct height in mStyleHeight and ignore
all fixed height.

solution: split mStyleHeight to mStylePctHeight and mStyleFixedHeight

This solution do add a member var to nsTableRowFrame. maybe better method
exists?

Chris, can u r= ?

Comment 7

[karnaze (gone)]

Comment on attachment 103568 [details] [diff] [review]
patch 0.1

r=karnaze if it passes the table regression tests. If there are some failed
tests, the new behavior may still be preferable.

Rick, please send me email directly when asking for a review.

Comment 8

[Rick Ju]

i smoketest the patch. it seems that it's ok but im not still very sure since
it's my first smoketest :) 

all the failed i got:

$grep failed N153995-verify.txt 

regression test verify/163614.rgd failed
GTK theme failed for widget type 87, error was 2, state was
[active=0,focused=0,inHover=0,disabled=0]
GTK theme failed for widget type 87, error was 2, state was
[active=0,focused=0,inHover=0,disabled=0]
GTK theme failed for widget type 82, error was 2, state was
[active=0,focused=0,inHover=0,disabled=0]
regression test verify/bug2479-1.rgd failed
regression test verify/bug2479-3.rgd failed
GTK theme failed for widget type 83, error was 2, state was
[active=0,focused=0,inHover=0,disabled=0]
regression test verify/bug96343.rgd failed
regression test verify/select_init.rgd failed
regression test verify/select_jsadd.rgd failed
regression test verify/select_onchange.rgd failed
regression test verify/bug42481.rgd failed



i checked all the testcases but all the pages looks ok. 

can i say the patch have passed or not?

Comment 9

[Rick Ju]

typo: i meant regression test.

Comment 10

[karnaze (gone)]

The tests can give bogus bbox differences or bogus frame state mismatches. If
you have looked at the failed tests which aren't due to frame state mismatches
and they don't differ visually from before your changes, then there isn't a
problem. 

Comment 11

[Rick Ju]

//confused..
Chris, can u explain it more?
in this case, all the failed tests mismatch in FrameState while no difference in
bbox. 

Comment 12

[karnaze (gone)]

I always see bogus frame state mismatches, and I usually don't pay much
attention to them. The only time I would be concerned about frame state
mismatches is if I actually changed the frame state bits, but even then, if a
problem arose, it would be reflected in a bbox difference or a tree difference.
So, it sounds like your patch passes.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 103568 [details] [diff] [review]
patch 0.1

> +  // the height based on a style pct on either the row or any cell

I know you just copied the comment, but please use "percentage height" instead
of "pct" there.

Also, mStylePercentHeight would be preferable, I would think... but karnaze's
call on the variable naming.

The code in nsTableRowFrame::SetFixedHeight needs to be reindented if this
patch is applied.

sr=bzbarsky with those...

Comment 14

[Rick Ju]

still prefer to use mStylePctHeight since there are full of such xxxPctXxxx
naming in nsTableFrame.cpp, nsTableRowFrame.cpp...

so using Pct should be more compatible in naming style. 

ie.: 
nsTableFrame::IsPctHeight()    
PRBool HasPctCol() const;
void SetHasPctCol(PRBool aValue);
...

Comment 15

[Bernd]

Boris, please dont invent a new naming convention, pct is just fine for me.

Comment 16

[Bernd]

fix checked in

Comment 17

[Madhur Bhatia]

verified with testcase attahcment 88982, attachment 89004 [details], attachment 101049 [details]
on winXP and linux platforms with trunk build 2003-03-12