Bug 20226 - BORDER attribute with the IMG tag, using percentage values not working.
Summary: BORDER attribute with the IMG tag, using percentage values not working.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 525.x (Safari 3.1)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 48594 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-07-30 03:43 PDT by deepak.sherveghar
Modified: 2011-09-02 01:07 PDT (History)
5 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description deepak.sherveghar 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.
Comment 1 deepak.sherveghar 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%.
Comment 2 Dave Hyatt 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.
Comment 3 deepak.sherveghar 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.

Comment 4 Dave Hyatt 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.
Comment 5 deepak.sherveghar 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 ..
Comment 6 Mihnea Ovidenie 2011-05-10 05:51:44 PDT
Created attachment 92941 [details]
Patch
Comment 7 WebKit Review Bot 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.
Comment 8 Mihnea Ovidenie 2011-05-10 06:10:18 PDT
Created attachment 92944 [details]
Same patch without tabs

Removed the tabs from the first patch.
Comment 9 Mihnea Ovidenie 2011-05-10 07:06:28 PDT
Created attachment 92949 [details]
Patch

Forgot to set up ? for review.
Comment 10 Simon Fraser (smfr) 2011-07-18 09:42:11 PDT
Comment on attachment 92949 [details]
Patch

Does this match other browsers, and the spec?
Comment 11 Simon Fraser (smfr) 2011-07-18 10:26:38 PDT
Normally invalid units cause the entire declaration to be dropped.
Comment 12 Mihnea Ovidenie 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.
Comment 13 Tab Atkins 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.
Comment 14 Tab Atkins 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.
Comment 15 Mihnea Ovidenie 2011-07-22 09:17:51 PDT
Created attachment 101737 [details]
Patch 2

Second version of patch.
Comment 16 Tab Atkins 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!
Comment 17 Simon Fraser (smfr) 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.
Comment 18 Mihnea Ovidenie 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.
Comment 19 WebKit Review Bot 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>
Comment 20 WebKit Review Bot 2011-07-22 13:28:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Mihnea Ovidenie 2011-09-02 01:07:40 PDT
*** Bug 48594 has been marked as a duplicate of this bug. ***