width/height attributes of input element should be supported when the type of the input element is image.

[Dongwoo Joshua Im (dwim)]

Regarding the HTML5 specification, width and height attributes of input element should be supported when the type of the input element is image.

First of all, IDL of input element has width and height attributes (http://www.w3.org/TR/html5/the-input-element.html#the-input-element) in it.
And, there is a description about width and height attributes on input element when their type attribute is in the Image Button state (http://www.w3.org/TR/html5/the-map-element.html#attr-dim-width).

And in my opinion, if the type of input element should be respected the height and width attributes 
(if the result of "m_inputType->shouldRespectHeightAndWidthAttributes()" is true), such as image type, 
width and height attributes should be accessible by JavaScript API.


So, I propose to add width and height attributes into input element.


I'll upload patch, so please review the patch and give some comments.

Thanks.

[Dongwoo Joshua Im (dwim)]

Created attachment 111382 [details]
Patch

[Dongwoo Joshua Im (dwim)]

Created attachment 111383 [details]
Patch

[Vineet Chaudhary (vineetc)]

View in context: https://bugs.webkit.org/attachment.cgi?id=111383&action=review

I am not reviewer, but here are my observation:

> Source/WebCore/html/HTMLInputElement.idl:42
> +        attribute long height;

After looking at spec http://www.whatwg.org/specs/web-apps/current-work/multipage/the-map-element.html#dom-dim-width 
it says "The attributes, if specified, must have values that are valid non-negative integers." 
so can we have unsigned long height instead of long height.

As It says "For iframe, embed, and object the IDL attributes are DOMString; for video the IDL attributes are unsigned long."

Also referring to spec http://www.whatwg.org/specs/web-apps/current-work/multipage/the-input-element.html#htmlinputelement height and with are mentioned as "attribute DOMString height"

Please correct me if I misunderstood.

[Dongwoo Joshua Im (dwim)]

Deer Vineet Chaudhary.

Thanks for your feedback.

As you mentioned,
width and height attributes are DOMString type in WHATWG and W3C spec.
So, DOMString is correct type for those two attributes.

I'll correct the patch.

[Dongwoo Joshua Im (dwim)]

Created attachment 111397 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 111397 [details]
Patch

Attachment 111397 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10124150

[Early Warning System Bot]

Comment on attachment 111397 [details]
Patch

Attachment 111397 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10122157

[WebKit Review Bot]

Comment on attachment 111397 [details]
Patch

Attachment 111397 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10126149

[Dongwoo Joshua Im (dwim)]

Created attachment 111404 [details]
Patch

[Dongwoo Joshua Im (dwim)]

Created attachment 111405 [details]
Patch

I re-upload patch, because previously uploaded file is wrong.

[Alexey Proskuryakov]

Comment on attachment 111405 [details]
Patch

Please add additional tests:

- setting value to a string that's not a valid non-negative integer (perhaps "5.5pt");
- setting value to a string representing a negative number specifically;
- changing width and height of non-image input elements (spec says that these attributes "do not apply" then, but what exactly does that mean?);
- how setting the value affects attributes (check node.getAttribute("width") in addition to node.width).

Please verify that these tests have matching results in WebKit an in other browsers implementing this.

Why this is not a [Reflect] attribute?

r- for test coverage. I don't know enough about this code to confidently review, but I suspect that this should use [Reflect].

[Dongwoo Joshua Im (dwim)]

(In reply to comment #11)
> (From update of attachment 111405 [details])
> Please add additional tests:
> 
> - setting value to a string that's not a valid non-negative integer (perhaps "5.5pt");

I will add this kind of test case.

> - setting value to a string representing a negative number specifically;

I will add this kind of test case.

> - changing width and height of non-image input elements (spec says that these attributes "do not apply" then, but what exactly does that mean?);

I will add this kind of test case.
In setter function, setAttribute function only be called when the type of input element take care of width and height. (the result of shouldRespectHeightAndWidthAttributes() is true)

> - how setting the value affects attributes (check node.getAttribute("width") in addition to node.width).

I will add this kind of test case.

> 
> Please verify that these tests have matching results in WebKit an in other browsers implementing this.

I will try.

> 
> Why this is not a [Reflect] attribute?

Regarding w3c spec (http://www.w3.org/TR/html5/the-map-element.html#attr-dim-width), width and height IDL attributes on the iframe, embed, object, and video elements must reflect the respective content attributes of the same name. 
But it does not say the width and height attributes of image and input element are reflect. And current implementation of the width and height attributes of image element are not [Reflect] attributes. 
And, some type of input element will not take care of the width and height attributes.
So, I think the width and height attributes of input element is not a [Reflect] attribute.

> 
> r- for test coverage. I don't know enough about this code to confidently review, but I suspect that this should use [Reflect].

[Dongwoo Joshua Im (dwim)]

Dear Alexey.

Regarding your comments, 
- setting value to a string that's not a valid non-negative integer (perhaps "5.5pt");
- setting value to a string representing a negative number specifically;

I tested the "iframe" and "image" element.

The width and height attributes of iframe element are DOMString.
The width and height attributes of image element are long.

And, the width and height attributes of input element are DOMString.
So, I think the result of the test of input element should be same as iframe element. 
Am I correct?


*** When I set '5.5px' as width, 
When node is iframe, 
   node.width is '5.5px'.
   node.getAttribute("width") is '5.5px'.

When node is image, 
   node.width is 0.
   node.getAttribute("width") is '0'.



*** When I set '1.2' as width, 
When node is iframe,
   node.width is '1.2'.
   node.getAttribute("width") is '1.2'.

*** And, when I set 1.2 as width, 
When node is image, 
   node.width is 1.
   node.getAttribute("width") is '1'.



*** When I set '-10' as width, 
When node is iframe,
   node.width is '-10'.
   node.getAttribute("width") is '-10'.

When node is image, 
   node.width is 0.
   node.getAttribute("width") is '-10'.

[Alexey Proskuryakov]

As mentioned in above comment, I don't know enough about this code to confidently review. Another reviewer will need to work with you.

Please add the tests, and see how they work in IE and Firefox. If neither IE nor Firefox match the spec, then perhaps the spec needs to be fixed.

[Kent Tamura]

(In reply to comment #12)
> > - changing width and height of non-image input elements (spec says that these attributes "do not apply" then, but what exactly does that mean?);
> 
> I will add this kind of test case.
> In setter function, setAttribute function only be called when the type of input element take care of width and height. (the result of shouldRespectHeightAndWidthAttributes() is true)

I'd like to know the behavior of other browsers for this point.

[Dongwoo Joshua Im (dwim)]

Created attachment 112072 [details]
Test page

I've tested this on FireFox 4.0.1, IE 8, and Opera 11.50.
If you have another browser, you can run this test page on that.

It seems that FireFox and Opera don't support this attribute yet.

IE 8 does partially support it.
It seems that IE 8 only works when the value of width is positive "number", such as 100, or "100".
It does not work when the value of width is "100px", or "-100".

I will try this on IE 9 soon.


And, you can see that the width and height are not changed by this way, if the type of input element is text.


I will revise the layout test, and upload patch again.

[Dongwoo Joshua Im (dwim)]

Created attachment 112075 [details]
Patch

Layout test is revised.

[Kent Tamura]

(In reply to comment #16)
> I've tested this on FireFox 4.0.1, IE 8, and Opera 11.50.
> If you have another browser, you can run this test page on that.
> 
> It seems that FireFox and Opera don't support this attribute yet.
> 
> IE 8 does partially support it.
> It seems that IE 8 only works when the value of width is positive "number", such as 100, or "100".
> It does not work when the value of width is "100px", or "-100".

IMO, we should not implement width and height now.
 - The standard is incomplete.
  The standard doesn't define behavior of width/height for non-image types.
 - The existing IE implementation isn't compliant.
  I confirmed IE's input.width and height were numbers, not strings.

[Kent Tamura]

Comment on attachment 112075 [details]
Patch

r- because the patch can't be applied.

[Kent Tamura]

The specification was updated.

--------

          attribute unsigned long height;
          attribute unsigned long width;

--------
The IDL attributes width and height must return the rendered width and height of the image, in CSS pixels, if an image is being rendered, and is being rendered to a visual medium; or else the intrinsic width and height of the image, in CSS pixels, if an image is available but not being rendered to a visual medium; or else 0, if no image is available. When the input element's type attribute is not in the Image Button state, then no image is available. [CSS]
--------

[Dongwoo Joshua Im (dwim)]

(In reply to comment #20)
> The specification was updated.
> 
> --------
> 
>           attribute unsigned long height;
>           attribute unsigned long width;
> 
> --------
> The IDL attributes width and height must return the rendered width and height of the image, in CSS pixels, if an image is being rendered, and is being rendered to a visual medium; or else the intrinsic width and height of the image, in CSS pixels, if an image is available but not being rendered to a visual medium; or else 0, if no image is available. When the input element's type attribute is not in the Image Button state, then no image is available. [CSS]
> --------

Thanks to inform this.


There is one more line below the paragraph which you quote.

"On setting, they must act as if they reflected the respective content attributes of the same name."

IMO, Even though the line says "reflect", I can't use [Reflect] for those two attributes, because width and height is valuable when the type is image.

I think I need to handle those attributes regarding the type of the input element.

Am I right?


I'll upload new patch with width and height attributes which are "unsigned long" type.

[Kent Tamura]

(In reply to comment #21)
> IMO, Even though the line says "reflect", I can't use [Reflect] for those two attributes, because width and height is valuable when the type is image.
> 
> I think I need to handle those attributes regarding the type of the input element.
> 
> Am I right?

Right.

[Dongwoo Joshua Im (dwim)]

Created attachment 113445 [details]
Patch

I changed the type of width and height attributes as "unsigned long".

[Dongwoo Joshua Im (dwim)]

Created attachment 113447 [details]
Patch

patch with binary file.

[Kent Tamura]

Comment on attachment 113447 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113447&action=review

> LayoutTests/ChangeLog:14
> +        * fast/forms/resources/samsung.gif: Added.

Are you sure that this image can be distributed in WebKit's license?

> LayoutTests/fast/forms/input-width-height-attributes.html:4
> +    <link rel="stylesheet" href="../../fast/js/resources/js-test-style.css">

This line is not needed.

> LayoutTests/fast/forms/input-width-height-attributes.html:42
> +    <div id="console"></div>

This line is not needed. js-test-pre.js automatically creates it.

> LayoutTests/fast/forms/input-width-height-attributes.html:45
> +    <script>
> +        description('width and height attributes of HTMLInputElement.<br>');

nit: Indenting all of the content is meaningless.

> LayoutTests/fast/forms/input-width-height-attributes.html:50
> +        div1.innerHTML = "<b>case #1.</b> Image type. --- HTML inline setting as \"160\", \"80px\","
> +                          + " result is " + image1.width + ", " + image1.height + "<br>" + div1.innerHTML;

We had better show "PASS" or "FAIL" in order to tell what is expected to other developers.

> Source/WebCore/html/HTMLInputElement.cpp:1017
> +int HTMLInputElement::height() const

Why is this "int"?

> Source/WebCore/html/HTMLInputElement.cpp:1022
> +    if (m_inputType->shouldRespectHeightAndWidthAttributes())

We prefer early exit. So, this function should be:

{
    if (!m_inputType->shouldRespectHeightAndWidthAttributes())
        return 0;
    :
    :

> Source/WebCore/html/HTMLInputElement.cpp:1023
> +        height = fastGetAttribute(heightAttr).toInt(&ok);

We had better use parseHTMLNonNegativeInteger().

Also, this implementation is incorrect.  The specification says height/width returns the intrinsic size of the image.  Please refer to the implementation of HTMLImageElement::width() and height().

[Kent Tamura]

(In reply to comment #25)
> The specification says height/width returns the intrinsic size of the image. 
... if height/width attribute is missing.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #25)
> (From update of attachment 113447 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113447&action=review
> 
> > LayoutTests/ChangeLog:14
> > +        * fast/forms/resources/samsung.gif: Added.
> 
> Are you sure that this image can be distributed in WebKit's license?
 
I've noticed that the CI images of Apple and Google are already included in the resource directory for the similar reasons (e.g., layout test purpose). Have you guys had any trouble with that (e.g., any sort of copyright violation)?

[Tony Chang]

(In reply to comment #27)
> (In reply to comment #25)
> > (From update of attachment 113447 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=113447&action=review
> > 
> > > LayoutTests/ChangeLog:14
> > > +        * fast/forms/resources/samsung.gif: Added.
> > 
> > Are you sure that this image can be distributed in WebKit's license?
> 
> I've noticed that the CI images of Apple and Google are already included in the resource directory for the similar reasons (e.g., layout test purpose). Have you guys had any trouble with that (e.g., any sort of copyright violation)?

I suspect the apple and google logos were checked in by accident.  Seems like we can replace them with other images for these tests.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #25)
> (From update of attachment 113447 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113447&action=review

> > Source/WebCore/html/HTMLInputElement.cpp:1017
> > +int HTMLInputElement::height() const
> 
> Why is this "int"?

I need to use getAttribute(widthAttr) to get the width.
The return value is string type, so I need to change the type.

As I know, toInt, toDouble, and toFloat are supported by wtf/text.
There are no 'toLong', or 'toUnsignedLong' functions.

That is the reason why the return type of width() function is int.


As you recommended, I refer the implementation of the HTMLImageElement.

The width attribute is 'long' type in idl file,
but, in the header file, the return type of width() function is 'int.'
And, the argument of setWidth() function is also 'int'.

In the HTMLInputElement, size attribute is 'unsigned long' type in idl file,
the return type of size() function in header file is 'int', as well.


IMHO, using 'int' as return type of width() and height() is reasonable.


But if you worried about it, 
I will use 'unsigned' as return type by casting the result of 'toInt'.

Please let me know your opinion.

Thanks.

[Kent Tamura]

Comment on attachment 113447 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113447&action=review

>>> Source/WebCore/html/HTMLInputElement.cpp:1017
>>> +int HTMLInputElement::height() const
>> 
>> Why is this "int"?
> 
> I need to use getAttribute(widthAttr) to get the width.
> The return value is string type, so I need to change the type.
> 
> As I know, toInt, toDouble, and toFloat are supported by wtf/text.
> There are no 'toLong', or 'toUnsignedLong' functions.
> 
> That is the reason why the return type of width() function is int.
> 
> 
> As you recommended, I refer the implementation of the HTMLImageElement.
> 
> The width attribute is 'long' type in idl file,
> but, in the header file, the return type of width() function is 'int.'
> And, the argument of setWidth() function is also 'int'.
> 
> In the HTMLInputElement, size attribute is 'unsigned long' type in idl file,
> the return type of size() function in header file is 'int', as well.
> 
> 
> IMHO, using 'int' as return type of width() and height() is reasonable.
> 
> 
> But if you worried about it, 
> I will use 'unsigned' as return type by casting the result of 'toInt'.
> 
> Please let me know your opinion.
> 
> Thanks.

"unsigned long" of WebIDL is 32-bit unsigned integer, and we have parseHTMLNonNegativeInteger() and String::toUInt().

Please change it to "unsigned" unless you have any technical difficulties.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #30)
> (From update of attachment 113447 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113447&action=review
> 
> "unsigned long" of WebIDL is 32-bit unsigned integer, and we have parseHTMLNonNegativeInteger() and String::toUInt().
> 
> Please change it to "unsigned" unless you have any technical difficulties.

I'm trying to use unsigned rather than int.

Then, prototypes are defined like below.

unsigned height(bool ignorePendingStylesheets = false) const;
unsigned width(bool ignorePendingStylesheets = false) const;
void setHeight(unsigned);
void setWidth(unsigned);

When I've tried to give a negative number to setWidth() and get width by width(),
some big positive number comes out.

So, which one is better choice?
 1. prototype : void setHeight(int);
    In the function, if the parameter is less than 0, just set 0 rather than the parameter.

 2.  prototype : void setHeight(unsigned);
    In the function, casting the parameter as int and check if it is less than 0 or not, and set 0 if it is less than 0.

[Kent Tamura]

(In reply to comment #31)
> When I've tried to give a negative number to setWidth() and get width by width(),
> some big positive number comes out.

The behavior of setting a negative value to a "unsigned long" property is defined by Web IDL specification.  If our binding code doesn't follow it, we should fix out binding code generator.

http://www.w3.org/TR/WebIDL/#es-unsigned-long

[Dongwoo Joshua Im (dwim)]

(In reply to comment #32)
> (In reply to comment #31)
> > When I've tried to give a negative number to setWidth() and get width by width(),
> > some big positive number comes out.
> 
> The behavior of setting a negative value to a "unsigned long" property is defined by Web IDL specification.  If our binding code doesn't follow it, we should fix out binding code generator.
> 
> http://www.w3.org/TR/WebIDL/#es-unsigned-long


Then, we need to make another bug to take care of this exceptional case.

I think that bug doesn't need to block this bug, because that is not only for this bug but for general cases.



IMHO, we need to fix toUInt32 function to handle this exceptional case.

In JSHTMLInputElement.cpp file, setWidth() function calls impl's setWidth() using toUInt32().

  imp->setWidth(value.toUInt32(exec));


And, in JSValueInlineMethods.h file, toUInt32() function is define like this.

  inline uint32_t JSValue::toUInt32(ExecState* exec) const
  {
      // See comment on JSC::toUInt32, above.                                                           
      return toInt32(exec);
  }

[Dongwoo Joshua Im (dwim)]

Created attachment 117162 [details]
Patch

[Kent Tamura]

Comment on attachment 117162 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117162&action=review

> LayoutTests/fast/forms/input-width-height-attributes.html:11
> +<style>
> +    div {
> +        border-style:solid;
> +        border-width:1px;
> +        width: 600px;
> +    }
> +</style>

Indenting whole the content makes no sense.  Please write it like:
<style>
div {
    border-stylid: solid;
    border-width: 1px;
    width: 600px;
}
</style>

BTW, this style looks not needed.

> LayoutTests/fast/forms/input-width-height-attributes.html:37
> +<script>
> +    description('width and height attributes of HTMLInputElement.<br>');

Indenting whole the content makes no sense.

<br> is not needed.

> LayoutTests/fast/forms/input-width-height-attributes.html:42
> +    div1.innerHTML = "<b>case #1.</b> Image type. --- HTML inline setting as \"160\", \"80px\","
> +                      + " result is " + image1.width + ", " + image1.height + "<br>" + div1.innerHTML;

You should print it by debug().

> LayoutTests/fast/forms/input-width-height-attributes.html:70
> +</script>

Test coverage is not good.  We should test height/image properties with non-image types.

> Source/WebCore/html/HTMLInputElement.cpp:1022
> +    if (!m_inputType->isImageButton())
> +        return 0;
> +

This type check is not needed. We can just call InputType::height().

> Source/WebCore/html/HTMLInputElement.h:138
> +    unsigned height(bool ignorePendingStylesheets = false) const;
> +    unsigned width(bool ignorePendingStylesheets = false) const;

There are no call sites of ignorePendingStylesheets=true.  We should remove the ignorePendingStylesheets parameter.

The ignorePendingStylesheets parameter of HTMLImageElement::width() and height() was introduced by http://trac.webkit.org/changeset/7625. But it seems the parameter is not used now.

> Source/WebCore/html/ImageInputType.cpp:187
> +        // check the attribute first for an explicit pixel valuea

A comment should be like a normal English sentences. It should start with a capital letter, and should end with '.'

> Source/WebCore/html/ImageInputType.h:69
> +    virtual unsigned height(bool) const;
> +    virtual unsigned width(bool) const;
> +    virtual void setHeight(unsigned);
> +    virtual void setWidth(unsigned);

We had better append OVERRIDE keyword to them.

> Source/WebCore/html/InputType.cpp:747
> +void InputType::setHeight(unsigned height)
> +{
> +}
> +
> +void InputType::setWidth(unsigned width)
> +{
> +}

The specification says:
> On setting, they must act as if they reflected the respective content attributes of the same name.

and doesn't say setting depends on input type.  I think we should set the corresponding attribute value here.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #35)
> (From update of attachment 117162 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117162&action=review

> Test coverage is not good.  We should test height/image properties with non-image types.
> 
I will add some more test cases which try to set/get width and height of textbox or something.

> > Source/WebCore/html/HTMLInputElement.cpp:1022
> > +    if (!m_inputType->isImageButton())
> > +        return 0;
> > +
> 
> This type check is not needed. We can just call InputType::height().
> 
I think this kind of early return is preferred in WebKit.
I will remove this two lines.


> > Source/WebCore/html/HTMLInputElement.h:138
> > +    unsigned height(bool ignorePendingStylesheets = false) const;
> > +    unsigned width(bool ignorePendingStylesheets = false) const;
> 
> There are no call sites of ignorePendingStylesheets=true.  We should remove the ignorePendingStylesheets parameter.
> 
> The ignorePendingStylesheets parameter of HTMLImageElement::width() and height() was introduced by http://trac.webkit.org/changeset/7625. But it seems the parameter is not used now.
> 
Oh. I see.

> > Source/WebCore/html/ImageInputType.cpp:187
> > +        // check the attribute first for an explicit pixel valuea
> 
> A comment should be like a normal English sentences. It should start with a capital letter, and should end with '.'
> 
I didn't check the comments carefully.
I will re-write comments.

> > Source/WebCore/html/ImageInputType.h:69
> > +    virtual unsigned height(bool) const;
> > +    virtual unsigned width(bool) const;
> > +    virtual void setHeight(unsigned);
> > +    virtual void setWidth(unsigned);
> 
> We had better append OVERRIDE keyword to them.
> 
Yes!

[Dongwoo Joshua Im (dwim)]

Created attachment 117571 [details]
Patch

I've add some more test cases, and fix some codes regarding Kent's comments.

[Kent Tamura]

Comment on attachment 117571 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117571&action=review

> LayoutTests/fast/forms/input-width-height-attributes.html:54
> +var div1 = document.getElementById("div1");

The variable div1 - div8 are not needed.

> LayoutTests/fast/forms/input-width-height-attributes.html:86
> +text1.width = 100;
> +text1.height = 50;
> +debug('Test case #5 : Text, Setting by JavaScript API as \"100\", \"50\"');
> +shouldBe('text1.width,text1.height', '0,0');
> +

After this, we had better test:
 - text1 has width= and height= HTML attributes.
 - Changing the type to image, width/height values are not changed.

> Source/WebCore/html/InputType.cpp:753
> +void InputType::setHeight(unsigned height)
> +{
> +    element()->setAttribute(heightAttr, String::number(height));
> +}
> +
> +void InputType::setWidth(unsigned width)
> +{
> +    element()->setAttribute(widthAttr, String::number(width));
> +}
> +

No other *InputType overrides setHeight() and setWidth().  We can remove them and move the code to HTMLInputElement::setHeight()/setWidth().

[Dongwoo Joshua Im (dwim)]

(In reply to comment #38)
> (From update of attachment 117571 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117571&action=review
> 
> > LayoutTests/fast/forms/input-width-height-attributes.html:54
> > +var div1 = document.getElementById("div1");
> 
> The variable div1 - div8 are not needed.
> 
Yes. we can remove div2 ~ div8.
We can remove image2 ~ image4, as well.

> > LayoutTests/fast/forms/input-width-height-attributes.html:86
> > +text1.width = 100;
> > +text1.height = 50;
> > +debug('Test case #5 : Text, Setting by JavaScript API as \"100\", \"50\"');
> > +shouldBe('text1.width,text1.height', '0,0');
> > +
> 
> After this, we had better test:
>  - text1 has width= and height= HTML attributes.
>  - Changing the type to image, width/height values are not changed.
> 
I will try.

> > Source/WebCore/html/InputType.cpp:753
> > +void InputType::setHeight(unsigned height)
> > +{
> > +    element()->setAttribute(heightAttr, String::number(height));
> > +}
> > +
> > +void InputType::setWidth(unsigned width)
> > +{
> > +    element()->setAttribute(widthAttr, String::number(width));
> > +}
> > +
> 
> No other *InputType overrides setHeight() and setWidth().  We can remove them and move the code to HTMLInputElement::setHeight()/setWidth().

Yes, you are right!

[Dongwoo Joshua Im (dwim)]

(In reply to comment #39)
> (In reply to comment #38)
> > (From update of attachment 117571 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=117571&action=review
> > 
> > > LayoutTests/fast/forms/input-width-height-attributes.html:54
> > > +var div1 = document.getElementById("div1");
> > 
> > The variable div1 - div8 are not needed.
> > 
> Yes. we can remove div2 ~ div8.
> We can remove image2 ~ image4, as well.
> 
I will remain the image2 ~ image4. That looks better.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #39)
> (In reply to comment #38)
> > (From update of attachment 117571 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=117571&action=review
 
> > > LayoutTests/fast/forms/input-width-height-attributes.html:86
> > > +text1.width = 100;
> > > +text1.height = 50;
> > > +debug('Test case #5 : Text, Setting by JavaScript API as \"100\", \"50\"');
> > > +shouldBe('text1.width,text1.height', '0,0');
> > > +
> > 
> > After this, we had better test:
> >  - text1 has width= and height= HTML attributes.
> >  - Changing the type to image, width/height values are not changed.
> > 
> I will try.
> 


IMO, if we want that width is 0 when we change text1 as image,
I think we should call setAttribute() when m_inputType is image.

Isn't it?

[Dongwoo Joshua Im (dwim)]

Created attachment 117586 [details]
Patch

[Dongwoo Joshua Im (dwim)]

Created attachment 117590 [details]
Patch

Previous patch has some problem.

[Dongwoo Joshua Im (dwim)]

Regarding the HTML5 spec,

1. Width and height attributes of input element are setted whatever the type of element is.
"On setting, they must act as if they reflected the respective content attributes of the same name."

2. When user want to get the width and height, it returns 0 if the type of element is not image.
"or else 0, if no image is available. When the input element's type attribute is not in the Image Button state, then no image is available."


I've tried to implement the code and test cases regarding the HTML5 spec.

Please let me know if I miss something.

Thanks!

[Darin Adler]

Comment on attachment 117590 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117590&action=review

Patch looks good. I am not sure that the test case covers all the cases that are different. From reading the height function we need:

    1) no renderer but with width attribute
    2) no renderer and no width attribute with a loaded image
    3) no renderer and no width attribute and no loaded image
    4) with renderer

And then for each of those, zoomed and not zoomed.

> Source/WebCore/html/ImageInputType.cpp:189
> +        if (parseHTMLNonNegativeInteger(element->getAttribute(heightAttr), height))

Should be fastGetAttribute here.

> Source/WebCore/html/ImageInputType.cpp:192
> +        // If the image is available, use its hieght.

Typo here. "hieght".

> Source/WebCore/html/ImageInputType.cpp:194
> +        if (m_imageLoader->image())
> +            return m_imageLoader->image()->imageSizeForRenderer(element->renderer(), 1.0f).height();

Normally we use just "1", not "1.0f" in cases like this.

> Source/WebCore/html/ImageInputType.cpp:210
> +        if (parseHTMLNonNegativeInteger(element->getAttribute(widthAttr), width))

Same comment about fastGetAttribute.

> Source/WebCore/html/InputType.h:266
> +    // Sets and Gets width and height of the button if it is image button.

The word "Gets" should not be capitalized.

I’m also not sure this comment is needed or helpful. Comments that say “why” are helpful. Comments that state information that could become out of date are not as good.

The comment refers to the input element as “the button”, but many input elements are not buttons.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #35)
> (From update of attachment 117162 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117162&action=review
> 
> > Source/WebCore/html/HTMLInputElement.h:138
> > +    unsigned height(bool ignorePendingStylesheets = false) const;
> > +    unsigned width(bool ignorePendingStylesheets = false) const;
> 
> There are no call sites of ignorePendingStylesheets=true.  We should remove the ignorePendingStylesheets parameter.
> 
> The ignorePendingStylesheets parameter of HTMLImageElement::width() and height() was introduced by http://trac.webkit.org/changeset/7625. But it seems the parameter is not used now.

If so, why don't we remove that from WebKit?

[Dongwoo Joshua Im (dwim)]

Created attachment 117848 [details]
Patch

Dear Darin, 
I've revised the patch regarding your comments, except the test case.

I don't fully understand how to make that kinds of test cases until now, 
so it will take much time.

If you are OK, I think I can make another patch for those things after this patch is landed.
Or, I will have some more time to make that kinds of test cases in this patch.

Please let me know your opinion.

Thanks!

[Dongwoo Joshua Im (dwim)]

Created attachment 118317 [details]
Patch

[Darin Adler]

(In reply to comment #46)
> (In reply to comment #35)
> > (From update of attachment 117162 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=117162&action=review
> > 
> > > Source/WebCore/html/HTMLInputElement.h:138
> > > +    unsigned height(bool ignorePendingStylesheets = false) const;
> > > +    unsigned width(bool ignorePendingStylesheets = false) const;
> > 
> > There are no call sites of ignorePendingStylesheets=true.  We should remove the ignorePendingStylesheets parameter.
> > 
> > The ignorePendingStylesheets parameter of HTMLImageElement::width() and height() was introduced by http://trac.webkit.org/changeset/7625. But it seems the parameter is not used now.
> 
> If so, why don't we remove that from WebKit?

Yes, we should remove it.

[Darin Adler]

Please take the time to write the tests. We have no reason to be in a hurry to land this change without sufficient tests.

[Adam Barth]

Comment on attachment 118317 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118317&action=review

> Source/WebCore/ChangeLog:6
> +        Reviewed by Darin Adler.

It sounds like Darin isn't happy with this patch due to some missing test cases.

[Eric Seidel (no email)]

Comment on attachment 117590 [details]
Patch

Cleared Darin Adler's review+ from obsolete attachment 117590 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Kent Tamura]

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

[Kent Tamura]

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

[Dongwoo Joshua Im (dwim)]

Created attachment 132977 [details]
Test case - no renderer but with width attribute

Dear Darin Adler,
I create a test case which you requested.

Attached test case can cover the first one of your list below.
4th one is already included in the previous patch.

    1) no renderer but with width attribute
    2) no renderer and no width attribute with a loaded image
    3) no renderer and no width attribute and no loaded image
    4) with renderer


I have trouble with the 2nd and 3rd.
* How can I test "no width"? Does it mean, don't set a value to the width/height attribute?
* I think there might be no differences between loaded/not-loaded image case when we just access to the width/height attribute. What is your expectation?

[Dongwoo Joshua Im (dwim)]

Created attachment 135481 [details]
Patch

[Dongwoo Joshua Im (dwim)]

Created attachment 135482 [details]
Patch

[Kent Tamura]

(In reply to comment #55)
> Created an attachment (id=132977) [details]
> Test case - no renderer but with width attribute
> 
> Dear Darin Adler,
> I create a test case which you requested.
> 
> Attached test case can cover the first one of your list below.
> 4th one is already included in the previous patch.
> 
>     1) no renderer but with width attribute
>     2) no renderer and no width attribute with a loaded image
>     3) no renderer and no width attribute and no loaded image
>     4) with renderer
> 
> 
> I have trouble with the 2nd and 3rd.
> * How can I test "no width"? Does it mean, don't set a value to the width/height attribute?

It means just an image input without the width attribute.

> * I think there might be no differences between loaded/not-loaded image case when we just access to the width/height attribute. What is your expectation?

Your code has different code paths for loaded case and not-loaded case.

[Kent Tamura]

Comment on attachment 135482 [details]
Patch

r- because tests are not enough yet.

[Ryosuke Niwa]

Comment on attachment 135482 [details]
Patch

cq- given r-.

[Dongwoo Joshua Im (dwim)]

Created attachment 139966 [details]
Patch

I add more tests.

[Kent Tamura]

Comment on attachment 139966 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139966&action=review

> LayoutTests/fast/forms/input-width-height-attributes-without-renderer-loaded-image-expected.txt:5
> +TEST COMPLETE
> +PASS ('width' in e) is true

'TEST COMPLETE,' then 'PASS' looks confusing.  Please follow my comments below.

> LayoutTests/fast/forms/input-width-height-attributes-without-renderer-loaded-image.html:9
> +if (window.layoutTestController) {
> +    layoutTestController.waitUntilDone();
> +    layoutTestController.dumpAsText();
> +}

Replace these lines with:
  jsTestIsAsync = true;

> LayoutTests/fast/forms/input-width-height-attributes-without-renderer-loaded-image.html:16
> +function log(text)
> +{
> +    var console = document.getElementById('console');
> +    console.appendChild(document.createTextNode(text));
> +    console.appendChild(document.createElement('br'));
> +}

Remove this function.  js-test-pre.js provides debug() function.

> LayoutTests/fast/forms/input-width-height-attributes-without-renderer-loaded-image.html:20
> +<input type="image" id="imageSlow" src="resources/image-slow.pl">

This element has a renderer.  You need to specify style="display:none;"

> LayoutTests/fast/forms/input-width-height-attributes-without-renderer-loaded-image.html:28
> +            shouldBeTrue("('width' in e)");
> +            shouldBeTrue("('height' in e)");

They are meaningless because HTMLInputElement always has width/height properties.
You should verify what values should width/height be.

> LayoutTests/fast/forms/input-width-height-attributes-without-renderer-loaded-image.html:29
> +            log("Success")

This message is meaningless. We can remove it.

> LayoutTests/fast/forms/input-width-height-attributes-without-renderer-loaded-image.html:30
> +            layoutTestController.notifyDone()

Replace this with:
  finishJSTest();

> LayoutTests/fast/forms/input-width-height-attributes-without-renderer-loaded-image.html:32
> +        } else if (readyState == 'loading' || readyState == 'interactive') {
> +            setTimeout("runTest()", 50);

Polling readyState is not efficient.  You should use events such as 'load' 'readystatechange'.

> LayoutTests/fast/forms/input-width-height-attributes-without-renderer-not-loaded-image.html:20
> +<input type="image" id="imageSlow" src="resources/image-slow.pl">

This has a renderer.

> LayoutTests/fast/forms/input-width-height-attributes-without-renderer-not-loaded-image.html:37
> +    function runTest() {
> +        readyState = document.readyState;
> +        if (readyState == 'complete') {
> +            log("Success")
> +            layoutTestController.notifyDone()
> +        } else if (readyState == 'loading' || readyState == 'interactive') {
> +            shouldBeTrue("('width' in e)");
> +            shouldBeTrue("('height' in e)");
> +
> +            setTimeout("runTest()", 50);
> +        }
> +    }
> +
> +    runTest();

I don't think this test needs to be asynchronous.

var e = document.getELementById("ImageSlow");
shouldBe(...)

is enough.

> LayoutTests/fast/forms/input-width-height-attributes-without-renderer.html:15
> +<input id="inputElement" type="image" width="50" height="50">

This element has a renderer.

> LayoutTests/fast/forms/input-width-height-attributes-without-renderer.html:16
> +No crash = test PASS

Why? 
We should verify what width/height should be.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #62)
> (From update of attachment 139966 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139966&action=review
> 
> > LayoutTests/fast/forms/input-width-height-attributes-without-renderer-loaded-image-expected.txt:5
> > +TEST COMPLETE
> > +PASS ('width' in e) is true
> 

Done.

> 'TEST COMPLETE,' then 'PASS' looks confusing.  Please follow my comments below.
> 
> > LayoutTests/fast/forms/input-width-height-attributes-without-renderer-loaded-image.html:9
> > +if (window.layoutTestController) {
> > +    layoutTestController.waitUntilDone();
> > +    layoutTestController.dumpAsText();
> > +}
> 
> Replace these lines with:
>   jsTestIsAsync = true;
> 

Done.

> > LayoutTests/fast/forms/input-width-height-attributes-without-renderer-loaded-image.html:16
> > +function log(text)
> > +{
> > +    var console = document.getElementById('console');
> > +    console.appendChild(document.createTextNode(text));
> > +    console.appendChild(document.createElement('br'));
> > +}
> 
> Remove this function.  js-test-pre.js provides debug() function.
> 

Done.

> > LayoutTests/fast/forms/input-width-height-attributes-without-renderer-loaded-image.html:20
> > +<input type="image" id="imageSlow" src="resources/image-slow.pl">
> 
> This element has a renderer.  You need to specify style="display:none;"
> 
> > LayoutTests/fast/forms/input-width-height-attributes-without-renderer-loaded-image.html:28
> > +            shouldBeTrue("('width' in e)");
> > +            shouldBeTrue("('height' in e)");
> 
> They are meaningless because HTMLInputElement always has width/height properties.
> You should verify what values should width/height be.
> 

width/width should be 0 because it is not setted.
I will use shouldBe.

> > LayoutTests/fast/forms/input-width-height-attributes-without-renderer-loaded-image.html:29
> > +            log("Success")
> 
> This message is meaningless. We can remove it.
> 
> > LayoutTests/fast/forms/input-width-height-attributes-without-renderer-loaded-image.html:30
> > +            layoutTestController.notifyDone()
> 
> Replace this with:
>   finishJSTest();
> 

Done.

> > LayoutTests/fast/forms/input-width-height-attributes-without-renderer-loaded-image.html:32
> > +        } else if (readyState == 'loading' || readyState == 'interactive') {
> > +            setTimeout("runTest()", 50);
> 
> Polling readyState is not efficient.  You should use events such as 'load' 'readystatechange'.
> 

I use 'document.onreadystatechange = function() {...}' rather than polling the state.

> > LayoutTests/fast/forms/input-width-height-attributes-without-renderer-not-loaded-image.html:20
> > +<input type="image" id="imageSlow" src="resources/image-slow.pl">
> 
> This has a renderer.
> 

Fixed.

> > LayoutTests/fast/forms/input-width-height-attributes-without-renderer-not-loaded-image.html:37
> > +    function runTest() {
> > +        readyState = document.readyState;
> > +        if (readyState == 'complete') {
> > +            log("Success")
> > +            layoutTestController.notifyDone()
> > +        } else if (readyState == 'loading' || readyState == 'interactive') {
> > +            shouldBeTrue("('width' in e)");
> > +            shouldBeTrue("('height' in e)");
> > +
> > +            setTimeout("runTest()", 50);
> > +        }
> > +    }
> > +
> > +    runTest();
> 
> I don't think this test needs to be asynchronous.
> 
> var e = document.getELementById("ImageSlow");
> shouldBe(...)
> 
> is enough.
> 

Fixed.

> > LayoutTests/fast/forms/input-width-height-attributes-without-renderer.html:15
> > +<input id="inputElement" type="image" width="50" height="50">
> 
> This element has a renderer.
> 
> > LayoutTests/fast/forms/input-width-height-attributes-without-renderer.html:16
> > +No crash = test PASS
> 
> Why? 
> We should verify what width/height should be.

Ok. I will use shouldBe here.

[Dongwoo Joshua Im (dwim)]

Created attachment 140485 [details]
Patch

[Kent Tamura]

Comment on attachment 140485 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140485&action=review

> LayoutTests/fast/forms/input-width-height-attributes-without-renderer-loaded-image.html:15
> +<input type="image" id="imageInput" src="resources/green.jpg" style="display:none">
> +<div id="console"></div>
> +<script language="JavaScript">
> +    var e = document.getElementById("imageInput");
> +    shouldBe('e.width', '0');
> +    shouldBe('e.height', '0');
> +    finishJSTest();

This is a test for *loaded* image.  So the test should be asynchronous and need to wait for image loading.

> LayoutTests/fast/forms/input-width-height-attributes-without-renderer-not-loaded-image.html:20
> +    var e = document.getElementById("imageSlow");
> +
> +    document.onreadystatechange = function() {
> +        readyState = document.readyState;
> +        if (readyState == 'complete') {
> +            finishJSTest();
> +        } else if (readyState == 'loading' || readyState == 'interactive') {
> +            shouldBe('e.width', '0');
> +            shouldBe('e.height', '0');

This is a test for *not loaded* image.  So we don't need to make this asynchronous.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #65)
> (From update of attachment 140485 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140485&action=review
> 
> > LayoutTests/fast/forms/input-width-height-attributes-without-renderer-loaded-image.html:15
> > +<input type="image" id="imageInput" src="resources/green.jpg" style="display:none">
> > +<div id="console"></div>
> > +<script language="JavaScript">
> > +    var e = document.getElementById("imageInput");
> > +    shouldBe('e.width', '0');
> > +    shouldBe('e.height', '0');
> > +    finishJSTest();
> 
> This is a test for *loaded* image.  So the test should be asynchronous and need to wait for image loading.
> 

Then, I will test 'shouldBe' if readyState == 'complete'.

> > LayoutTests/fast/forms/input-width-height-attributes-without-renderer-not-loaded-image.html:20
> > +    var e = document.getElementById("imageSlow");
> > +
> > +    document.onreadystatechange = function() {
> > +        readyState = document.readyState;
> > +        if (readyState == 'complete') {
> > +            finishJSTest();
> > +        } else if (readyState == 'loading' || readyState == 'interactive') {
> > +            shouldBe('e.width', '0');
> > +            shouldBe('e.height', '0');
> 
> This is a test for *not loaded* image.  So we don't need to make this asynchronous.

I did this because I want to make sure to test 'shoudBe' before readyState == 'complete'.

I will fix this what you recommended at the previous comment.

[Dongwoo Joshua Im (dwim)]

Created attachment 140493 [details]
Patch

[Kent Tamura]

Comment on attachment 140493 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140493&action=review

> LayoutTests/fast/forms/input-width-height-attributes-without-renderer-loaded-image.html:18
> +        if (readyState == 'complete') {
> +            shouldBe('e.width', '0');
> +            shouldBe('e.height', '0');

The code says:
     // If the image is available, use its width.
      if (m_imageLoader->image())
          return m_imageLoader->image()->imageSizeForRenderer(element->renderer(), 1).width();

e.width and e.height should not be 0, right?
You need to set display:none after completing the image loading.

[Dongwoo Joshua Im (dwim)]

Created attachment 140622 [details]
Patch

[Kent Tamura]

Comment on attachment 140622 [details]
Patch

Looks good.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #70)
> (From update of attachment 140622 [details])
> Looks good.

Thanks for the review!

This is my very first patch to WebCore, 
I think I can't finish this patch without your kind review!! ;)

[WebKit Review Bot]

Comment on attachment 140622 [details]
Patch

Clearing flags on attachment: 140622

Committed r116389: <http://trac.webkit.org/changeset/116389>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

<rdar://problem/11445434>

[Kent Tamura]

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