Summary: | REGRESSION (Safari 5.1-r95043): Incorrect box-shadow offset | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | mitz | ||||||||
Component: | Layout and Rendering | Assignee: | Matthew Delaney <mdelaney7> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | mdelaney7, simon.fraser, webkit-bug-importer | ||||||||
Priority: | P1 | Keywords: | InRadar, Regression | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
URL: | data:text/html,%3Cdiv%20style=%22width:%20100px;%20height:%20100px;%20-webkit-box-shadow:%20black%201px%201px;%20background:%20red;%22%3E | ||||||||||
Bug Depends on: | 68137 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
mitz
2011-09-13 16:27:38 PDT
Created attachment 107465 [details]
Patch
Comment on attachment 107465 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=107465&action=review > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1028 > + if (!this->isAcceleratedContext()) { Why the “this->” here? (In reply to comment #3) > (From update of attachment 107465 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=107465&action=review > > > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1028 > > + if (!this->isAcceleratedContext()) { > > Why the “this->” here? I realize it's unnecessary. Just thought it reads clearer, as if to explicitly say "I am" - and I recall Simon using that convention too. Not sure if it's against WebKit style rules. (In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 107465 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=107465&action=review > > > > > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1028 > > > + if (!this->isAcceleratedContext()) { > > > > Why the “this->” here? > > I realize it's unnecessary. Just thought it reads clearer, as if to explicitly say "I am" - and I recall Simon using that convention too. Not sure if it's against WebKit style rules. I’ve only seen this used when there’s a same-named identifier in local scope (e.g. bool foo = this->foo()). Created attachment 107502 [details]
Patch
Comment on attachment 107502 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=107502&action=review > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1030 > + // Work around <rdar://problem/5539388> by ensuring that the offsets will get truncated > + // to the desired integer. You may want to add a link to <rdar://problem/10056277> as well. > LayoutTests/fast/box-shadow/one-pixel-offset-bottom-right.html:1 > +<div style="width: 100px; height: 100px; -webkit-box-shadow: black 1px 1px; background: red;"> Please don't use red in a passing test. Don't we have existing tests that show this shadow offset problem? I'm not sure we need a new one. (In reply to comment #7) > (From update of attachment 107502 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=107502&action=review > > > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1030 > > + // Work around <rdar://problem/5539388> by ensuring that the offsets will get truncated > > + // to the desired integer. > > You may want to add a link to <rdar://problem/10056277> as well. Done. > > > LayoutTests/fast/box-shadow/one-pixel-offset-bottom-right.html:1 > > +<div style="width: 100px; height: 100px; -webkit-box-shadow: black 1px 1px; background: red;"> > > Please don't use red in a passing test. > > Don't we have existing tests that show this shadow offset problem? I'm not sure we need a new one. None that I found that hit the correct combination of offsets and div size to trigger the bug while also being outside the default tolerance of the pixel tests. I'm adding in a better test that hits a handful of those cases enough to fail. Created attachment 107514 [details]
Patch
Committed r95207: <http://trac.webkit.org/changeset/95207> *** Bug 67052 has been marked as a duplicate of this bug. *** |