Bug 68041 - REGRESSION (Safari 5.1-r95043): Incorrect box-shadow offset
Summary: REGRESSION (Safari 5.1-r95043): Incorrect box-shadow offset
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Matthew Delaney
URL: data:text/html,%3Cdiv%20style=%22widt...
Keywords: InRadar, Regression
: 67052 (view as bug list)
Depends on: 68137
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-13 16:27 PDT by mitz
Modified: 2011-09-15 14:22 PDT (History)
3 users (show)

See Also:


Attachments
Patch (19.81 KB, patch)
2011-09-14 23:47 PDT, Matthew Delaney
no flags Details | Formatted Diff | Diff
Patch (19.89 KB, patch)
2011-09-15 09:04 PDT, Matthew Delaney
no flags Details | Formatted Diff | Diff
Patch (36.14 KB, patch)
2011-09-15 11:18 PDT, Matthew Delaney
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2011-09-13 16:27:38 PDT
In the URL, the black shadow should be visible along the bottom of the red square, but in r95043 on OS X Lion, it isn’t.
Comment 1 Radar WebKit Bug Importer 2011-09-13 21:39:54 PDT
<rdar://problem/10121212>
Comment 2 Matthew Delaney 2011-09-14 23:47:40 PDT
Created attachment 107465 [details]
Patch
Comment 3 mitz 2011-09-15 00:04:58 PDT
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?
Comment 4 Matthew Delaney 2011-09-15 08:44:21 PDT
(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.
Comment 5 mitz 2011-09-15 08:46:08 PDT
(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()).
Comment 6 Matthew Delaney 2011-09-15 09:04:14 PDT
Created attachment 107502 [details]
Patch
Comment 7 Simon Fraser (smfr) 2011-09-15 09:10:52 PDT
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.
Comment 8 Matthew Delaney 2011-09-15 11:17:29 PDT
(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.
Comment 9 Matthew Delaney 2011-09-15 11:18:29 PDT
Created attachment 107514 [details]
Patch
Comment 10 Matthew Delaney 2011-09-15 11:59:39 PDT
Committed r95207: <http://trac.webkit.org/changeset/95207>
Comment 11 Matthew Delaney 2011-09-15 14:22:49 PDT
*** Bug 67052 has been marked as a duplicate of this bug. ***