Incorrect style recalc for text-decoration-[style, color] for immediate child text nodes.

Incorrect style recalc for text-decoration-[style, color] for immediate child text nodes.

There is a bug where the text decoration color/style is not updated when hovered (or
the style is otherwise changed). This is because text-decoration-color/style are non-
inheritable properties, so we don't bother recalculating the child styles if only
text-decoration-color or -style changes. Hence, the immediate text children of an
element will use the old RenderStyle of the parent.

Fixed by recalculating child styles if the text decorations differ between styles.

BUG=350840
TEST=text-decoration-[color, line, style].html

[andersr, 2014-03-10 10:10:28]

An alternative fix (not this patch) which also looks like it should work:

Modify RenderObject::getTextDecorationColors such that it does not start the
traversal with the style for the text node itself, but rather its parent.

That will currently not fix text-decoration-style, though, because it's
implemented incorrectly. (Should be easy to fix).

[Stephen Chennney, 2014-03-10 14:19:30]

+esprehn

[esprehn, 2014-03-12 09:51:17]

I think you actually want to fix these properties to be inherited using the same
mechanism we inherit text-decoration with. Specifically look into
_text_decorations in RenderStyle.

The spec says these things aren't really inherited, but in the engine we need to
treat them like they are.

If you did want to make this change though then it should be inside
::inheritedNotEqual, ::compare() is supposed to be generic.

[andersr, 2014-03-12 10:45:39]

On 2014/03/12 09:51:17, esprehn wrote:
> I think you actually want to fix these properties to be inherited using the
same
> mechanism we inherit text-decoration with. Specifically look into
> _text_decorations in RenderStyle.
> The spec says these things aren't really inherited, but in the engine we need
to
> treat them like they are.

Yes, I thought about that.

What we currently do is store the text-decoration in nonInheritableProperties,
but store the
"applied" decorations as three bits of inheritable information. Then, during
painting, we traverse parents to figure out which element each decoration
originated from, so that we know which color (and now style
https://codereview.chromium.org/195093002/ ) to use. I assumed this delayed
ghetto-inheritance was done because it was undesirable to duplicate the applied
color as inheritable information.

It seems to me that in order to do it in a similar way to our current "applied
decorations"-way, we must store the color (and style) for each applied
decoration. I.e. rather than set three bits, maintain a list of N applied
decoration objects. The increase from three bits to a list of N
AppliedDecoration objects did not strike me as an obviously acceptable change.
:)

Is that acceptable? Is there a better way?

(Related note: Firefox supports multiple decorations on the same line, e.g. you
can have a wavy underline with a solid underline drawn over it. Storing N
AppliedDecoration objects would also give us that possibility. Currently, with
three bits, we can only support one color/style per
underline/overline/line-through).

[esprehn, 2014-03-14 21:42:11]

On 2014/03/12 10:45:39, rude wrote:
> ...
> 
> Is that acceptable? Is there a better way?
> 
> (Related note: Firefox supports multiple decorations on the same line, e.g.
you
> can have a wavy underline with a solid underline drawn over it. Storing N
> AppliedDecoration objects would also give us that possibility. Currently, with
> three bits, we can only support one color/style per
> underline/overline/line-through).

Yes, lets just add a pointer in there to a list of decorations. I don't think
these are so common that it's going to be an issue. As long as the common case
of just underline (since that's links) remains using a faster/simpler method it
seems like an acceptable change.

[andersr, 2014-03-31 17:07:14]

Closing this.

New and quite different fix: https://codereview.chromium.org/203273003/

[andersr, 2014-04-03 10:59:33]

> New and quite different fix: https://codereview.chromium.org/203273003/

That was the wrong URL. The correct one is:
https://codereview.chromium.org/219633002/