Bug 13709

Summary: Table border doesn't show up
Product: WebKit Reporter: David Scott <pcs0325>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: a.suchit, bdakin, bfulgham, buildbot, chenzx, commit-queue, ddkilzer, dglazkov, eae, esprehn+autocc, gyuyoung.kim, hyatt, jchaffraix, kling, koivisto, leviw, macpherson, menard, mitz, ojan.autocc, peter+ews, rakuco, rego+ews, rniwa, sstigler1985, suchit.agrawal, webkit-bug-importer, webkit.review.bot, xan.lopez
Priority: P2 Keywords: HasReduction, InRadar
Version: 420+   
Hardware: Mac   
OS: OS X 10.4   
URL: http://eee.uci.edu/07s/23720/weekly.html
Bug Depends on: 114310    
Bug Blocks:    
Attachments:
Description Flags
Original page source
none
Reduced test case
none
Reduced test case v2
none
Patch
none
Patch
none
Archive of layout-test-results from gce-cr-linux-03 for chromium-linux-x86_64
none
Patch
none
Archive of layout-test-results from gce-cr-linux-05 for chromium-linux-x86_64
none
Patch
none
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2
none
Patch
none
Patch
none
Patch
none
Patch
none
selection regression
none
Patch
none
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2
none
Patch
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 none

Description David Scott 2007-05-13 20:27:27 PDT
The table isn't displayed at this web page: http://eee.uci.edu/07s/23720/weekly.html

I'm using Shiira web browser which uses Webkit. Doesn't work in Shiira. I tried it in Safari, doesn't work there either. However, it does work in Firefox 2.0.

By looking at the source I can tell the page was generated by Microsoft Word, but I don't much beyond that.
Comment 1 David Kilzer (:ddkilzer) 2007-05-13 21:27:30 PDT
Confirmed with a local debug build of WebKit r21447 with Safari 2.0.4 (419.3) on Mac OS X 10.4.9 (8P135).

Comment 2 Dave Hyatt 2007-05-13 21:41:20 PDT
The border is using the "windowtext" system color.  I would guess we have some bug with that system color implementation.
Comment 3 David Scott 2007-05-15 21:27:07 PDT
That page I linked above is working right now, but I didn't update anything. I don't know if the instructor changed the way she saved the page.
Comment 4 Sam Stigler 2008-02-20 21:49:42 PST
Confirmed as still not working in r30377 on 10.5.2.
Comment 5 David Kilzer (:ddkilzer) 2008-04-27 16:51:19 PDT
Created attachment 20854 [details]
Original page source

This is an MS Word 9 document exported as HTML.
Comment 6 David Kilzer (:ddkilzer) 2008-04-27 17:22:06 PDT
Created attachment 20855 [details]
Reduced test case

The problem is that a border width of 0.5pt is apparently rounded down to zero.

Playing with the border value, I found that 0.743pt shows a border, while 0.742pt does not.

This had nothing to do with the 'windowtext' border color since a sufficiently large border width draws a border with 'windowtext' as the color.
Comment 7 David Kilzer (:ddkilzer) 2008-04-27 17:25:32 PDT
Created attachment 20856 [details]
Reduced test case v2

The previous reduced test case worked because I left the border width at 0.743pt.

With it set to .5pt, no border is drawn.  (Note that .9pt will work, so it's not misparsing without a leading zero.)
Comment 8 David Kilzer (:ddkilzer) 2009-07-05 09:13:54 PDT
Would switching to using floats to store CSS values fix this?
Comment 9 Suchit Agrawal 2013-03-28 09:53:31 PDT
I am working for this issue. I have patch for it.
Comment 10 Suchit Agrawal 2013-04-01 06:38:55 PDT
Created attachment 195965 [details]
Patch
Comment 11 WebKit Review Bot 2013-04-01 06:43:20 PDT
Comment on attachment 195965 [details]
Patch

Attachment 195965 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17330601
Comment 12 Early Warning System Bot 2013-04-01 06:43:43 PDT
Comment on attachment 195965 [details]
Patch

Attachment 195965 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17378165
Comment 13 Early Warning System Bot 2013-04-01 06:44:37 PDT
Comment on attachment 195965 [details]
Patch

Attachment 195965 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17357767
Comment 14 EFL EWS Bot 2013-04-01 06:45:06 PDT
Comment on attachment 195965 [details]
Patch

Attachment 195965 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17386133
Comment 15 Peter Beverloo (cr-android ews) 2013-04-01 06:48:41 PDT
Comment on attachment 195965 [details]
Patch

Attachment 195965 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/17371169
Comment 16 WebKit Review Bot 2013-04-01 07:03:17 PDT
Comment on attachment 195965 [details]
Patch

Attachment 195965 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17314639
Comment 17 Build Bot 2013-04-01 07:14:12 PDT
Comment on attachment 195965 [details]
Patch

Attachment 195965 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17339417
Comment 18 Build Bot 2013-04-01 07:31:35 PDT
Comment on attachment 195965 [details]
Patch

Attachment 195965 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17348451
Comment 19 Suchit Agrawal 2013-04-01 10:33:38 PDT
Created attachment 195988 [details]
Patch
Comment 20 Build Bot 2013-04-01 10:38:36 PDT
Comment on attachment 195988 [details]
Patch

Attachment 195988 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17326322
Comment 21 Build Bot 2013-04-01 11:15:01 PDT
Comment on attachment 195988 [details]
Patch

Attachment 195988 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17305489
Comment 22 Suchit Agrawal 2013-04-01 11:21:02 PDT
(In reply to comment #19)
> Created an attachment (id=195988) [details]
> Patch
> 

Border is properly displayed if border width is .8pt or more. But border does not display for below .8pt value.

When .7pt or lesser value is coverted to pixels, it's value comes less than 1px. When this value is set to border width(unsigned int), it becomes zero. However on other browsers, unless the border width is not explicitly set to zero, a minimum of 1px is used.

I fixed this issue with couple of overloaded functions. If the template is integer type than it rounds of the value to 1 when the computed value X is 0 < X < 1. If the template is float type than value returns as it is.
Comment 23 Julien Chaffraix 2013-04-01 11:28:32 PDT
Comment on attachment 195988 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195988&action=review

> Source/WebCore/ChangeLog:10
> +        Border width is currently of type unsigned. The pixel value of 0.7pt or less 

This was a decision made when switching to sub-pixel layout to keep the value unsigned (see http://trac.webkit.org/wiki/LayoutUnit). IIRC it was to ensure that borders were always of the author size.

> Source/WebCore/ChangeLog:22
> +        Any result which is less than 1, set to 1 in case of integer templates.
> +        Any result which is less than 1, returnedas it is in case of float templates.

I don't really understand these rules.

What are other browsers doing? Are we matching them?

Nothing is said in CSS 2.1 (or CSS 3 borders) about it, I think we should seek out clarification in this case or else we will not reach inter-operable behavior?

> Source/WebCore/css/StyleBuilder.cpp:649
> +    }

Do we really need all this copied and pasted code? (LayoutUnit does some similar operations but the core function is kept around).
Comment 24 Suchit Agrawal 2013-04-01 11:48:02 PDT
(In reply to comment #23)
> (From update of attachment 195988 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=195988&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        Border width is currently of type unsigned. The pixel value of 0.7pt or less 
> 
> This was a decision made when switching to sub-pixel layout to keep the value unsigned (see http://trac.webkit.org/wiki/LayoutUnit). IIRC it was to ensure that borders were always of the author size.
> 
> > Source/WebCore/ChangeLog:22
> > +        Any result which is less than 1, set to 1 in case of integer templates.
> > +        Any result which is less than 1, returnedas it is in case of float templates.
> 
> I don't really understand these rules.
> 
When the computed value of border width in pixels from pt units is less than 1px, border width becomes 0 due to rounding off double to unsigned. However the border width should not be set to zero unless explicitly specified by the author.

Also for cases where ApplyPropertyComputeLength ::applyValue() has to compute a floating type value, the computed result should not be rounded off as in the above case.

> What are other browsers doing? Are we matching them?
> 
> Nothing is said in CSS 2.1 (or CSS 3 borders) about it, I think we should seek out clarification in this case or else we will not reach inter-operable behavior?
> 
Varified the behavior on FF and IE8. Webkit matches both FF and IE with this patch applied.

> > Source/WebCore/css/StyleBuilder.cpp:649
> > +    }
> 
> Do we really need all this copied and pasted code? (LayoutUnit does some similar operations but the core function is kept around).
> 
I will check and get back to you on this.
Comment 25 WebKit Review Bot 2013-04-01 12:49:43 PDT
Comment on attachment 195988 [details]
Patch

Attachment 195988 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17392107

New failing tests:
svg/W3C-SVG-1.1/text-spacing-01-b.svg
fast/sub-pixel/zoomed-em-border.html
tables/mozilla_expected_failures/bugs/bug89315.html
css2.1/t1604-c542-letter-sp-00-b-a.html
svg/text/text-spacing-01-b.svg
css2.1/20110323/c541-word-sp-000.htm
Comment 26 WebKit Review Bot 2013-04-01 12:49:48 PDT
Created attachment 196008 [details]
Archive of layout-test-results from gce-cr-linux-03 for chromium-linux-x86_64

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03  Port: chromium-linux-x86_64  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
Comment 27 Suchit Agrawal 2013-04-02 06:49:55 PDT
Created attachment 196129 [details]
Patch
Comment 28 Suchit Agrawal 2013-04-02 07:07:24 PDT
(In reply to comment #24)
> (In reply to comment #23)
> > (From update of attachment 195988 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=195988&action=review

> > What are other browsers doing? Are we matching them?
> > 
> > Nothing is said in CSS 2.1 (or CSS 3 borders) about it, I think we should seek out clarification in this case or else we will not reach inter-operable behavior?
> > 
> Varified the behavior on FF and IE8. Webkit matches both FF and IE with this patch applied.
> 
> > > Source/WebCore/css/StyleBuilder.cpp:649
> > > +    }
> > 
> > Do we really need all this copied and pasted code? (LayoutUnit does some similar operations but the core function is kept around).
> > 
> I will check and get back to you on this.
> 
Used function template in place of many overloaded functions and function template specialization is used for handing special case 'unsigned'.
Comment 29 WebKit Review Bot 2013-04-02 07:41:08 PDT
Comment on attachment 196129 [details]
Patch

Attachment 196129 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17383205

New failing tests:
tables/mozilla_expected_failures/bugs/bug89315.html
Comment 30 WebKit Review Bot 2013-04-02 07:41:13 PDT
Created attachment 196136 [details]
Archive of layout-test-results from gce-cr-linux-05 for chromium-linux-x86_64

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: chromium-linux-x86_64  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
Comment 31 Beth Dakin 2013-04-03 17:24:25 PDT
It definitely seem unfortunate to define these templates in StyleBuilder.cpp when the existing templates for computeLength are in CSSPrimitiveValue. Ultimately, this is not a tables bug so much as it is a CSS bug. Maybe Antti or Andreas can take a look?
Comment 32 Antti Koivisto 2013-04-04 02:59:37 PDT
Comment on attachment 196129 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196129&action=review

> Source/WebCore/css/StyleBuilder.cpp:586
> +template <> unsigned computeLength<unsigned>(RenderStyle* style, RenderStyle* rootStyle, float zoom, CSSPrimitiveValue* primitiveValue)

So ApplyPropertyComputeLength<unsigned,... version is only used for border properties, making this special rounding behavior border specific. This is a very indirect and confusing way of getting border specific behavior.

I think it would be better to refactor this a bit. The current ApplyPropertyComputeLength could be replaced with several simpler versions

ApplyPropertyComputeBorderWidth (for border, outline and similar properties. this would have your new rounding behavior)
ApplyPropertyComputeTextSpacing (for word and letter-spacing, only case that has svgZoomEnabled)
ApplyPropertyComputeTransformOriginZ (this is the only float case and seems to want the rounding behavior only. maybe not even needed.)

Each of these can be much simpler and understandable than the current overly generic type. Most of the template parameters can be removed.
Comment 33 Suchit Agrawal 2013-04-04 09:56:53 PDT
Created attachment 196486 [details]
Patch
Comment 34 WebKit Review Bot 2013-04-04 10:01:04 PDT
Attachment 196486 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/borders/border-width-less-then-a-unit-of-pt-expected.html', u'LayoutTests/fast/borders/border-width-less-then-a-unit-of-pt.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/StyleBuilder.cpp']" exit_code: 1
Source/WebCore/css/StyleBuilder.cpp:575:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/css/StyleBuilder.cpp:576:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/css/StyleBuilder.cpp:577:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/css/StyleBuilder.cpp:578:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/css/StyleBuilder.cpp:666:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/css/StyleBuilder.cpp:667:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/css/StyleBuilder.cpp:668:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 7 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 35 Suchit Agrawal 2013-04-04 10:04:02 PDT
(In reply to comment #32)
> (From update of attachment 196129 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=196129&action=review
> 
> > Source/WebCore/css/StyleBuilder.cpp:586
> > +template <> unsigned computeLength<unsigned>(RenderStyle* style, RenderStyle* rootStyle, float zoom, CSSPrimitiveValue* primitiveValue)
> 
> So ApplyPropertyComputeLength<unsigned,... version is only used for border properties, making this special rounding behavior border specific. This is a very indirect and confusing way of getting border specific behavior.
> 
> I think it would be better to refactor this a bit. The current ApplyPropertyComputeLength could be replaced with several simpler versions
> 
> ApplyPropertyComputeBorderWidth (for border, outline and similar properties. this would have your new rounding behavior)
> ApplyPropertyComputeTextSpacing (for word and letter-spacing, only case that has svgZoomEnabled)
> ApplyPropertyComputeTransformOriginZ (this is the only float case and seems to want the rounding behavior only. maybe not even needed.)
> 
> Each of these can be much simpler and understandable than the current overly generic type. Most of the template parameters can be removed.
>
I applied above suggestion with border width logic which is used in previous patch.
Comment 36 Build Bot 2013-04-04 13:18:07 PDT
Comment on attachment 196486 [details]
Patch

Attachment 196486 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17484095

New failing tests:
tables/mozilla_expected_failures/bugs/bug89315.html
Comment 37 Build Bot 2013-04-04 13:18:11 PDT
Created attachment 196511 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.2
Comment 38 Build Bot 2013-04-04 21:14:50 PDT
Comment on attachment 196486 [details]
Patch

Attachment 196486 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17493328

New failing tests:
tables/mozilla_expected_failures/bugs/bug89315.html
Comment 39 Build Bot 2013-04-04 21:14:56 PDT
Created attachment 196572 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2
Comment 40 Antti Koivisto 2013-04-05 04:37:08 PDT
Comment on attachment 196486 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196486&action=review

> Source/WebCore/css/StyleBuilder.cpp:625
>  enum ComputeLengthSVGZoom {SVGZoomDisabled = 0, SVGZoomEnabled};
>  template <typename T,
>            T (RenderStyle::*getterFunction)() const,
>            void (RenderStyle::*setterFunction)(T),
>            T (*initialFunction)(),
>            ComputeLengthNormal normalEnabled = NormalDisabled,
> -          ComputeLengthThickness thicknessEnabled = ThicknessDisabled,
>            ComputeLengthSVGZoom svgZoomEnabled = SVGZoomDisabled>
> -class ApplyPropertyComputeLength {
> +class ApplyPropertyComputeTextSpacing {

- text spacing properties are always int so T can be dropped
- svgZoomEnabled and normalEnabled are always true for text spacing properties. Those can be dropped.

> Source/WebCore/css/StyleBuilder.cpp:669
> +template <typename T,
> +          T (RenderStyle::*getterFunction)() const,
> +          void (RenderStyle::*setterFunction)(T),
> +          T (*initialFunction)()>
> +class ApplyPropertyComputeTransformOriginZ {

This is always float. No need for T.

> Source/WebCore/css/StyleBuilder.cpp:2200
> -    setPropertyHandler(CSSPropertyWebkitBorderHorizontalSpacing, ApplyPropertyComputeLength<short, &RenderStyle::horizontalBorderSpacing, &RenderStyle::setHorizontalBorderSpacing, &RenderStyle::initialHorizontalBorderSpacing>::createHandler());
> +    setPropertyHandler(CSSPropertyWebkitBorderHorizontalSpacing, ApplyPropertyComputeTextSpacing<short, &RenderStyle::horizontalBorderSpacing, &RenderStyle::setHorizontalBorderSpacing, &RenderStyle::initialHorizontalBorderSpacing>::createHandler());
>      setPropertyHandler(CSSPropertyWebkitBorderImage, ApplyPropertyBorderImage<BorderImage, CSSPropertyWebkitBorderImage, &RenderStyle::borderImage, &RenderStyle::setBorderImage>::createHandler());
> -    setPropertyHandler(CSSPropertyWebkitBorderVerticalSpacing, ApplyPropertyComputeLength<short, &RenderStyle::verticalBorderSpacing, &RenderStyle::setVerticalBorderSpacing, &RenderStyle::initialVerticalBorderSpacing>::createHandler());
> +    setPropertyHandler(CSSPropertyWebkitBorderVerticalSpacing, ApplyPropertyComputeTextSpacing<short, &RenderStyle::verticalBorderSpacing, &RenderStyle::setVerticalBorderSpacing, &RenderStyle::initialVerticalBorderSpacing>::createHandler());

These are not text spacing properties so shouldn't use ApplyPropertyComputeTextSpacing. I suspect they can just use ApplyPropertyComputeBorderWidth with the new rounding.

> Source/WebCore/css/StyleBuilder.cpp:2221
> -    setPropertyHandler(CSSPropertyWebkitColumnRuleWidth, ApplyPropertyComputeLength<unsigned short, &RenderStyle::columnRuleWidth, &RenderStyle::setColumnRuleWidth, &RenderStyle::initialColumnRuleWidth, NormalDisabled, ThicknessEnabled>::createHandler());
> +    setPropertyHandler(CSSPropertyWebkitColumnRuleWidth, ApplyPropertyComputeBorderWidth<unsigned short, &RenderStyle::columnRuleWidth, &RenderStyle::setColumnRuleWidth, &RenderStyle::initialColumnRuleWidth, ThicknessEnabled>::createHandler());

This too.

> LayoutTests/fast/borders/border-width-less-then-a-unit-of-pt.html:38
> +<div style="border-width:.8pt">There should be a border around this text.</div>
> +<br/>
> +<table style="border-width:1.3pt">
> +    <tr>
> +    <td>Table border should present.</td>
> +    </tr>
> +</table>

The tests should cover all properties where behavior changes (outline-width, etc).
Comment 41 Antti Koivisto 2013-04-05 04:40:33 PDT
(In reply to comment #38)
> New failing tests:
> tables/mozilla_expected_failures/bugs/bug89315.html

This is a progression. It would be good to move the test from tables/mozilla_expected_failures to tables/mozilla (assuming it is fully correct now, update the results otherwise).
Comment 42 Suchit Agrawal 2013-04-05 09:48:25 PDT
Created attachment 196646 [details]
Patch
Comment 43 WebKit Review Bot 2013-04-05 09:50:04 PDT
Attachment 196646 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/borders/border-width-less-then-a-unit-of-pt-expected.html', u'LayoutTests/fast/borders/border-width-less-then-a-unit-of-pt.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/StyleBuilder.cpp']" exit_code: 1
Source/WebCore/css/StyleBuilder.cpp:578:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/css/StyleBuilder.cpp:620:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/css/StyleBuilder.cpp:621:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/css/StyleBuilder.cpp:663:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/css/StyleBuilder.cpp:664:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 44 Suchit Agrawal 2013-04-05 09:57:29 PDT
> (From update of attachment 196486 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=196486&action=review
> 
> > Source/WebCore/css/StyleBuilder.cpp:625
> >  enum ComputeLengthSVGZoom {SVGZoomDisabled = 0, SVGZoomEnabled};
> >  template <typename T,
> >            T (RenderStyle::*getterFunction)() const,
> >            void (RenderStyle::*setterFunction)(T),
> >            T (*initialFunction)(),
> >            ComputeLengthNormal normalEnabled = NormalDisabled,
> > -          ComputeLengthThickness thicknessEnabled = ThicknessDisabled,
> >            ComputeLengthSVGZoom svgZoomEnabled = SVGZoomDisabled>
> > -class ApplyPropertyComputeLength {
> > +class ApplyPropertyComputeTextSpacing {
> 
> - text spacing properties are always int so T can be dropped
> - svgZoomEnabled and normalEnabled are always true for text spacing properties. Those can be dropped.
> 
Changed as per comments.

> 
> > Source/WebCore/css/StyleBuilder.cpp:669
> > +template <typename T,
> > +          T (RenderStyle::*getterFunction)() const,
> > +          void (RenderStyle::*setterFunction)(T),
> > +          T (*initialFunction)()>
> > +class ApplyPropertyComputeTransformOriginZ {
> 
> This is always float. No need for T.
> 
Changed as per comments.

> 
> > Source/WebCore/css/StyleBuilder.cpp:2200
> > -    setPropertyHandler(CSSPropertyWebkitBorderHorizontalSpacing, ApplyPropertyComputeLength<short, &RenderStyle::horizontalBorderSpacing, &RenderStyle::setHorizontalBorderSpacing, &RenderStyle::initialHorizontalBorderSpacing>::createHandler());
> > +    setPropertyHandler(CSSPropertyWebkitBorderHorizontalSpacing, ApplyPropertyComputeTextSpacing<short, &RenderStyle::horizontalBorderSpacing, &RenderStyle::setHorizontalBorderSpacing, &RenderStyle::initialHorizontalBorderSpacing>::createHandler());
> >      setPropertyHandler(CSSPropertyWebkitBorderImage, ApplyPropertyBorderImage<BorderImage, CSSPropertyWebkitBorderImage, &RenderStyle::borderImage, &RenderStyle::setBorderImage>::createHandler());
> > -    setPropertyHandler(CSSPropertyWebkitBorderVerticalSpacing, ApplyPropertyComputeLength<short, &RenderStyle::verticalBorderSpacing, &RenderStyle::setVerticalBorderSpacing, &RenderStyle::initialVerticalBorderSpacing>::createHandler());
> > +    setPropertyHandler(CSSPropertyWebkitBorderVerticalSpacing, ApplyPropertyComputeTextSpacing<short, &RenderStyle::verticalBorderSpacing, &RenderStyle::setVerticalBorderSpacing, &RenderStyle::initialVerticalBorderSpacing>::createHandler());
> 
Changed as per comments.

> 
> These are not text spacing properties so shouldn't use ApplyPropertyComputeTextSpacing. I suspect they can just use ApplyPropertyComputeBorderWidth with the new rounding.
> 
> > Source/WebCore/css/StyleBuilder.cpp:2221
> > -    setPropertyHandler(CSSPropertyWebkitColumnRuleWidth, ApplyPropertyComputeLength<unsigned short, &RenderStyle::columnRuleWidth, &RenderStyle::setColumnRuleWidth, &RenderStyle::initialColumnRuleWidth, NormalDisabled, ThicknessEnabled>::createHandler());
> > +    setPropertyHandler(CSSPropertyWebkitColumnRuleWidth, ApplyPropertyComputeBorderWidth<unsigned short, &RenderStyle::columnRuleWidth, &RenderStyle::setColumnRuleWidth, &RenderStyle::initialColumnRuleWidth, ThicknessEnabled>::createHandler());
> 
> This too.
> 
No change required.

> 
> > LayoutTests/fast/borders/border-width-less-then-a-unit-of-pt.html:38
> > +<div style="border-width:.8pt">There should be a border around this text.</div>
> > +<br/>
> > +<table style="border-width:1.3pt">
> > +    <tr>
> > +    <td>Table border should present.</td>
> > +    </tr>
> > +</table>
> 
> The tests should cover all properties where behavior changes (outline-width, etc).
> 
Test case based on border width, outline-offset, outline-width, border spacing and column rule width properties.
Comment 45 Antti Koivisto 2013-04-05 10:15:25 PDT
Comment on attachment 196646 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196646&action=review

> Source/WebCore/css/StyleBuilder.cpp:584
> -        // note: CSSPropertyLetter/WordSpacing right now sets to zero if it's not a primitive value for some reason...
> -        if (!value->isPrimitiveValue())
> -            return;
> -
>          CSSPrimitiveValue* primitiveValue = static_cast<CSSPrimitiveValue*>(value);

Why is it ok to remove this test? Does something guarantee the value is primitive?

> Source/WebCore/css/StyleBuilder.cpp:618
> +enum ComputeLengthNormal {NormalDisabled = 0, NormalEnabled};
> +enum ComputeLengthSVGZoom {SVGZoomDisabled = 0, SVGZoomEnabled};

I don't think these are used anywhere anymore. You should remove them.
Comment 46 Suchit Agrawal 2013-04-05 11:55:04 PDT
Created attachment 196661 [details]
Patch
Comment 47 Suchit Agrawal 2013-04-05 12:00:09 PDT
(In reply to comment #45)
> (From update of attachment 196646 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=196646&action=review
> 
> > Source/WebCore/css/StyleBuilder.cpp:584
> > -        // note: CSSPropertyLetter/WordSpacing right now sets to zero if it's not a primitive value for some reason...
> > -        if (!value->isPrimitiveValue())
> > -            return;
> > -
> >          CSSPrimitiveValue* primitiveValue = static_cast<CSSPrimitiveValue*>(value);
> 
It was removed because Ther was commet that this condition for ASSPropertyLetter/wordspacing. As per discussion with you. I revert back it.

> 
> Why is it ok to remove this test? Does something guarantee the value is primitive?
> 
> > Source/WebCore/css/StyleBuilder.cpp:618
> > +enum ComputeLengthNormal {NormalDisabled = 0, NormalEnabled};
> > +enum ComputeLengthSVGZoom {SVGZoomDisabled = 0, SVGZoomEnabled};
> 
> I don't think these are used anywhere anymore. You should remove them.
> 
I removed these enums.
Comment 48 Antti Koivisto 2013-04-05 14:10:28 PDT
Comment on attachment 196661 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196661&action=review

> Source/WebCore/css/StyleBuilder.cpp:668
> +        // note: CSSPropertyLetter/WordSpacing right now sets to zero if it's not a primitive value for some reason...

I don't know what this comment means but it shouldn't be anywhere else than ApplyPropertyComputeTextSpacing.

> LayoutTests/ChangeLog:11
> +        This test file have border width, outline offset, outline width, border
> +        spacing and column rule width properties test cases.
> +        * fast/borders/border-width-less-then-a-unit-of-pt-expected.html: Added.
> +        * fast/borders/border-width-less-then-a-unit-of-pt.html: Added.

You still need to update the test result for tables/mozilla_expected_failures/bugs/bug89315.html
Comment 49 Suchit Agrawal 2013-04-08 05:58:12 PDT
Created attachment 196847 [details]
Patch
Comment 50 Suchit Agrawal 2013-04-08 06:09:22 PDT
(In reply to comment #48)
> (From update of attachment 196661 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=196661&action=review
> 
> > Source/WebCore/css/StyleBuilder.cpp:668
> > +        // note: CSSPropertyLetter/WordSpacing right now sets to zero if it's not a primitive value for some reason...
> 
> I don't know what this comment means but it shouldn't be anywhere else than ApplyPropertyComputeTextSpacing.
> 
> > LayoutTests/ChangeLog:11
> > +        This test file have border width, outline offset, outline width, border
> > +        spacing and column rule width properties test cases.
> > +        * fast/borders/border-width-less-then-a-unit-of-pt-expected.html: Added.
> > +        * fast/borders/border-width-less-then-a-unit-of-pt.html: Added.
> 
> You still need to update the test result for tables/mozilla_expected_failures/bugs/bug89315.html
> 
Test result for tables/mozilla_expected_failures/bugs/bug89315.html is updated in qt and made rebaseline for other platforms.
Comment 51 Suchit Agrawal 2013-04-08 06:59:01 PDT
Created attachment 196852 [details]
Patch
Comment 52 WebKit Commit Bot 2013-04-09 04:49:46 PDT
Comment on attachment 196852 [details]
Patch

Clearing flags on attachment: 196852

Committed r148010: <http://trac.webkit.org/changeset/148010>
Comment 53 WebKit Commit Bot 2013-04-09 04:49:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 54 WebKit Commit Bot 2013-04-09 15:51:02 PDT
Re-opened since this is blocked by bug 114310
Comment 55 Antti Koivisto 2013-04-10 06:42:46 PDT
Created attachment 197258 [details]
selection regression

Note the extra pixel between the selection outline and the control.
Comment 56 Suchit Agrawal 2013-04-12 05:56:39 PDT
Created attachment 197753 [details]
Patch
Comment 57 Suchit Agrawal 2013-04-12 06:01:13 PDT
(In reply to comment #55)
> Created an attachment (id=197258) [details]
> selection regression
> 
> Note the extra pixel between the selection outline and the control.
> 
This regression is fixed.
Template function roundForImpreciseConversion<>() is called with template 'T' in place of 'unsigned'.
Comment 58 Antti Koivisto 2013-04-12 06:24:52 PDT
(In reply to comment #57)
> This regression is fixed.
> Template function roundForImpreciseConversion<>() is called with template 'T' in place of 'unsigned'.

Can you add a test for it?
Comment 59 Build Bot 2013-04-12 07:05:41 PDT
Comment on attachment 197753 [details]
Patch

Attachment 197753 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/131044

New failing tests:
editing/style/5065910.html
Comment 60 Build Bot 2013-04-12 07:05:48 PDT
Created attachment 197845 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2
Comment 61 Suchit Agrawal 2013-04-12 08:41:14 PDT
Created attachment 197850 [details]
Patch
Comment 62 Suchit Agrawal 2013-04-12 08:48:30 PDT
(In reply to comment #58)
> (In reply to comment #57)
> > This regression is fixed.
> > Template function roundForImpreciseConversion<>() is called with template 'T' in place of 'unsigned'.
> 
> Can you add a test for it?
> 
Added new test case for focus ring arround form field.
Comment 63 Build Bot 2013-04-12 09:39:02 PDT
Comment on attachment 197850 [details]
Patch

Attachment 197850 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/83258

New failing tests:
fast/forms/Gap-test-between-form-field-and-focus-ring.html
platform/mac-wk2/plugins/mouse-events-scaled.html
Comment 64 Build Bot 2013-04-12 09:39:07 PDT
Created attachment 197861 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2
Comment 65 Chen Zhixiang 2013-07-10 01:00:47 PDT
Bug 118523 is related.
Comment 66 Chen Zhixiang 2013-07-10 01:37:48 PDT
*** Bug 118523 has been marked as a duplicate of this bug. ***
Comment 67 Csaba Osztrogonác 2013-11-05 08:55:16 PST
Comment on attachment 196646 [details]
Patch

Cleared review? from obsolete attachment 196646 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).
Comment 68 Andreas Kling 2014-02-05 11:23:04 PST
Comment on attachment 197850 [details]
Patch

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.
Comment 69 Brent Fulgham 2022-07-07 12:18:59 PDT
Safari, Chrome, and Firefox produce the same output on "Reduced test case v2". I don't believe there are any remaining compatibility issues here.

The test case in Bug 118523 also works properly.
Comment 70 Radar WebKit Bug Importer 2022-07-07 12:19:34 PDT
<rdar://problem/96616976>