[Webkit-unassigned] [Bug 30491] Pressing Enter in <isindex> doesn't submit it if there is no form

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=30491





--- Comment #12 from Daniel Bates <dbates at webkit.org>  2009-10-26 17:26:17 PDT ---
(In reply to comment #11)
> (From update of attachment 41814 [details])
> > +        RefPtr<HTMLFormElement> myForm = form();
> 
> I don’t think the "my" prefix is helpful to explain this local variable's
> purpose. By definition, this->form() is "my form", so the prefix simply creates
> confusion. I suggest formForSubmission as the name here.

Will do.

> 
> > +        bool isIsIndexElementAndThereIsNoForm = (inputType() == ISINDEX) && !form();
> 
> It seems a waste to call form() again when we have a local variable with that
> same value in it. Also, those parentheses are not needed, and we normally omit
> them although I suppose they could confuse newcomers to C programming.
> 
> > +        if (isIsIndexElementAndThereIsNoForm) {
> > +            // Since this <isindex> is not contained within a <form>, we create one.
> > +            myForm = new HTMLFormElement(formTag, document());
> > +            myForm->registerFormElement(this);
> > +            myForm->setMethod("GET");
> > +            if (!document()->baseURL().isEmpty()) {
> > +                // We treat the href property of the <base> element as the form action, as per section 7.5 
> > +                // "Queries and Indexes" of the HTML 2.0 spec. <http://www.w3.org/MarkUp/html-spec/html-spec_7.html#SEC7.5>.
> > +                myForm->setAction(document()->baseURL().string());
> > +            }            
> > +        }
> 
> Putting so much code in line into this function just for this element is
> inelegant. I suggest creating a separate function to create the temporary form.
> It could be a non-member function or if you prefer it could be a private member
> function named createTemporaryForm which returns a PassRefPtr<HTMLFormElement>.
> Then the code here would be just:
> 
>     RefPtr<HTMLFormElement> formForSubmission = form();
> 
>     // If there is no form and the element is an <isindex>, then create a
> temporary form just to be used for submission.
>     if (!formForSubmission && inputType() == ISINDEX)
>         formForSubmission = createTemporaryForm(this).
> 
>     // Form may never have been present, or may have been destroyed by code
> responding to the change event.
>     if (formForSubmission)
>         formForSubmission->submitClick(evt);
> 
> I think that's easier to read.

I agree. I'll refactor it.

> 
> > +        if (isIsIndexElementAndThereIsNoForm)
> > +            myForm.clear();
> 
> This is not needed. A RefPtr will automatically release its value when it goes
> out of scope and there's no need to ensure it's dereferenced before calling
> setDefaultHandled.
> 

Will remove it.

> Since the <isindex> is one of the form's elements, isn't there a chance this
> might call dispatchSimulatedClick on the <isindex>? What prevents this from
> happening? If it does happen, will it do the right thing? Why won't this just
> end up creating another temporary form in an infinite loop?

No, there is no chance this will call dispatchSimulatedClick on the <isindex>.

Looking at HTMLFormElement::submitClick, which is the only place where
dispatchSimulatedClick is called from in the file HTMLFormElement.cpp, only
image/submit buttons are dispatched simulated clicks by the first conjunction
of the inner-if condition (*), element->isSuccessfulSubmitButton(). And, by
line 903 of HTMLInputElement.cpp
<http://trac.webkit.org/browser/trunk/WebCore/html/HTMLInputElement.cpp#L903>,
HTMLInputElement::isSuccessfulSubmitButton() only returns true if it is enabled
and the type property is either "image" or "submit". But, an <isindex> element
has type "isindex", so (*) never evaluates to true. So,
HTMLFormElement::submitClick never calls dispatchSimulatedClick on an <isindex>
element.

> Do we guarantee this form won't be seen by any JavaScript code? Is there any
> way an event handler might somehow see this event and get its target and so get
> at the underlying form?

I assume you are referring to the case that dispatchSimulatedClick fired on an
<isindex> element, in which case a JavaScript installed event handler may be
able to catch the simulated click event. By the above argument, it is
impossible for method dispatchSimulatedClick() to be called. So, no simulated
click event is generated. Or am I misunderstanding your question?

> > +If you are running this test by hand, submit the phrase: "This is a test" (without quotes).
> 
> Doesn't that text automatically get put into the field by the JavaScript even
> when running the test by hand? I think that instruction is unneeded, but OK.

Changed to read: "If you are running this test by hand, press the enter/return
key on your keyboard to submit."

> Why does this need to be an HTTP test? We can do form submissions with local
> URLs. Can these same tests just be put in fast/forms instead of http/tests/misc
> and run without requiring the http server? Is it in the http section because of
> the base element?

Yes, it is in the http section because of the HTML Base element. If you want we
can probably separate the test cases so as to put
isindex-with-no-form-base-href.html in fast/form and leave
isindex-with-no-form-base-href.html in http/tests/misc.

Note, another isindex test case, isindex-formdata.html, (which I based my tests
on) is in the http/tests/misc directory.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.