1235566 - Support will-change: background-position

Status: ASSIGNED

(Core :: Layout, defect)

Description

[Markus Stange [:mstange]]

Since bug 947062 we now use ImageLayers for (supported) background images with animated background-position. However, when we determine whether background-position is animated, we only recognize CSS animations, and inline style changes to the properties that are made within a scroll event handler.
In addition to these, we should respect will-change: background-position.

Comment 1

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1235566 - Respect will-change: background-position. r?dbaron

Review commit: https://reviewboard.mozilla.org/r/29145/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29145/

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8702609 [details]
MozReview Request: Bug 1235566 - Respect will-change: background-position. r?dbaron

https://reviewboard.mozilla.org/r/29145/#review26463

::: layout/style/nsRuleNode.cpp:6100
(Diff revision 1)
> +        if (buffer.EqualsLiteral("background") ||
> +            buffer.EqualsLiteral("background-position") ||
> +            buffer.EqualsLiteral("background-position-x") ||
> +            buffer.EqualsLiteral("background-position-y")) {
> +          display->mWillChangeBitField |= NS_STYLE_WILL_CHANGE_BACKGROUND_POS;
> +        }

So once the patch in bug 686281 to deal with will-change on a shorthand lands (at least if it addresses my review comments in bug 686281 comment 336 correctly) it would probably be better to check for just background-position-x and background-position-y, by their CSS property ID, inside the function I propose in those review comments.

Given how long it's taking for bug 686281 to land, it might help if you took over and landed that one patch (which is independent of the rest), if CJ is ok with that.

::: layout/style/nsStyleConsts.h:248
(Diff revision 1)
> +#define NS_STYLE_WILL_CHANGE_BACKGROUND_POS     (1<<5)

I find it weird to abbreviate position and not abbreviate background.  Do you mind renaming this to either ...BG_POS or ...BG_POSITION?

Otherwise this looks good, but it's a little annoying that there's so much reviewed-but-unlanded stuff floating around here.