Summary: | Table border doesn't show up | ||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Scott <pcs0325> | ||||||||||||||||||||||||||||||||||||||||||
Component: | CSS | Assignee: | 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
David Scott
2007-05-13 20:27:27 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). The border is using the "windowtext" system color. I would guess we have some bug with that system color implementation. 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. Created attachment 20854 [details]
Original page source
This is an MS Word 9 document exported as HTML.
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.
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.)
Would switching to using floats to store CSS values fix this? I am working for this issue. I have patch for it. Created attachment 195965 [details]
Patch
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 on attachment 195965 [details] Patch Attachment 195965 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17378165 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 on attachment 195965 [details] Patch Attachment 195965 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17386133 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 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 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 on attachment 195965 [details] Patch Attachment 195965 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17348451 Created attachment 195988 [details]
Patch
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 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 (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 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). (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 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 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
Created attachment 196129 [details]
Patch
(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 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 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
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 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. Created attachment 196486 [details]
Patch
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.
(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 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 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 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 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 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). (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). Created attachment 196646 [details]
Patch
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.
> (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 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. Created attachment 196661 [details]
Patch
(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 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 Created attachment 196847 [details]
Patch
(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. Created attachment 196852 [details]
Patch
Comment on attachment 196852 [details] Patch Clearing flags on attachment: 196852 Committed r148010: <http://trac.webkit.org/changeset/148010> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by bug 114310 Created attachment 197258 [details]
selection regression
Note the extra pixel between the selection outline and the control.
Created attachment 197753 [details]
Patch
(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'. (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 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 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
Created attachment 197850 [details]
Patch
(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 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 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
Bug 118523 is related. *** Bug 118523 has been marked as a duplicate of this bug. *** 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 on attachment 197850 [details]
Patch
Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.
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. |