Adjust background clip when local attachment is used with non-visible overflow.

Adjust background clip when local attachment is used with non-visible overflow.

BUG=625882
TEST=compositing/backgrounds/local-background.html

Committed: https://crrev.com/e3a98dc7050d4e575967177edaca49742024e8c8
Cr-Commit-Position: refs/heads/master@{#407871}

[flackr, 2016-07-05 21:21:33]

flackr@chromium.org changed reviewers:
+ meade@chromium.org

[flackr, 2016-07-05 21:21:34]

Hi,

Can you take a look at this patch? This fixes an issue we have with compositing
background-attachment local scrollable elements (See the test). Basically,
LayoutBox::backgroundIsKnownToBeOpaqueInRect assumes that the background clip on
the style is correct which results in us marking the background layer of
composited scrollers as opaque incorrectly for background attachment local. The
spec says that for scrollable elements the ‘border-box’ value of
‘background-clip’ may be treated the same as ‘padding-box’ [2] so this change
modifies the computed style so that we can assume it has the correct clip rather
than complicating bounds checks elsewhere. WDYT?

Thanks,
Rob

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...
[2] https://www.w3.org/TR/css3-background/#the-background-attachment

[meade_UTC10, 2016-07-06 05:27:10]

meade@chromium.org changed reviewers:
+ timloh@chromium.org

[meade_UTC10, 2016-07-06 05:27:10]

lgtm, but I don't have owners. Adding Tim for owners.

[Timothy Loh, 2016-07-06 05:45:01]

Doesn't this mean that getComputedStyle(e).backgroundClip will now be
padding-box instead of border-box? The spec defines the computed value for
background-clip to be 'as specified' so changing the value here is probably not
right (FF returns border-box). Maybe we could instead add another bitfield for
the adjusted value so getComputedStyle and inheritance use the unadjusted value?

[flackr, 2016-07-06 19:53:50]

On 2016/07/06 at 05:45:01, timloh wrote:
> Doesn't this mean that getComputedStyle(e).backgroundClip will now be
padding-box instead of border-box? The spec defines the computed value for
background-clip to be 'as specified' so changing the value here is probably not
right (FF returns border-box). Maybe we could instead add another bitfield for
the adjusted value so getComputedStyle and inheritance use the unadjusted value?

I see, thanks for catching this. Perhaps it's simpler to provide/use a helper
method for calculating the background clip rect which takes this into account
(see patch 3)?

[flackr, 2016-07-08 01:34:22]

Tim, would you have a chance to look at the latest patch?

[Timothy Loh, 2016-07-08 08:50:42]

On 2016/07/08 01:34:22, flackr wrote:
> Tim, would you have a chance to look at the latest patch?

Sorry I didn't get a chance to look at it today, and will be OOO next week. Feel
free to send to someone else if you want it in sooner (looks like it's just
touching layout code now?)

[flackr, 2016-07-08 13:37:36]

flackr@chromium.org changed reviewers:
+ eae@chromium.org
- timloh@chromium.org

[flackr, 2016-07-08 13:37:36]

+Emil, can you take a look?

[eae, 2016-07-12 16:40:14]

LGTM

Nice change!

[eae, 2016-07-12 16:40:18]

The CQ bit was checked by eae@chromium.org

[eae, 2016-07-12 16:40:19]

The patchset sent to the CQ was uploaded after l-g-t-m from meade@chromium.org
Link to the patchset: https://codereview.chromium.org/2127663002/#ps40001
(title: "Restrict to the padding box rect when attachment is local on scrollable
element.")

[commit-bot: I haz the power, 2016-07-12 16:41:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-12 18:29:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-12 18:29:48]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[flackr, 2016-07-25 21:02:47]

The CQ bit was checked by flackr@chromium.org

[commit-bot: I haz the power, 2016-07-25 21:03:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-25 23:52:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-25 23:52:34]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[flackr, 2016-07-26 17:31:20]

The CQ bit was checked by flackr@chromium.org

[commit-bot: I haz the power, 2016-07-26 17:31:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-26 19:03:53]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-26 19:05:16]

Description was changed from

==========
Adjust background clip when local attachment is used with non-visible overflow.

BUG=625882
TEST=compositing/backgrounds/local-background.html
==========

to

==========
Adjust background clip when local attachment is used with non-visible overflow.

BUG=625882
TEST=compositing/backgrounds/local-background.html

Committed: https://crrev.com/e3a98dc7050d4e575967177edaca49742024e8c8
Cr-Commit-Position: refs/heads/master@{#407871}
==========

[commit-bot: I haz the power, 2016-07-26 19:05:17]

Patchset 3 (id:??) landed as
https://crrev.com/e3a98dc7050d4e575967177edaca49742024e8c8
Cr-Commit-Position: refs/heads/master@{#407871}