Bug 9185 – REGRESSION: UserID field appears with an incorrect height on americanexpresslogin page

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Scott Park]

Overview: 
 - The UserID field on "https://www99.americanexpress.com/myca/logon/us/en/en_US/logon/LogLogon.jsp?DestPage=https%3A%2F%2Fwww99.americanexpress.com%2Fmyca%2Facctsumm%2Fus%2Faction%3Frequest_type%3Dauthreg_acctAccountSummary%26entry_point%3Dlnk_homepage%26aexp_nav%3Dsc_checkbill" does not have the "visibly" correct height.

Steps to Reproduce:
 - Visit the above URL

Actual Results:
 - User ID form field shows up with a very small height, 3 or 4px. This does not occur under the release version of Safari.
 
Expected Results:
 - The User ID form field shows up with the same height as the Password field.

Date and Build:
 - Sun May 28 20:09 GMT 2006 / WebKit-SVN-r14619

Additional Comments:
 - I fully recognize that this could be related to the markup, but the release version of webkit in Safari doesn't do this.

[Scott Park]

Created attachment 8621 [details]
Screen shot of the issue

[Scott Park]

Created attachment 8622 [details]
Awful source of the problem page

[Joost de Valk (AlthA)]

This needs a quick check, but sounds like a regression, and i have a vague feeling that this is a dupe too. ddkilzer oculd you do this quick check for us? :)

[Scott Park]

Still happens under latest nightly: r14643 - May 31st....

[David Kilzer (:ddkilzer)]

Confirming bug on locally built WebKit r14643.  Works fine with Safari 2.0.3 (417.9.3) on Mac OS X 10.4.6 (8I127/PowerPC).  Adding Regression keyword and setting priority to P1.

[Darin Adler]

I'm sure this is a "new text field" bug.

[Alice Liu]

<rdar://problem/4575273>

[David Carson]

Created attachment 8878 [details]
reduced test case.

It seems that the browser is honoring the height attribute, but I can can't find anywhere that there a height attribute is valid for an input element.

[David Carson]

> It seems that the browser is honoring the height attribute, but I can can't
> find anywhere that there a height attribute is valid for an input element.

Actually, found height is valid for an input when it is an image. But that seems to be the only time.

[David Carson]

Created attachment 8880 [details]
Tests all types of <INPUT> elements

It seems that there are a number of INPUT elements that are broken.

[David Carson]

Created attachment 8905 [details]
patch, test case and change log

[Darin Adler]

Comment on attachment 8905 [details]
patch, test case and change log

Seems to me the two if statements should be combined so that they share everything except the getAttribute check.

Also, the type of the exception code parameter should be ExceptionCode rather than int.

Finally, is removing the attribute really the right thing to do here for the old fix or for this new one? I'd think it would be better to ignore the attribute when computing height -- actually removing the attribute from the DOM seems clearly wrong.

[Maciej Stachowiak]

Agreed with darin. This is parallel to the width behavior, but buggy in at least two ways:

1) The attribute will be gone from the DOM - pretty sure other browsers don't do that.
2) Processed only at attach time, so it won't do anything about a height attribute (or width attribute) added  after the fact when the element has already been rendered, which seems wrong.

I could make test cases to show these two problems if that would help.

I think the right way to fix this is to make the if statement cases for widthAttr and heightAttr check for inputType() like the src and alt attrs do, and to also check for already-set width and height attrs in setInputType, so they can be added if type is image or hidden, or their decls can be removed if type used to be image or hidden.

Please consider taking this approach instead.

[Darin Adler]

(In reply to comment #13)
> I think the right way to fix this is to make the if statement cases for
> widthAttr and heightAttr check for inputType() like the src and alt attrs do,
> and to also check for already-set width and height attrs in setInputType, so
> they can be added if type is image or hidden, or their decls can be removed if
> type used to be image or hidden.

So the fix would be to:

    1) remove width logic from HTMLInputElement::attach
    2) change HTMLInputElement::parseMappedAttribute width and height cases to do the work only for the correct input types, and
    3) change HTMLInputElement::setInputType as Maciej describes above

And to test all three we'd need tests that check that the width and height attributes are "unmolested" and that the width and height is correctly set (or not set) including in cases where the type attribute is changed after the element is already on the page.

[David Carson]

This will teach me for listening to Hyatt. The solution that is being proposed was what was originally being developed. After some discussions with Hyatt, we moved the logic to attach() method.
The case that does not seem to be addressed is when JS is used to change the type from input type=text to type=image. 

[Darin Adler]

(In reply to comment #15)
> The case that does not seem to be addressed is when JS is used to change the
> type from input type=text to type=image. 

The setInputType function will be called in that case. That's what item (3) is talking about above.

[David Carson]

Created attachment 9016 [details]
patch, test cases and change logs

Made changes as suggested my Darin and mjs.
Supports changing the type dynamically to and from IMAGE and the height and width information is not lost.
Three test cases created.

[David Carson]

Created attachment 9017 [details]
Patch, test cases and change logs

Same as previous comment, but removed the code that was commented out.

[Darin Adler]

Comment on attachment 9017 [details]
Patch, test cases and change logs

Looks good.

I think an HTMLInputElement member function called respectHeightAndWidthAttrs() or something along those lines would make the code more readable:

    if ((attrName == widthAttr && respectHeightAndWidthAttrs()) ||
        (attrName == heightAttr && respectHeightAndWidthAttrs()) ||

        if (respectHeightAndWidthAttrs())
            addCSSLength(attr, CSS_PROP_WIDTH, attr->value());

Among other things, the definition function will give you a great opportunity to comment about why this is needed, something that's missing in the current patch. A comment that says something along the lines of "Width and height attributes must only work for certain types of input elements." would help, although I suggest we omit mention of "dumb web sites".

The rest of my comments are about the HTMLInputElement::setInputType() function.

I don't think needRecalcStyle is a great name for a local variable here, because the code is specifically about height and width attributes, not about style in general, and there's no general style recalculation. I would have written something more like this:

    bool didRespectHeightAndWidth = respectHeightAndWidthAttrs();
    bool willRespectHeightAndWidth = newType == IMAGE || newType == HIDDEN;

It's even possible you could set willRespectHeightAndWidth *after* m_type is changed, the way willStoreValue and willMaintainState work, in which case it would be:

    bool willRespectHeightAndWidth = respectHeightAndWidthAttrs();

With these booleans, the first if would be:

    if (didRespectHeightAndWidth && !willRespectHeightAndWidth)

And the later if would be:

    if (!didRespectHeightAndWidth && willRespectHeightAndWidth)

And you would not need a needRecalcStyle boolean.

To help make lines like this one more readable:

+                MappedAttribute* heightMapAttr = static_cast<MappedAttribute*>(mapAttr->getAttributeItem(heightAttr));

I would add a new function to NamedMappedAttrMap: an override of getAttributeItem like the one for attributeItem that's already there, which is an inline function that casts the type of the result.

I think this section of code may need a call to setChanged():

+                    heightMapAttr->setDecl(0);
+                    mapAttr->declRemoved();

The code in attributeChanged calls setChanged() and I can't see why that wouldn't be needed here too. I also think that this code should be in a member function in the StyledElement class. Otherwise, this new code would be the first call to declRemoved "out in the derived classes", which doesn't seem great to me, since the whole decl counting machinery seems a little fragile. Perhaps we could add a function named StyledElement::attributeRemoved. Or maybe we can just call attributeChanged here, a little later in the function once the type has been changed, so the code in HTMLInputElement::mapToEntry and HTMLInputElement::parseMappedAttribute can do its thing. Given the rest of the code, it looks like that would work well. Given what's here it looks like that's possibly something you tried already, perhaps? Was there a problem?

I'm going to say review- just because of the setChanged() issue. Otherwise this looks great!

[David Carson]

Created attachment 9035 [details]
patch, test cases and change logs

Took in the great comments from Darin. Patch is now a little smaller and cleaner.

[Darin Adler]

Comment on attachment 9035 [details]
patch, test cases and change logs

r=me

[Darin Adler]

Working on landing this now.

[Darin Adler]

Committed revision 15044.