147558 - {inc} Moving form elements (button moves when clicked)

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect, P3)

Description

[Brian Matzon]

I was browsing around the mozilla.org site, when I got to:
http://tegu.mozilla.org/graph/multiquery.cgi?&testname=startup
When clicking the submit button, it is moved to the line below, and the 'click'
is never performed.

Using RC2.

Comment 1

[Brian Matzon]

fixed URL, having duplicate http:// line.
Sorry for spam.

Comment 2

[Carl Busjahn]

Bug is same in RC3 on Linux.  This doesn't seem to go away by fixing simple html
syntax errors (such as missing body, head, etc, however, the Submit button is
incorrect code.  <input type="submit" value="submit"> is the Correct submit code.

<input TYPE="button" VALUE="Submit" onClick="scrape_for_names()"> is the code
used in this document to submit.  Notice the incorrect type.  Fixing the type to
submit doesn't fix the "skip" bug though.

Comment 3

[Oddsson]

Attachment: testcase

Testcase.

Please set status whiteboard to testcase.

Note: Placing <form> inside <ul> is not valid HTML 4 but I'm guessing this is
still a bug.

Comment 4

[Oddsson]

Attachment: testcase

Improved (correct) testcase.

Comment 5

[Oddsson]

->testcase

Comment 6

[Dave]

The reason the "click is never performed" is that the form button has moved so
that the mouse pointer is no longer over it when the button is released.  To
observe this:  press down the mouse over the form submit button (but don't let
go); then if the submit button jumps around, move the mouse pointer back over it
before releasing the mouse button.

Workarounds I have found:
1) Refrain from nesting forms in a list nested in a table.
2) Specify a width for any of the elements enclosing the form (for example,
<table width=250>, or <li style="width: 250px">, etc.).
3) Include enough text somewhere in the li/ul to cause line-wrap.
4) Add onMouseDown="return false;" to the input element.  This is the most
interesting one, because onMouseDown="return true;" does not work as a
workaround, suggesting that the layout shake-up happens after the onMouseDown
event falls through per usual.  But it does work when we interrupt things with
an alert box -- onMouseDown="alert(''); return true;".  Not sure what all this
means.

I don't think this bug is really about "moving form elements" (form elements
jumping around), or about missing click events.  The test case makes it easy to
narrow down the problem to a form nested in a table nested in a list. 
Furthermore, it is clear to me that the bug is not that things move around
arbitrarily, but rather that there is some change in the width of the form's
enclosing elements resulting in a reflow that makes it appear as though there
are "moving form elements."  Note that the button isn't always the thing to jump
around; it depends on where you position extra text within the ul/li that
encloses the form (for example, try putting text after the button within the
enclosing li or ul).  I would summarize what I observe this way:  "form width
shrinks on mousedown for input type=button|submit|reset|file when form is nested
in list nested in table."
I observe this behavior on daily build 2002052508, Win98.  I hope all this
helps.  Perhaps someone has something to add to these observations.

Comment 7

[Laura Krietemeyer]

Changing QA Contact

Comment 8

[Kevin McCluskey (gone)]

Reassigning to karnaze

Comment 9

[karnaze (gone)]

mass reassign to default owner

Comment 10

[Frank Wein [:mcsmurf]]

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

Comment 11

[Alexander Opitz]

This also happens on www.funcomeunity.de with latest mozilla from trunk on Linux

Comment 12

[Mats Palmgren (inactive)]

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

Comment 13

[Bernd]

         button 024EC9A0 d=756,264 me=756
         text 024ECC7C r=2 a=0,UC c=UC,UC cnt=927
         text 024ECC7C d=0,0 me=0
        block 024EC72C d=1656,264 me=756 m=1656
       block 024EC594 d=2136,264 me=1236 m=1656    <<<<<<<<<<<<<<<<<<
      block 024EC10C d=2136,456 me=1236 m=1656
     cell 024EC0AC d=2160,480 me=1260 m=1680

This is a block reflow issue. The block does not update correctly the
mMaximumWidth ( The maximum width that the frame would consume if it were
reflowed with an unconstrained available width. ref. Waterson
http://www.mozilla.org/newlayout/doc/block-and-line.html) during incr. reflow.
I don't believe that the mMaximumWidth could ever be smaller than line->Xmost in
an unconstrained block.

Comment 14

[Bernd]

Attachment: patch

Comment 15

[Bernd]

Comment on attachment 156862 [details] [diff] [review]
patch

the patch has passed the rtest, so doing the the correct thing should be fine.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I need to check what coordinate space nsLineBox::mBounds is in. 
nsBlockFrame::ReflowLine and PostPlaceLine both do:

    aLine->mMaximumWidth = aLine->mBounds.XMost();

I think that needs to subtract the left border and padding, but I'm not sure.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

In other words, does this patch cause a regression on attachment 85310 [details] with the
form element removed?

Comment 18

[Bernd]

Yes, it does cause a regression. Not a visible one without borders, but it
reports a too large mMaxWidth (it is just by borderpadding.left too large)
during the incr. reflow, this will give a too large table, which does not
tightly wrap the input.

Comment 19

[Bernd]

Attachment: another attempt

Somewhere the offset needs to be added. I can not claim that I know block
reflow good enough to know where this place is. This place now fixes the
testcase and does not regress as the other patch the scenario that David
described. Both patches have passed the rtest (so far for the meaning of rtests
:-() I will stop this buisiness now, somewhere in the maximumWidth computation
is a bug, that the xOffset is not taken into account.

Comment 20

[Hideo Saito]

Attachment: other patch

Please refer to the following two functions, nsTableRowFrame::ReflowChildren
and nsTableRowFrame::IR_TargetIsChild. With the initial reflow, maxWidth is set
to the MaximumWidth of the cellFrame. I think that with the incremental reflow,
cellMet.width should be set.

nsTableRowFrame::ReflowChildren(
	  ...
	  rv = ReflowChild(kidFrame, aPresContext, desiredSize, kidReflowState,

			   x, 0, 0, status);

	  if (cellToWatch) { 
	    nscoord maxWidth = (NS_UNCONSTRAINEDSIZE == availCellWidth) 
				? desiredSize.width :
desiredSize.mMaximumWidth;
	    // save the max element width and max width
	    if (desiredSize.mComputeMEW) {
	      cellFrame->SetPass1MaxElementWidth(desiredSize.width,
desiredSize.mMaxElementWidth);
	  ...
	    // save the max element width and max width
	    cellFrame->SetMaximumWidth(maxWidth);
	  }

nsTableRowFrame::IR_TargetIsChild(
    ...
    rv = ReflowChild(aNextFrame, aPresContext, cellMet, kidRS,
		     cellOrigin.x, 0, 0, aStatus);
    ...    
    // cache the max-elem and maximum widths
    cellFrame->SetPass1MaxElementWidth(cellMet.width,
cellMet.mMaxElementWidth);
    cellFrame->SetMaximumWidth(cellMet.mMaximumWidth);

Comment 21

[Hideo Saito]

Comment on attachment 165708 [details] [diff] [review]
other patch

Sorry, this patch is wrong since a table cell spreads unexpectedly.

Comment 22

[Hideo Saito]

Attachment: other patch

I think that a width of the indent is added to both mMaxElementWidth and
mMaximumWidth, if the value of mSpace.x means a width of the indent.

Comment 23

[Hideo Saito]

Attachment: patch

In the initial reflow, it is not requested to compute the maximum width.
In the incremental reflow, the computed maximum width does not include the
x-position rect in the space manager. I think that the maximum x-most of each
line should be computed.

Comment 24

[Bernd]

I should not review this, see comment #19. But before you ask roc, please
describe what sort of testing this patch got. The regression is a required
minimum but not sufficient to declare the patch correct.

Comment 25

[Hideo Saito]

Attachment: testcase2

I tested using the test case. The list item's margin in the second table has
percentage value. The testing is a click and text zoom. I checked that there
was no change in table size.

Comment 26

[Bernd]

s/the regression is/the regression testing/

see http://www.mozilla.org/newlayout/doc/regression_tests.html

Comment 27

[Jim Booth]

See Bug 269643 - When clicking link, page redraws with different layout, click
is ignored  

This shows a very similar problem but no form, button or list are involved. 
Just nested tables.

Comment 28

[Hideo Saito]

Attachment: testcase using nested padding

Comment 29

[Hideo Saito]

Attachment: patch

I think that influence is smaller than previous patch, although the value of
availSpace.x is set to mSpace.x.

Comment 30

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

Comment on attachment 185958 [details] [diff] [review]
patch

Looks good.

Please add a comment here mentioning that line->mMaximumWidth includes the left
border/padding of this block, but not the right.

At nsBlockFrame.cpp line 1457 or thereabouts, dbaron wrote "	  // XXXldb Why
right and not left?" ... Replace that with a comment mentioning that the reflow
state mMaximumWidth includes the left border/padding but not the right.

Put similar comments where those variable are declared in nsBlockReflowState.h
and nsLineBox.h.

Thanks!

Comment 31

[Hideo Saito]

Comment on attachment 185958 [details] [diff] [review]
patch

I think that this patch is low-risk. To prevet the line from breaking or the
elements from moving, the left border/padding of the nested blocks is added to
the maximum width in the line box.

Comment 32

[Hideo Saito]

Attachment: patch to check in

I added roc's comments to the attachment 185958 [details] [diff] [review].

Comment 33

[Hideo Saito]

roc, is the attachment 186485 [details] [diff] [review] OK? if it is so, could you check in to the trunk?

Comment 34

[Hideo Saito]

masayuki, attachment 185958 [details] [diff] [review] was approved for 1.8b3. However, I ask to check in
attachment 186485 [details] [diff] [review]. Could you check in to the trunk? and please check Bug 291759.

Comment 35

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

checked in. Thanks Hideo, the update is great.