Bug 18703 – changing the 'size' property on a text input does not affect its length

• Component: Forms
• Status: RESOLVED FIXED

[Eric Roman]

Clicking on the maps tab causes the input field to overflow onto next line.

[Eric Roman]

Created attachment 20779 [details]
Try to set the "size" property on an input field -- has no effect in safari

In FF/Opera/IE, this works. However in Safari it does not. Reproduced on winxp Safari 3.1 and nightly r32413

[Glenn Wilson]

Created attachment 22803 [details]
Possible fix to bug 18703

This patch should fix bug 18703.  Input fields did not repaint themselves when their "size" attributes were changed.  When a text input field's size was changed, nothing happened on Safari, but the field resized on FF/IE.

This fix tells the html element to recalculate and repaint itself if it is a text field element and has been resized.

Also included is a layout test (not sure I put it in the right directory, though.)

[Mark Rowe (bdash)]

Your patch appears to use tabs for indentation.  The WebKit code base uses spaces for indentation, and the SVN repository will not allow us to commit any files containing tabs.  Can you please update the patch to not use tabs?

[Eric Seidel (no email)]

Comment on attachment 22803 [details]
Possible fix to bug 18703

r- for using tabs and forgetting the newline at the end of the file.

However, I think this fix is also not quite right.  I think the correct call might be setChanged(true).  I don't think we generally call repaint() directly.  But Hyatt or Beth (who hack in this code more often than I) should comment.

[Glenn Wilson]

Created attachment 22922 [details]
Improved possible fix for bug 18703

Oops -- sorry about that.  Although I could blame my IDE, not remembering to use spaces instead of tabs is probably my own fault.

Here is an improved potential patch.  Tabs have been replaced with spaces, and a newline has been added to the end of the layout test.

I tried using setChanged() instead of requesting the renderer repaint the element, but it didn't seem to have any effect.  Also using setValueMatchesRenderer(false) doesn't have any effect either.  What's the right way to let the renderer know to do this?

[Glenn Wilson]

Created attachment 22923 [details]
Improved possible fix for bug 18703

Forgot the tabs in the JS in the layout test...

[Darin Adler]

Comment on attachment 22923 [details]
Improved possible fix for bug 18703

This is the wrong place to make a change. The setSize function is a helper function that calls setAttribute, but the same bug can happen if you call setAttribute directly; you can do that from JavaScript. Just change the line in your test from:

    document.getElementById('success').size=30;

to:

    document.getElementById('success').setAttribute("size", 30);

The code that always runs when sizeAttr is changed is in HTMLInputElement::parseMappedAttribute, so if we need to do more work, it would be there.

I think a direct call to setNeedsLayoutAndPrefWidthsRecalc on the renderer is not correct. It may be as simple as calling setChanged. We should look at other attributes that don't work via CSS but can affect the layout to find the right pattern. I wasn't able to think of one right away -- maybe Hyatt can.

[Glenn Wilson]

Created attachment 24610 [details]
Possible patch to bug 18703

Here is an improved patch for this issue; I think this addresses Darin's points.

Instead of telling the renderer that the element is dirty in setSize of HTMLInputElement, instead, this patch adds a small bit of logic to Element's setAttribute method.  HTMLInputElement's setSize and parseMappedAttributes functions both invoke Element.setAttribute, so calling either will show the input resizing.

Additionally, I updated the layout test to verify that setting the size and setAttribute("size") will update the size.

The other 'invalidation' methods (setChanged and attributeChanged, specifically) did not seem to incur a later re-layout, so outside of defining a new method, I had to call setNeedsLayoutAndPrefWidthsRecalc.  I think this logic is correct: resizing an element that already had a size could have dramatic effects on how a page is sized and layed out, right?

This logic was added to both setAttribute methods in dom/Element.  I'm not sure why these two methods are not refactored, since they look strikingly similar, but I don't know enough about their differences to refactor them here.

[Dave Hyatt]

Comment on attachment 24610 [details]
Possible patch to bug 18703

Adding checks for derived class HTML-specific attributes in Element.cpp is wrong.

[Dave Hyatt]

Comment on attachment 22923 [details]
Improved possible fix for bug 18703

This is a 1-line fix. setChanged is just missing from inside parseMappedAttribute.

else if (attr->name() == sizeAttr) {
        m_size = !attr->isNull() ? attr->value().toInt() : 20;
    }

Throw a setNeedsLayoutAndPrefWidthsRecalc() in there and you're done.

[Dave Hyatt]

(Not setChanged, sorry meant setNeedsLayoutAndPrefWidthsDirty.

[Glenn Wilson]

Created attachment 24621 [details]
Possible patch to bug 18703.

Ok, here's the modified patch with the suggested changes....it is probably much simpler this way, so thanks for the feedback :)

I added the call that you specified, but I also had to add a check whether the renderer was valid first.  I debated checking the type of the input element too, but decided against it since non-text form input elements could also have sizes that could be changed (the password type comes to mind.)  

I originally spotted 'idAttr' in Element, and thought size was also a valid DOM attribute that could be tested, but I didn't realize it is HTML-specific.  Sorry about that.

Thanks for the review!

[Sam Weinig]

Comment on attachment 24621 [details]
Possible patch to bug 18703.

Looks good. r=me with some comments.

> +        Reviewed by NOBODY (OOPS!).
> +
> +        Fixed bug 18703.  Text fields would not repaint themselves
> +        after having their "size" attributes modified.  This modification 
> +        tells the object to recalculate its width and repaint itself when 
> +        its "size" attribute is parsed.

It is helpful to put the URL and title of the bug you are fixing in the ChangeLog

> +        if(renderer())
Space required after the if.

> [phone]  Glenn Wilson  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Added a new text to verify that the width of text input 
> +        fields can be modified by javascript using the "size" attribute.
Again, a URL and title help to give context.

[Glenn Wilson]

Created attachment 24640 [details]
Improved patch for bug 18703

Here is an improved patch with better changelog comments and space after the 'if'.  The original problem URL (http://my.att.net) seems to have been modified since this was reported, so I'm not able to repro the problem on this site with Safari 3.1.

Thanks for the review!

[Darin Adler]

Comment on attachment 24640 [details]
Improved patch for bug 18703

I think the basics of this patch are great. It can land as-is.

There are two ways I think it could be improved.

The real architecture for something like this is to make the DOM element call updateFromElement() on the renderer at the right time. This is considered better than making a specific call to the renderer, because it's up to the renderer to know what decisions it makes about the appearance based on what attributes. So instead of a call to setNeedsLayoutAndPrefWidthsRecalc, we'd have a call to updateFromElement here. Then RenderTextControl::updateFromElement would get logic to notice changes in the size attribute (for HTMLInputElement) and the cols attribute (for HTMLTextAreaElement) to determine whether that will change the size. This is better encapsulation. We want the invalidation based on size to go into RenderText control because RenderTextControl::calcPrefWidths is the function that knows the algorithm for how it affects the size. Unfortunately, HTMLTextAreaElement already uses the same design you're using here, so I don't think this change makes things any worse.

So:

    1) Use updateFromElement and put the logic about exactly what the effect of this attribute it into RenderTextControl. And change HTMLTextAreaElement to call updateFromElement instead.

We want to make sure to to do recalc that's unnecessary; it's common for sites to have bugs where they repeatedly set an attribute, and we'd like to minimize the work we do in cases like that.

    2) Don't do any update if the size isn't changing. This can be done most elegantly inside a RenderTextControl::updateFromElement function by remembering the old size factor value from calcPrefWidths, but it can even be done in HTMLInputElement::parseMappedAttribute by checking the old m_size value and not calling anything if it's not changing.

The test would be even better if the regression test said what it was testing in the output.

Anyway, I would have loved to see the improvements above done, but the change seems fine exactly as-is.

r=me

[Dave Hyatt]

I really dislike the updateFromElement architecture actually, since you end up checking a bunch of attributes/properties when you already know the only one that changed.

[Glenn Wilson]

(In reply to comment #16)
> I really dislike the updateFromElement architecture actually, since you end up
> checking a bunch of attributes/properties when you already know the only one
> that changed.
> 

I'm looking to make these improvements to this patch, so I'm hoping to have an updated patch uploaded soon.

Does a updateSizeFromElement (or, generally, a updateXXXFromElement) make better sense, since it's specific to what has changed, but encapsulates the logic of how the renderer is updating?  Or will having a plethora of updateXXXFromElement methods open a can of worms?

[Sam Weinig]

I don't think this is the correct usage of the GoogleBug, which is really meant to be a bug in a high profile google web product and not a Chromium issue.

[Glenn Wilson]

(In reply to comment #15)
>     1) Use updateFromElement and put the logic about exactly what the effect of
> this attribute it into RenderTextControl. And change HTMLTextAreaElement to
> call updateFromElement instead.
> 
> We want to make sure to to do recalc that's unnecessary; it's common for sites
> to have bugs where they repeatedly set an attribute, and we'd like to minimize
> the work we do in cases like that.
> 
>     2) Don't do any update if the size isn't changing. This can be done most
> elegantly inside a RenderTextControl::updateFromElement function by remembering
> the old size factor value from calcPrefWidths, but it can even be done in
> HTMLInputElement::parseMappedAttribute by checking the old m_size value and not
> calling anything if it's not changing.

I've tried out some of the improvements above, and found two different possibilities for changing RenderTextControl that I'd like your opinions about.  Both seem to work, but they both have inefficiencies that I'm not ecstatic about....perhaps you can spot improvements?

1)  Adding size-change detection to HtmlInputElement and detection of size changes to updateFromElement, and if the sizes have changed, calling setNeedsLayoutAndPrefWidthsRecalc:

Added to RenderTextControl::updateFromElement:
    int old_min_width = m_minPrefWidth;
    int old_max_width = m_maxPrefWidth;
    calcPrefWidths();
    if (old_min_width != m_minPrefWidth &&
        old_max_width != m_maxPrefWidth)
        this->setNeedsLayoutAndPrefWidthsRecalc();

Added to HtmlInputElement::parseMappedAttribute:
        short old_size = m_size;
        m_size = !attr->isNull() ? attr->value().toInt() : 20;
        if (renderer() && m_size != old_size) {
            setValueMatchesRenderer(false);
            static_cast<RenderTextControl*>(renderer())->updateFromElement();
        }

I don't like this because it measures the size twice and could lead to two calls to calcPrefWidths(), which is not efficient.  Is there a way in RenderTextControl to detect size changes without calculating calcPrefWidths?

2)  Adding another method to RenderTextControl that is only called when widths change, which circumvents needing to detect size changes in RenderTextControl:

Added to RenderTextControl:

void RenderTextControl::updateSizeFromElement() 
{
    setNeedsLayoutAndPrefWidthsRecalc();
    updateFromElement();
}

Added to HtmlInputElement::parseMappedAttribute:
        short old_size = m_size;
        m_size = !attr->isNull() ? attr->value().toInt() : 20;
        if (renderer() && m_size != old_size) {
            setValueMatchesRenderer(false);
            static_cast<RenderTextControl*>(renderer())->updateSizeFromElement();
        }

I like the second option better, because it feels cleaner. However, adding another method to RenderTextControl to be called just when the object's size changes may not be ideal.  What can be done to improve this?

[Darin Fisher (:fishd, Google)]

http://trac.webkit.org/changeset/38722