Bug 20495 - paintBorder in RenderObject paint two times the corners
Summary: paintBorder in RenderObject paint two times the corners
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Minor
Assignee: Simon Fraser (smfr)
URL:
Keywords:
Depends on: 58761
Blocks:
  Show dependency treegraph
 
Reported: 2008-08-24 00:10 PDT by Mario Bensi
Modified: 2011-04-19 11:46 PDT (History)
3 users (show)

See Also:


Attachments
test to draw a border (383 bytes, text/html)
2008-08-24 00:21 PDT, Mario Bensi
no flags Details
fix paintBorder (2.28 KB, patch)
2008-08-24 00:23 PDT, Mario Bensi
mario.bensi: review-
Details | Formatted Diff | Diff
fix paintBorder and add test before borderPaint (2.53 KB, patch)
2008-08-25 04:15 PDT, Mario Bensi
no flags Details | Formatted Diff | Diff
fix paintBorder, add test and fix coding style (2.53 KB, patch)
2008-08-25 05:21 PDT, Mario Bensi
hyatt: review-
Details | Formatted Diff | Diff
fix paintBorder in RenderObject and remove the bug on acid2 (2.97 KB, patch)
2008-09-01 06:20 PDT, Mario Bensi
eric: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mario Bensi 2008-08-24 00:10:35 PDT
here a log of drawBorder coordinate in paintBorder :

x1 : 215 y1 : 474 x2 : 768 y2 : 478
x1 : 215 y1 : 533 x2 : 768 y2 : 537
x1 : 215 y1 : 474 x2 : 219 y2 : 537
x1 : 764 y1 : 474 x2 : 768 y2 : 537

the result is that : 

------------------------
|  |                |  |
------------------------
|  |                |  |
|  |                |  |
|  |                |  |
|  |                |  |
|  |                |  |
------------------------
|  |                |  |
------------------------

I think we should have that : 

------------------------
|                      |
------------------------
|  |                |  |
|  |                |  |
|  |                |  |
|  |                |  |
|  |                |  |
------------------------
|                      |
------------------------
Comment 1 Mario Bensi 2008-08-24 00:21:15 PDT
Created attachment 22957 [details]
test to draw a border
Comment 2 Mario Bensi 2008-08-24 00:23:50 PDT
Created attachment 22958 [details]
fix paintBorder

remove the border width when the paintBorder draw the left and rigth side.
Comment 3 Mario Bensi 2008-08-25 04:15:23 PDT
Created attachment 22977 [details]
fix paintBorder and add test before borderPaint
Comment 4 Mario Bensi 2008-08-25 05:21:55 PDT
Created attachment 22979 [details]
fix paintBorder, add test and fix coding style
Comment 5 Eric Seidel (no email) 2008-08-27 17:40:13 PDT
Hyatt, beth? any thoughts?  This looks sane to me.
Comment 6 Dave Hyatt 2008-08-27 18:26:32 PDT
Comment on attachment 22979 [details]
fix paintBorder, add test and fix coding style

I think this needs test cases.  You are in effect choosing one border style to "win" at the corner.  We need to test this to see how we match up.

This is very important for tests like Acid 2.
Comment 7 Dave Hyatt 2008-08-27 18:26:58 PDT
We need to compare with IE and Firefox.

Comment 8 Mario Bensi 2008-09-01 06:20:58 PDT
Created attachment 23099 [details]
fix paintBorder in RenderObject and remove the bug on acid2

I use the old code when the rect is not available and this fix the problem on acid2 and on the text border you have not the border paint two time.
Comment 9 Eric Seidel (no email) 2008-09-02 03:14:50 PDT
Comment on attachment 23099 [details]
fix paintBorder in RenderObject and remove the bug on acid2

I can't evaluate the correctness of this patch, but I can say that the copy-paste code used here is a bad idea.

The if should be used to store the necessary variables, and then a single drawBorder call should be used.

It appears that you're changing where it's drawing from/to.  It seems the code right above this adjusts y and y2 in an if, why can't this change also do the same? (use an if to adjust y and y2?  or copy y and y2 into some other nicely named variables and ajust them there).  Copy/paste of code which is confusing to begin with is a bad idea.  Ideally this method would be cleaned up to use FloatPoint and possibly to take fewer parameters :)
Comment 10 Simon Fraser (smfr) 2011-04-19 11:46:23 PDT
http://trac.webkit.org/changeset/84273