Closed Bug 147558 Opened 23 years ago Closed 19 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
Future

People

(Reporter: brian, Unassigned)

References

()

Details

(Keywords: testcase)

Attachments

(6 files, 5 obsolete files)

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.
fixed URL, having duplicate http:// line. Sorry for spam.
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.
Attached file testcase (obsolete) —
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.
Attachment #85298 - Attachment is obsolete: true
Attached file testcase
Improved (correct) testcase.
->testcase
Keywords: testcase
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Changing QA Contact
QA Contact: petersen → amar
Reassigning to karnaze
Assignee: attinasi → karnaze
Priority: -- → P3
Target Milestone: --- → Future
mass reassign to default owner
Assignee: karnaze → table
Component: Layout → Layout: Tables
QA Contact: amar → madhur
Target Milestone: Future → ---
Target Milestone: --- → Future
*** Bug 187568 has been marked as a duplicate of this bug. ***
This also happens on www.funcomeunity.de with latest mozilla from trunk on Linux
OS: Windows 2000 → All
*** Bug 252884 has been marked as a duplicate of this bug. ***
Blocks: 195770, 237159
Summary: Moving form elements → Moving form elements (button moves when clicked)
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.
Assignee: core.layout.tables → nobody
Component: Layout: Tables → Layout: Block and Inline
QA Contact: madhur → core.layout.block-and-inline
Summary: Moving form elements (button moves when clicked) → {incr} Moving form elements (button moves when clicked)
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → bernd_mozilla
Status: NEW → ASSIGNED
Comment on attachment 156862 [details] [diff] [review] patch the patch has passed the rtest, so doing the the correct thing should be fine.
Attachment #156862 - Flags: superreview?
Attachment #156862 - Flags: review?
Attachment #156862 - Flags: superreview?(dbaron)
Attachment #156862 - Flags: superreview?
Attachment #156862 - Flags: review?(dbaron)
Attachment #156862 - Flags: review?
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.
In other words, does this patch cause a regression on attachment 85310 [details] with the form element removed?
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.
Attached patch another attemptSplinter Review
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.
Attachment #156862 - Attachment is obsolete: true
Attachment #156862 - Flags: superreview?(dbaron)
Attachment #156862 - Flags: review?(dbaron)
Assignee: bernd_mozilla → nobody
Status: ASSIGNED → NEW
No longer blocks: 237159
Attached patch other patch (obsolete) — Splinter Review
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 on attachment 165708 [details] [diff] [review] other patch Sorry, this patch is wrong since a table cell spreads unexpectedly.
Attachment #165708 - Attachment is obsolete: true
Attached patch other patch (obsolete) — Splinter Review
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.
Blocks: 189367
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #165805 - Attachment is obsolete: true
Attachment #182370 - Flags: review?(bernd_mozilla)
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.
Attachment #182370 - Flags: review?(bernd_mozilla)
Attached file 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.
s/the regression is/the regression testing/ see http://www.mozilla.org/newlayout/doc/regression_tests.html
Summary: {incr} Moving form elements (button moves when clicked) → {inc} Moving form elements (button moves when clicked)
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.
Blocks: 291759
Attached patch patchSplinter Review
I think that influence is smaller than previous patch, although the value of availSpace.x is set to mSpace.x.
Attachment #182370 - Attachment is obsolete: true
Attachment #185958 - Flags: review?(roc)
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!
Attachment #185958 - Flags: superreview+
Attachment #185958 - Flags: review?(roc)
Attachment #185958 - Flags: review+
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.
Attachment #185958 - Flags: approval1.8b3?
I added roc's comments to the attachment 185958 [details] [diff] [review].
Attachment #185958 - Flags: approval1.8b3? → approval1.8b3+
roc, is the attachment 186485 [details] [diff] [review] OK? if it is so, could you check in to the trunk?
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.
Hardware: PC → All
checked in. Thanks Hideo, the update is great.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: