Summary: | width/height attributes of input element should be supported when the type of the input element is image. | ||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dongwoo Joshua Im (dwim) <dw.im> | ||||||||||||||||||||||||||||||||||||||||||||||
Component: | DOM | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | abarth, ap, code.vineet, darin, dglazkov, donggwan.kim, gyuyoung.kim, ian, info, ojan, s.choi, tkent, tony, webkit.review.bot, yosin | ||||||||||||||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Dongwoo Joshua Im (dwim)
2011-10-17 22:20:57 PDT
Created attachment 111382 [details]
Patch
Created attachment 111383 [details]
Patch
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. 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. Created attachment 111397 [details]
Patch
Comment on attachment 111397 [details] Patch Attachment 111397 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10124150 Comment on attachment 111397 [details] Patch Attachment 111397 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10122157 Comment on attachment 111397 [details] Patch Attachment 111397 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10126149 Created attachment 111404 [details]
Patch
Created attachment 111405 [details]
Patch
I re-upload patch, because previously uploaded file is wrong.
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].
(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]. 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'. 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. (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. 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.
Created attachment 112075 [details]
Patch
Layout test is revised.
(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. Comment on attachment 112075 [details]
Patch
r- because the patch can't be applied.
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] -------- (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. (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. Created attachment 113445 [details]
Patch
I changed the type of width and height attributes as "unsigned long".
Created attachment 113447 [details]
Patch
patch with binary file.
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(). (In reply to comment #25) > The specification says height/width returns the intrinsic size of the image. ... if height/width attribute is missing. (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)? (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. (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. 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. (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. (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 (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); } Created attachment 117162 [details]
Patch
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. (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! Created attachment 117571 [details]
Patch
I've add some more test cases, and fix some codes regarding Kent's comments.
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(). (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! (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. (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? Created attachment 117586 [details]
Patch
Created attachment 117590 [details]
Patch
Previous patch has some problem.
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! 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. (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? 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!
Created attachment 118317 [details]
Patch
(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. Please take the time to write the tests. We have no reason to be in a hurry to land this change without sufficient tests. 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. 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. *** Bug 80111 has been marked as a duplicate of this bug. *** *** Bug 80114 has been marked as a duplicate of this bug. *** 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?
Created attachment 135481 [details]
Patch
Created attachment 135482 [details]
Patch
(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. Comment on attachment 135482 [details]
Patch
r- because tests are not enough yet.
Comment on attachment 135482 [details]
Patch
cq- given r-.
Created attachment 139966 [details]
Patch
I add more tests.
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. (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. Created attachment 140485 [details]
Patch
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. (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. Created attachment 140493 [details]
Patch
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. Created attachment 140622 [details]
Patch
Comment on attachment 140622 [details]
Patch
Looks good.
(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!! ;) Comment on attachment 140622 [details] Patch Clearing flags on attachment: 140622 Committed r116389: <http://trac.webkit.org/changeset/116389> All reviewed patches have been landed. Closing bug. *** Bug 90818 has been marked as a duplicate of this bug. *** |