Bug 20226

Summary:

BORDER attribute with the IMG tag, using percentage values not working.

Product:

WebKit

Reporter:

deepak.sherveghar

Component:

Images

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ddkilzer, simon.fraser, tabatkins, webkit, webkit.review.bot

Priority:

Version:

525.x (Safari 3.1)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	none
Same patch without tabs	none
Patch	simon.fraser: review-
Simpler test case	none
Patch 2	simon.fraser: review+, simon.fraser: commit-queue-
Patch 3	none

deepak.sherveghar

Reported 2008-07-30 03:43:26 PDT

I have this simple HTML page. <HTML> <HEAD/> <BODY> <IMG SRC="abc.gif" border="0%" ALT="Alt Text"> </BODY> </HTML> Where width and height of "abc.gif" is 320x240. Expected result: A black border should surround the image, of a size that is the specified percentage of the available space, for <IMG> tag above. Actual result: No matter what the size of border specified in terms of percentage, we always see a SOLID border of width 3 pixels. Note that if we specify the border in terms of pixel's then everthing works fine. Also I have debugged with a webkit windows (cairo/CURL) build and found that the problem is with the cssParser. For boder=10% (anyvalue in terms of %) CSSParser::parseValue returns false. For border values specified in terms of pixels CSSParser::parseValue returns true and everything works fine.

Attachments
Patch (6.93 KB, patch) 2011-05-10 05:51 PDT, Mihnea Ovidenie	no flags	Details Formatted Diff Diff
Same patch without tabs (6.96 KB, patch) 2011-05-10 06:10 PDT, Mihnea Ovidenie	no flags	Details Formatted Diff Diff
Patch (6.45 KB, patch) 2011-05-10 07:06 PDT, Mihnea Ovidenie	simon.fraser: review-	Details Formatted Diff Diff
Simpler test case (33.37 KB, application/zip) 2011-07-18 10:37 PDT, Mihnea Ovidenie	no flags	Details
Patch 2 (7.40 KB, patch) 2011-07-22 09:17 PDT, Mihnea Ovidenie	simon.fraser: review+ simon.fraser: commit-queue-	Details Formatted Diff Diff
Patch 3 (6.85 KB, patch) 2011-07-22 11:49 PDT, Mihnea Ovidenie	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

deepak.sherveghar

Comment 1 2008-07-30 03:48:07 PDT

(In reply to comment #0) > I have this simple HTML page. > <HTML> > <HEAD/> > <BODY> > <IMG SRC="abc.gif" border="0%" ALT="Alt Text"> > </BODY> > </HTML> > > Where width and height of "abc.gif" is 320x240. > > Expected result: > A black border should surround the image, of a size that is the specified > percentage of the available space, for <IMG> tag above. > > Actual result: > No matter what the size of border specified in terms of percentage, we always > see a SOLID border of width 3 pixels. > > Note that if we specify the border in terms of pixel's then everthing works > fine. > > Also I have debugged with a webkit windows (cairo/CURL) build and found that > the problem is with the cssParser. > > For boder=10% (anyvalue in terms of %) CSSParser::parseValue returns false. > For border values specified in terms of pixels CSSParser::parseValue returns > true and everything works fine. > sorry for small typo ..... the above IMG tag should be <IMG SRC="abc.gif" border="10%" ALT="Alt Text"> with border as 10% and not 0%.

Dave Hyatt

Comment 2 2008-07-30 11:30:35 PDT

What do other browsers do? I suspect they drop the % and treat the number like a pixel value.

deepak.sherveghar

Comment 3 2008-07-30 11:57:52 PDT

Other Desktop Browser's like IE 7 and firefox 2.0 do display the expected behaviour (border around the image). Also I installed FIREBUG (a tool that allows to inspect, edit and monitor CSS, HTML and JavaScript live in any web page). I inspected the HTML code and found that it does seem to remove the % sign. AS all i can see in the HTML code is <IMG SRC="abc.gif" border="10" ALT="Alt Text"> No percent sign. I Inspected and edited the border's value in terms of pixel and percent e.g border=10 and border=10% Both the above values for border (10 and 10%) show border with same thickness in firefox.

Dave Hyatt

Comment 4 2008-07-30 11:58:45 PDT

So we just need to strip % from the string we pass to the CSS parser. Sounds easy enough.

deepak.sherveghar

Comment 5 2008-07-30 12:40:19 PDT

I tried removing the '%' sign from the string .. it does render the border properly now. But Dave one question. I just couldnt figure out why does our css parser cant deal with '%' unit properly ??? It does seem to work with all other units like pcs and pixels ..

Mihnea Ovidenie

Comment 6 2011-05-10 05:51:44 PDT

Created attachment 92941 [details] Patch

WebKit Review Bot

Comment 7 2011-05-10 05:56:02 PDT

Attachment 92941 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/ChangeLog:11: Line contains tab character. [whitespace/tab] [5] Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.

Mihnea Ovidenie

Comment 8 2011-05-10 06:10:18 PDT

Created attachment 92944 [details] Same patch without tabs Removed the tabs from the first patch.

Mihnea Ovidenie

Comment 9 2011-05-10 07:06:28 PDT

Created attachment 92949 [details] Patch Forgot to set up ? for review.

Simon Fraser (smfr)

Comment 10 2011-07-18 09:42:11 PDT

Comment on attachment 92949 [details] Patch Does this match other browsers, and the spec?

Simon Fraser (smfr)

Comment 11 2011-07-18 10:26:38 PDT

Normally invalid units cause the entire declaration to be dropped.

Mihnea Ovidenie

Comment 12 2011-07-18 10:37:03 PDT

Created attachment 101171 [details] Simpler test case Attached a simple test case containing a html and a png.

Tab Atkins

Comment 13 2011-07-18 10:41:36 PDT

The behavior of border="10%" should be to apply a 10px border. First, given that you are using a presentational attribute, we use HTML's rules for parsing it. The 'border' attribute is defined as "mapping to a pixel length property" <http://www.whatwg.org/specs/web-apps/current-work/complete/rendering.html#maps-to-the-pixel-length-property>, which means we first try to parse it with the "rules for parsing non-negative integers" <http://www.whatwg.org/specs/web-apps/current-work/complete/common-microsyntaxes.html#rules-for-parsing-non-negative-integers>. When you do that, it turns the value "10", which is then interpreted as the CSS length 10px. Note that if you were to use "border-width: 10%" in actual CSS, it would have no effect, because the border-width property is defined as taking "<length> | thin | medium | thick" in both CSS 2.1 and the current ED of the Backgrounds & Borders module. Thus, a declaration like that would be invalid and be ignored. Since this is an actual bug in our parser, I've marked this bug as confirmed.

Tab Atkins

Comment 14 2011-07-18 10:44:40 PDT

(In reply to comment #8) > Created an attachment (id=92944) [details] > Same patch without tabs > > Removed the tabs from the first patch. I'm not yet a reviewer, so I can't r- the patch, but I'd like to. While this solves the problem at hand, it would be better to implement the algorithm as the spec specifies, where you simply ignore whitespace and '+' on the front of the number, then grab as many contiguous digits as possible and interpret that as a length. Special-casing '%' here is incorrect.

Mihnea Ovidenie

Comment 15 2011-07-22 09:17:51 PDT

Created attachment 101737 [details] Patch 2 Second version of patch.

Tab Atkins

Comment 16 2011-07-22 10:34:12 PDT

(In reply to comment #15) > Created an attachment (id=101737) [details] > Patch 2 > > Second version of patch. I'm not yet experience enough to comment on the overall style, but I'm happy with the direction of this patch. Yay!

Simon Fraser (smfr)

Comment 17 2011-07-22 11:02:43 PDT

Comment on attachment 101737 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=101737&action=review > Source/WebCore/html/HTMLImageElement.cpp:101 > +int HTMLImageElement::parseBorderWidthAttribute(Attribute* attr) Does this need to be a class method? I don't see it using any members.

Mihnea Ovidenie

Comment 18 2011-07-22 11:49:39 PDT

Created attachment 101748 [details] Patch 3 The method to parse the border with attribute is made static as it does not touch any of the HTMLImageElement members. Also, i changed its return type to unsigned int.

WebKit Review Bot

Comment 19 2011-07-22 13:28:09 PDT

Comment on attachment 101748 [details] Patch 3 Clearing flags on attachment: 101748 Committed r91601: <http://trac.webkit.org/changeset/91601>

WebKit Review Bot

Comment 20 2011-07-22 13:28:14 PDT

All reviewed patches have been landed. Closing bug.

Mihnea Ovidenie

Comment 21 2011-09-02 01:07:40 PDT

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

Note You need to log in before you can comment on or make changes to this bug.