125578 - Remove nsFormFrame entirely from existence

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[John Keiser (jkeiser)]

According to pollmann, nsFormFrame only existed to manage form controls and
submission.  When bug 108308 lands, that purpose will be entirely obsolete.

Comment 1

[John Keiser (jkeiser)]

There is a question in my mind about whether or not save/restore state has
anything to do with nsFormFrame.  Thus bug 108309 is marked as a blocker though
it might not be.

People believe this will fix bug 86633 and bug 44470.  It will sure as heck
reduce memory consumption for forms.

Comment 2

[John Keiser (jkeiser)]

This is 1.1 material at the moment, esp. since (we think) it blocks those other
bugs.

Comment 3

[John Keiser (jkeiser)]

Attachment: Patch

This removes nsFormFrame and fixes the dependent bugs as well by removing the
!important rule from stylesheet.  To accomplish this it was necessary to move
the password manager initialization and form submit on pressing ENTER to
content.  Additionally, I had to have submit() and reset() call
HandleDOMEvent() directly, since HandleDOMEventWithTarget *does not work* for
elements with display: none.

Tested against:
- pressing enter to submit (bug 99920)
- forms with display: inline (bug 86633) and none (bug 44470)
- http://www.johnkeiser.com/cgi-bin/mozform.pl
- password manager init (verify that password is filled in when you go to the
first password page)
- Bugzilla (bug search at bottom, upload attachment, make comment, query)

Comment 4

[Alexandru Savulov]

Comment on attachment 85110 [details] [diff] [review]
Patch

r= alexsavulov

boy, this was a big one again. clean removal, good work.

Comment 5

[John Keiser (jkeiser)]

Attachment: Patch v1.0.1

Fixed JST's sr:
- changed nsIGfxTextControlFrame2::FireIfChanged() to CheckFireOnChange()
- made nsFormControlHelper::GetDisabled() inline-able and just call
nsIContent::HasAttr()
- nsHTMLInputElement::HandleDOMEvent() - moved the FireIfChanged() inside the
if (frame) block

Deliberately not fixed from JST's sr:
- still use HandleDOMEvent() directly instead of
PresShell::HandleEventWithTarget.  This makes it possible to call submit() or
reset() on a form with display: none since HandleEventWithTarget does not work
in that regard (probably should be fixed but I'm not sure of the
ramifications).   I have stepped through the code and HandleEventWithTarget
just happens not do anything extra in this case (now that nsFormFrame is gone).
 Added comments to this effect and filed bug 148542 to deal
HandleEventWithTarget.

Also:
- added a comment on nsFormControlHelper::GetName()
- removed nsFormControlHelper::GetValue() and GetInputElementValue(), neither
of which were used
- added Win32/Mac build changes

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 85916 [details] [diff] [review]
Patch v1.0.1

sr=jst

Comment 7

[John Keiser (jkeiser)]

Comment on attachment 85916 [details] [diff] [review]
Patch v1.0.1

Carrying r=alexsavulov

Comment 8

[John Keiser (jkeiser)]

Fix checked in.  Godspeed.

Comment 9

[John Keiser (jkeiser)]

For drivers considering inclusion of this patch in 1.0.x: bug 149282 and bug
149685 need to be checked in if this is checked in.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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