Map width/height to style for foreignObject

Map width/height to style for foreignObject

As we move towards using the style system to resolve percentages for
svg, start small by promoting width and height for foreignObject to
presentation attributes.

This is a preparation for issue 308992 where we plan to make
height="100%" for svg elements behave as style="height:100%". Unless
height is mapped to style for foreignObject, percentage style values
for svg inside foreignObject cannot be resolved.

This has been resolved in the SVG WG, see minutes
http://www.w3.org/2014/04/08-svg-minutes.html#item04

BUG=308992, 358206
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=171306

[davve, 2014-04-02 09:37:35]

Baby steps for presentation attributes, starting with foreignObject.

I think the new repaints is because we now tell the style system about when
width/height attributes change on foreignObject, which we have to do to get
style() updated when something like an svg animation would animate the
width/height attribute or svg dom poking at the value. I've added a test for the
latter.

I haven't had much experience with LayoutTests/TestExpectations before so please
check that I've done that correctly.

[fs, 2014-04-02 11:00:41]

https://codereview.chromium.org/218403009/diff/40001/Source/core/svg/SVGForei...
File Source/core/svg/SVGForeignObjectElement.cpp (right):

https://codereview.chromium.org/218403009/diff/40001/Source/core/svg/SVGForei...
Source/core/svg/SVGForeignObjectElement.cpp:94: if (name == SVGNames::widthAttr
|| name == SVGNames::heightAttr) {
It seems unnecessary to check these twice (for no apparent reason)?

https://codereview.chromium.org/218403009/diff/40001/Source/core/svg/SVGForei...
Source/core/svg/SVGForeignObjectElement.cpp:96:
addPropertyToPresentationAttributeStyle(style, CSSPropertyWidth,
m_width->currentValue()->valueAsString());
Why not just pass |value| as the value? (IIUC, if there's animations, they would
use the override style.)

[krit, 2014-04-02 11:07:01]

On 2014/04/02 11:00:41, fs wrote:
>
https://codereview.chromium.org/218403009/diff/40001/Source/core/svg/SVGForei...
> File Source/core/svg/SVGForeignObjectElement.cpp (right):
> 
>
https://codereview.chromium.org/218403009/diff/40001/Source/core/svg/SVGForei...
> Source/core/svg/SVGForeignObjectElement.cpp:94: if (name ==
SVGNames::widthAttr
> || name == SVGNames::heightAttr) {
> It seems unnecessary to check these twice (for no apparent reason)?
> 
>
https://codereview.chromium.org/218403009/diff/40001/Source/core/svg/SVGForei...
> Source/core/svg/SVGForeignObjectElement.cpp:96:
> addPropertyToPresentationAttributeStyle(style, CSSPropertyWidth,
> m_width->currentValue()->valueAsString());
> Why not just pass |value| as the value? (IIUC, if there's animations, they
would
> use the override style.)
 
I wonder how that works with SVG DOM. Specifically, foreignObject.width.baseVal
=.

I assume we do not have tests for that why no test is failing.

[davve, 2014-04-02 11:17:48]

https://codereview.chromium.org/218403009/diff/40001/Source/core/svg/SVGForei...
File Source/core/svg/SVGForeignObjectElement.cpp (right):

https://codereview.chromium.org/218403009/diff/40001/Source/core/svg/SVGForei...
Source/core/svg/SVGForeignObjectElement.cpp:94: if (name == SVGNames::widthAttr
|| name == SVGNames::heightAttr) {
On 2014/04/02 11:00:41, fs wrote:
> It seems unnecessary to check these twice (for no apparent reason)?

Thanks. Was remains from earlier versions.

https://codereview.chromium.org/218403009/diff/40001/Source/core/svg/SVGForei...
Source/core/svg/SVGForeignObjectElement.cpp:96:
addPropertyToPresentationAttributeStyle(style, CSSPropertyWidth,
m_width->currentValue()->valueAsString());
On 2014/04/02 11:00:41, fs wrote:
> Why not just pass |value| as the value? (IIUC, if there's animations, they
would
> use the override style.)

I specifically added a test that fails when using |value|. Svg dom won't
override in current state. Something like setting width.baseVal.value will not
touch the underlying attribute.

[fs, 2014-04-02 11:54:59]

https://codereview.chromium.org/218403009/diff/40001/Source/core/svg/SVGForei...
File Source/core/svg/SVGForeignObjectElement.cpp (right):

https://codereview.chromium.org/218403009/diff/40001/Source/core/svg/SVGForei...
Source/core/svg/SVGForeignObjectElement.cpp:96:
addPropertyToPresentationAttributeStyle(style, CSSPropertyWidth,
m_width->currentValue()->valueAsString());
On 2014/04/02 11:17:48, David Vest wrote:
> On 2014/04/02 11:00:41, fs wrote:
> > Why not just pass |value| as the value? (IIUC, if there's animations, they
> would
> > use the override style.)
> 
> I specifically added a test that fails when using |value|. Svg dom won't
> override in current state. Something like setting width.baseVal.value will not
> touch the underlying attribute.

Ok. I figured that's what the change in svgAttributeChanged() was for. Some
synchronization missing in the pres-attr code-path then maybe...

[davve, 2014-04-02 12:43:55]

https://codereview.chromium.org/218403009/diff/40001/Source/core/svg/SVGForei...
File Source/core/svg/SVGForeignObjectElement.cpp (right):

https://codereview.chromium.org/218403009/diff/40001/Source/core/svg/SVGForei...
Source/core/svg/SVGForeignObjectElement.cpp:96:
addPropertyToPresentationAttributeStyle(style, CSSPropertyWidth,
m_width->currentValue()->valueAsString());
On 2014/04/02 11:55:00, fs wrote:
> On 2014/04/02 11:17:48, David Vest wrote:
> > On 2014/04/02 11:00:41, fs wrote:
> > > Why not just pass |value| as the value? (IIUC, if there's animations, they
> > would
> > > use the override style.)
> > 
> > I specifically added a test that fails when using |value|. Svg dom won't
> > override in current state. Something like setting width.baseVal.value will
not
> > touch the underlying attribute.
> 
> Ok. I figured that's what the change in svgAttributeChanged() was for.

It is. But it only triggers the recomputation, doesn't affect the outcome.

> Some synchronization missing in the pres-attr code-path then maybe...

I'll look into it.

[davve, 2014-04-02 14:13:44]

Dependency on svg animation values from presentation attributes now addressed.
Had to take some care to bridge the svg attribute to css property conversion.
PTAL.

[fs, 2014-04-02 14:38:30]

https://codereview.chromium.org/218403009/diff/80001/Source/core/svg/SVGForei...
File Source/core/svg/SVGForeignObjectElement.cpp (right):

https://codereview.chromium.org/218403009/diff/80001/Source/core/svg/SVGForei...
Source/core/svg/SVGForeignObjectElement.cpp:97: if (length->unitType() ==
LengthTypeNumber)
Won't the mode == SVGAttributeMode on the MutableStylePropertySet make this
unnecesary?

[davve, 2014-04-02 18:23:26]

https://codereview.chromium.org/218403009/diff/80001/Source/core/svg/SVGForei...
File Source/core/svg/SVGForeignObjectElement.cpp (right):

https://codereview.chromium.org/218403009/diff/80001/Source/core/svg/SVGForei...
Source/core/svg/SVGForeignObjectElement.cpp:97: if (length->unitType() ==
LengthTypeNumber)
On 2014/04/02 14:38:31, fs wrote:
> Won't the mode == SVGAttributeMode on the MutableStylePropertySet make this
> unnecesary?

True, it would still work without it. But we would miss the length fast-path in
BisonCSSParser::parseValue which made me add it (when I originally wrote it for
SVGSVGElement). I can go either way.

[fs, 2014-04-03 07:50:40]

Non-owner LGTM.

https://codereview.chromium.org/218403009/diff/80001/Source/core/svg/SVGForei...
File Source/core/svg/SVGForeignObjectElement.cpp (right):

https://codereview.chromium.org/218403009/diff/80001/Source/core/svg/SVGForei...
Source/core/svg/SVGForeignObjectElement.cpp:97: if (length->unitType() ==
LengthTypeNumber)
On 2014/04/02 18:23:27, David Vest wrote:
> On 2014/04/02 14:38:31, fs wrote:
> > Won't the mode == SVGAttributeMode on the MutableStylePropertySet make this
> > unnecesary?
> 
> True, it would still work without it. But we would miss the length fast-path
in
> BisonCSSParser::parseValue which made me add it (when I originally wrote it
for
> SVGSVGElement). I can go either way.

Ok. Maybe just add a comment to that effect then?

[davve, 2014-04-03 08:43:56]

https://codereview.chromium.org/218403009/diff/80001/Source/core/svg/SVGForei...
File Source/core/svg/SVGForeignObjectElement.cpp (right):

https://codereview.chromium.org/218403009/diff/80001/Source/core/svg/SVGForei...
Source/core/svg/SVGForeignObjectElement.cpp:97: if (length->unitType() ==
LengthTypeNumber)
On 2014/04/03 07:50:41, fs wrote:
> On 2014/04/02 18:23:27, David Vest wrote:
> > On 2014/04/02 14:38:31, fs wrote:
> > > Won't the mode == SVGAttributeMode on the MutableStylePropertySet make
this
> > > unnecesary?
> > 
> > True, it would still work without it. But we would miss the length fast-path
> in
> > BisonCSSParser::parseValue which made me add it (when I originally wrote it
> for
> > SVGSVGElement). I can go either way.
> 
> Ok. Maybe just add a comment to that effect then?

Leaning towards removing now. Enabling the presentation attribute cache for SVG
elements would make this even more redundant.

[davve, 2014-04-03 09:19:41]

On 2014/04/03 08:43:56, David Vest wrote:
> Leaning towards removing now. Enabling the presentation attribute cache for
SVG
> elements would make this even more redundant.

Now removed. I rebased in the same upload, which I realized wasn't a nice thing
to do. The only change to the patch is:

...
-        if (length->unitType() == LengthTypeNumber)
-            length->setUnitType(LengthTypePX);
...

[pdr., 2014-04-09 10:01:15]

On 2014/04/03 09:19:41, David Vest wrote:
> On 2014/04/03 08:43:56, David Vest wrote:
> > Leaning towards removing now. Enabling the presentation attribute cache for
> SVG
> > elements would make this even more redundant.
> 
> Now removed. I rebased in the same upload, which I realized wasn't a nice
thing
> to do. The only change to the patch is:
> 
> ...
> -        if (length->unitType() == LengthTypeNumber)
> -            length->setUnitType(LengthTypePX);
> ...

LGTM.

We discussed this at the SVG f2f and resolved to map these in the spec. Could
you add a little comment in the description point to that resolution?

[davve, 2014-04-10 11:56:28]

Found that surrounding code used synchronizeAllAttributes() instead of doing
manual flag checking. Could you PTAL, fs? The difference is in
https://codereview.chromium.org/218403009/diff2/120001:150001/Source/core/dom...

[fs, 2014-04-10 12:16:32]

On 2014/04/10 11:56:28, David Vest wrote:
> Found that surrounding code used synchronizeAllAttributes() instead of doing
> manual flag checking. Could you PTAL, fs? The difference is in
>
https://codereview.chromium.org/218403009/diff2/120001:150001/Source/core/dom...

Looks like a slightly heavier sledgehammer (possibly also syncs other
attributes). Hopefully we'll notice if it's disproportionate though, so still
LGTM.

[davve, 2014-04-10 12:19:23]

The CQ bit was checked by davve@opera.com

[commit-bot: I haz the power, 2014-04-10 12:19:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davve@opera.com/218403009/150001

[commit-bot: I haz the power, 2014-04-10 12:54:24]

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

[commit-bot: I haz the power, 2014-04-10 12:54:25]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[davve, 2014-04-10 14:43:18]

The CQ bit was checked by davve@opera.com

[commit-bot: I haz the power, 2014-04-10 14:43:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davve@opera.com/218403009/170001

[commit-bot: I haz the power, 2014-04-10 15:52:38]

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

[commit-bot: I haz the power, 2014-04-10 15:52:38]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[pdr., 2014-04-10 18:53:02]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-04-10 18:53:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davve@opera.com/218403009/170001

[commit-bot: I haz the power, 2014-04-10 20:01:27]

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

[commit-bot: I haz the power, 2014-04-10 20:01:28]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[pdr., 2014-04-10 20:37:05]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-04-10 20:37:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davve@opera.com/218403009/170001

[commit-bot: I haz the power, 2014-04-10 21:41:56]

Change committed as 171306