Bug 13709 – Table border doesn't show up

• Component: CSS
• Status: RESOLVED FIXED

[David Scott]

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.

[David Kilzer (:ddkilzer)]

Confirmed with a local debug build of WebKit r21447 with Safari 2.0.4 (419.3) on Mac OS X 10.4.9 (8P135).

[Dave Hyatt]

The border is using the "windowtext" system color.  I would guess we have some bug with that system color implementation.

[David Scott]

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.

[Sam Stigler]

Confirmed as still not working in r30377 on 10.5.2.

[David Kilzer (:ddkilzer)]

Created attachment 20854 [details]
Original page source

This is an MS Word 9 document exported as HTML.

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

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.)

[David Kilzer (:ddkilzer)]

Would switching to using floats to store CSS values fix this?

[Suchit Agrawal]

I am working for this issue. I have patch for it.

[Suchit Agrawal]

Created attachment 195965 [details]
Patch

[WebKit Review Bot]

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

[Early Warning System Bot]

Comment on attachment 195965 [details]
Patch

Attachment 195965 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17378165

[Early Warning System Bot]

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

[EFL EWS Bot]

Comment on attachment 195965 [details]
Patch

Attachment 195965 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17386133

[Peter Beverloo (cr-android ews)]

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

[WebKit Review Bot]

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

[Build Bot]

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

[Build Bot]

Comment on attachment 195965 [details]
Patch

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

[Suchit Agrawal]

Created attachment 195988 [details]
Patch

[Build Bot]

Comment on attachment 195988 [details]
Patch

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

[Build Bot]

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

[Suchit Agrawal]

(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.

[Julien Chaffraix]

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).

[Suchit Agrawal]

(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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Suchit Agrawal]

Created attachment 196129 [details]
Patch

[Suchit Agrawal]

(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'.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Beth Dakin]

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?

[Antti Koivisto]

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.

[Suchit Agrawal]

Created attachment 196486 [details]
Patch

[WebKit Review Bot]

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.

[Suchit Agrawal]

(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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Antti Koivisto]

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).

[Antti Koivisto]

(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).

[Suchit Agrawal]

Created attachment 196646 [details]
Patch

[WebKit Review Bot]

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.

[Suchit Agrawal]

> (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.

[Antti Koivisto]

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.

[Suchit Agrawal]

Created attachment 196661 [details]
Patch

[Suchit Agrawal]

(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.

[Antti Koivisto]

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

[Suchit Agrawal]

Created attachment 196847 [details]
Patch

[Suchit Agrawal]

(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.

[Suchit Agrawal]

Created attachment 196852 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 196852 [details]
Patch

Clearing flags on attachment: 196852

Committed r148010: <http://trac.webkit.org/changeset/148010>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 114310

[Antti Koivisto]

Created attachment 197258 [details]
selection regression

Note the extra pixel between the selection outline and the control.

[Suchit Agrawal]

Created attachment 197753 [details]
Patch

[Suchit Agrawal]

(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'.

[Antti Koivisto]

(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?

[Build Bot]

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

[Build Bot]

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

[Suchit Agrawal]

Created attachment 197850 [details]
Patch

[Suchit Agrawal]

(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.

[Build Bot]

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

[Build Bot]

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

[Chen Zhixiang]

Bug 118523 is related.

[Chen Zhixiang]

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

[Csaba Osztrogonác]

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).

[Andreas Kling]

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.

[Brent Fulgham]

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.

[Radar WebKit Bug Importer]

<rdar://problem/96616976>