Bug 15823 - getPropertyValue for border returns null, should compute the shorthand value
: getPropertyValue for border returns null, should compute the shorthand value
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: CSS
: 523.x (Safari 3)
: All All
: P3 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-11-03 18:27 PDT by Timothy Hatcher
Modified: 2009-01-07 02:15 PST (History)
7 users (show)

See Also:


Attachments
First attempt (3.97 KB, patch)
2008-05-03 09:13 PDT, Rob Buis
ddkilzer: review-
Details | Formatted Diff | Diff
Improved patch (7.00 KB, patch)
2008-05-31 08:47 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Updated layout test (save to LayoutTests/fast/css/) (3.31 KB, text/plain)
2008-06-02 13:56 PDT, David Kilzer (:ddkilzer)
darin: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Hatcher 2007-11-03 18:27:14 PDT
getPropertyValue for border returns null when a computed shorthand value should be returned. This is visible in the Inspector.
Comment 1 Rob Buis 2008-05-03 09:13:25 PDT
Created attachment 20947 [details]
First attempt

This patch fixes getPropertyValue("border"). I think it is okay to make another
code path for border as it is the only shorthand property AFAIK that consists of only other shorthand properties.
Cheers,

Rob.
Comment 2 David Kilzer (:ddkilzer) 2008-05-28 06:42:45 PDT
Comment on attachment 20947 [details]
First attempt

What happens when you try to get the "border" property of a div like this?

  <div id="border" style="border-left: 5px solid red; border-top: 5px solid green; border-right: 5px solid blue; border-bottom: 5px solid purple; " ></div>

I think it returns "5px 5px 5px 5px solid solid solid solid green blue purple red", which is not a legal value for the "border" property.

Does the CSS spec say what should happen in the case where a single "border" property can't accurately represent an element's border?

I tried the above test case in Firefox 2.0.0.x, and it returns the value for "border-top" when asking for the "border" property!  Not sure what MSIE 6 or 7 do.

r- since this change doesn't return a value valid in the negative case.  (We should at least have a "negative" test case as well.)
Comment 3 David Kilzer (:ddkilzer) 2008-05-28 06:43:47 PDT
(In reply to comment #2)
> r- since this change doesn't return a value valid in the negative case.  (We
> should at least have a "negative" test case as well.)

Or a "valid value".  :)

Comment 4 Rob Buis 2008-05-30 13:32:08 PDT
Hi David,

(In reply to comment #2)
> (From update of attachment 20947 [details] [edit])
> What happens when you try to get the "border" property of a div like this?
> 
>   <div id="border" style="border-left: 5px solid red; border-top: 5px solid
> green; border-right: 5px solid blue; border-bottom: 5px solid purple; " ></div>
> 
> I think it returns "5px 5px 5px 5px solid solid solid solid green blue purple
> red", which is not a legal value for the "border" property.

Correct, good find.

> Does the CSS spec say what should happen in the case where a single "border"
> property can't accurately represent an element's border?

Will look into that.

> I tried the above test case in Firefox 2.0.0.x, and it returns the value for
> "border-top" when asking for the "border" property!  Not sure what MSIE 6 or 7
> do.

Opera does "5px solid".
 
> r- since this change doesn't return a value valid in the negative case.  (We
> should at least have a "negative" test case as well.)

Ok, will work on that. Thanks for the review!
Cheers,

Rob.
Comment 5 Rob Buis 2008-05-31 08:47:35 PDT
Created attachment 21451 [details]
Improved patch

The testcase mentioned by David is now included, as well as one where border is set and then overridden. The behaviour with patch matches that of Opera.
Cheers,

Rob.
Comment 6 Darin Adler 2008-06-01 12:35:49 PDT
Comment on attachment 21451 [details]
Improved patch

+            for (int i = 0; i < nrprops;++i) {

Missing space after second colon.

Looks fine. r=me
Comment 7 Rob Buis 2008-06-01 23:45:57 PDT
Landed in r34292.
Comment 8 David Kilzer (:ddkilzer) 2008-06-02 11:15:49 PDT
Rob, what happens in this case?

<div id="border4" style="border-left: 4px solid green; border-top: 5px solid green; border-right: 5px solid green; border-botton: 5px solid green;" ></div>

I think that's going to produce this value, which would not be correct:

"   solid green"
Comment 9 David Kilzer (:ddkilzer) 2008-06-02 11:22:20 PDT
Comment on attachment 21451 [details]
Improved patch

Clearing darin's r+ since this has landed and I reopened the bug.
Comment 10 David Kilzer (:ddkilzer) 2008-06-02 11:57:58 PDT
(In reply to comment #8)
> Rob, what happens in this case?
> 
> <div id="border4" style="border-left: 4px solid green; border-top: 5px solid
> green; border-right: 5px solid green; border-botton: 5px solid green;" ></div>
> 
> I think that's going to produce this value, which would not be correct:
> 
> "   solid green"

Actually, that's "solid green", and apparently that's supported!
Comment 11 Rob Buis 2008-06-02 12:18:37 PDT
Hi David,

(In reply to comment #10)
> (In reply to comment #8)
> > Rob, what happens in this case?
> > 
> > <div id="border4" style="border-left: 4px solid green; border-top: 5px solid
> > green; border-right: 5px solid green; border-botton: 5px solid green;" ></div>

It is border-botton above. Even the spelling indicates that ;)

> > I think that's going to produce this value, which would not be correct:
> > 
> > "   solid green"
> 
> Actually, that's "solid green", and apparently that's supported!

I must admit I did not test that before landing, but now I did indeed it
returns "solid green", like Opera. I guess the report can be reclosed now?
Cheers,

Rob.
Comment 12 David Kilzer (:ddkilzer) 2008-06-02 13:07:12 PDT
(In reply to comment #11)
> I must admit I did not test that before landing, but now I did indeed it
> returns "solid green", like Opera. I guess the report can be reclosed now?

Thanks, Rob

I'm going to update the layout test to cover all permutations of mismatched border values.
Comment 13 David Kilzer (:ddkilzer) 2008-06-02 13:56:50 PDT
Created attachment 21467 [details]
Updated layout test (save to LayoutTests/fast/css/)

(In reply to comment #12)
> I'm going to update the layout test to cover all permutations of mismatched
> border values.

New layout test produces this results with ToT WebKit:

PASS div1.style.getPropertyValue("border") is '5px solid green'
PASS div2.style.getPropertyValue("border") is '5px solid'
FAIL div3.style.getPropertyValue("border") should be 5px. Was 5px green.
PASS div4.style.getPropertyValue("border") is 'solid green'
FAIL div5.style.getPropertyValue("border") should be null (of type object). Was green (of type string).
PASS div6.style.getPropertyValue("border") is '5px'
PASS div7.style.getPropertyValue("border") is 'solid'
PASS div8.style.getPropertyValue("border") is null
PASS div9.style.getPropertyValue("border") is null
PASS successfullyParsed is true

Note that the following list of values are acceptable for the "border" style:

border-width
border-width border-style
border-width border-style border-color
border-style
border-style border-color

While the following are not valid (in ToT WebKit, Opera 9.27 and Firefox 2.0.0.x):

border-color
border-width border-color

Should we return "null" in the above two cases?  Opera 9.27 returns the two invalid values, even thought they can't be used in a new declaration.
Comment 14 David Kilzer (:ddkilzer) 2008-06-21 12:10:36 PDT
Comment on attachment 21467 [details]
Updated layout test (save to LayoutTests/fast/css/)

Marking for review to get feedback.
Comment 15 Darin Adler 2008-06-23 10:22:55 PDT
Comment on attachment 21467 [details]
Updated layout test (save to LayoutTests/fast/css/)

rs=me
Comment 16 David Kilzer (:ddkilzer) 2008-06-23 13:35:06 PDT
Copying other interested parties.
Comment 17 David Kilzer (:ddkilzer) 2008-06-23 13:54:01 PDT
Committed updated test:

$ svn commit LayoutTests
Sending        LayoutTests/ChangeLog
Sending        LayoutTests/fast/css/getPropertyValue-border-expected.txt
Sending        LayoutTests/fast/css/getPropertyValue-border.html
Transmitting file data ...
Committed revision 34745.
Comment 18 Robert Blaut 2009-01-07 02:15:45 PST
*** Bug 23159 has been marked as a duplicate of this bug. ***