326250 - [FIX]Input button layout doesn't use GetValue()

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P3)

Description

[Steuard Jensen]

As described in Bug 114997 comment 54, <input type="button"> layout gets the button's value straight from its value attribute rather than calling nsHTMLInputElement::GetValue() as it should:

http://lxr.mozilla.org/mozilla1.8/source/layout/forms/nsHTMLButtonControlFrame.cpp#202
----------
NS_IMETHODIMP
nsHTMLButtonControlFrame::GetValue(nsAString* aResult)
{
  return nsFormControlHelper::GetValueAttr(mContent, aResult);
}
----------

This GetValueAttr() function is defined in nsFormControlHelper.cpp; as the name
indicates, it finds the value using GetAttr().  This GetValue() function is
inherited and called in turn by
nsGfxButtonControlFrame::CreateAnonymousContent():

http://lxr.mozilla.org/mozilla1.8/source/layout/forms/nsGfxButtonControlFrame.cpp#159
----------
  result = GetValue(&initvalue);
----------

That can lead to unwanted behavior when GetValue() gives a different result (for example, after my patch to bug 114997, buttons with leading or trailing newlines  in their value get excess whitespace on top; see that bug for a screenshot and a testcase that displays the problem after the patch is applied).

I see two possible fixes for this problem.  The first is to replace the function call in nsGfxButtonControlFrame.cpp with something like

  result = mContent.GetValue(&initvalue);

(For that to actually work, some sort of QueryInterface would have to be used on mContent, but I do not yet have any idea how to do that.)  This fixes the problem, but it leaves nsHTMLButtonControlFrame::GetValue() as a trap for the unwary. 

Another fix would be to make essentially this same change in nsHTMLButtonControlFrame::GetValue() itself.  The difficulty with that is that this more general class also applies to <button> elements.  It looks like nsHTMLButtonElement _does_ have a GetValue() method (it is called during form submission), but I haven't been able to track down its definition in the source to make sure it is appropriate.  Even if it is, the QI process might need to be different in the two cases anyway (again, I know nothing about QI yet).

In either case, it seems that a fix for this bug would be simple for anyone who is comfortable with QI.  I'll look forward to learning what the syntax is supposed to be.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Sicking, can you think of a reason why this code is doing GetAttr() rather than GetValue()?  I can't....

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The only reason I can think of would be performance which is a bogus reason really.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

I got a little carried away.. it's so easy in forms code!

Changes:

1)  Use the _same_ code in initial label setting and dealing with value attr
    changes.  We used to do different things in the two cases.
2)  Use nsIDOMHTMLInputElement::value in that code.
3)  Change a SetText that tried to notify to not do so (the node's not in the
    DOM tree, so no need to try and notify).
4)  Eliminate the now-unused nsFormControlHelper::GetValueAttr

Comment 4

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 5

[Steuard Jensen]

Quick summary: I want to know if this patch together with my patch to bug 114997 eliminates the button display problem that I described in bug 114997 comment 48 (and in comment 0 here), but I only have a 1.8 branch source tree.  When I adapt this patch for the 1.8 branch, that problem persists.  Could someone with a trunk build see if the problem is fixed there?

(In reply to comment #3)
> Created an attachment (id=211559) [edit]
> Proposed patch

For better or worse, I have focused my attention on the 1.8 branch.  As far as I can tell, most of your patch can be carried over to the branch without too much effort, except for:

> 4)  Eliminate the now-unused nsFormControlHelper::GetValueAttr

(GetValueAttr is still used in a couple of places in 1.8.)  I can post my branch version of the patch if anyone is interested, but I only expect this bug to be fixed on trunk anyway.

The strange thing is that combining this patch (as changed for 1.8) with my patch for bug 114997 does not fix the issue of button display that I described in bug 114997 comment 48.  The behavior of the "expanded testcase" that I posted there seems to be identical whether I apply the patch for this bug or not.  So is this patch actually calling nsHTMLInputElement::GetValue() (which I've modified in the other bug) or some other GetValue function?

As I've said, I know almost nothing about QI or anything like it.  But looking at this patch, it seems that the button is being cast to class nsIDOMHTMLInputElement.  Does that mean that it is treated as an nsHTMLInputElement in C++ code?  (The IDL file for nsIDOMHTMLInputElement doesn't list a GetValue() method.)

In any case, I'd like to be confident that this fix really is calling the function that we want it to be calling.  Can someone with a trunk build apply my patch for bug 114997 and see if the button display issue is gone?

Comment 6

[Boris Zbarsky [:bzbarsky]]

> For better or worse, I have focused my attention on the 1.8 branch. 

I kinda doubt that any of this code will be landing on the 1.8 branch...

> So is this patch actually calling nsHTMLInputElement::GetValue()

It is.

> Does that mean that it is treated as an nsHTMLInputElement in C++ code?

Well.  The button _is_ an nsHTMLInputElement.  QI to nsIDOMHTMLInputElement is only needed to call the right method on it.

> (The IDL file for nsIDOMHTMLInputElement doesn't list a GetValue() method.)

It lists a "value" property, which is translated into C++ as GetValue() and SetValue() methods.

I'll try applying your patch when I get home tonight.

Comment 7

[Boris Zbarsky [:bzbarsky]]

See bug 114997 comment 57 for testing result.

Comment 8

[Steuard Jensen]

Fixed on 1.8 branch by combined patch from bug 114997 comment 83 (checkin by bzbarsky, comment 86).