Resolution for issue Fill or stroke attributes on a use element

[tjindal]

Resolution for issue Fill or stroke attributes on a use element changes as paint implementation is not getting transfered from reference to graphic element

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

• [ ] Bug fix (non-breaking change which fixes an issue)
• [ ] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• [ ] I have signed the Adobe Open Source CLA.
• [ ] My code follows the code style of this project.
• [ ] My change requires a change to the documentation.
• [ ] I have updated the documentation accordingly.
• [ ] I have read the CONTRIBUTING document.
• [ ] I have added tests to cover my changes.
• [ ] All new and existing tests passed.

[dirkschulze]

For what ever reason, I am not detected as reviewer of this PR anymore. I can't even give inline comments. Here my comments:

I see the issue now.

We have 2 phases:

1. parsing phase and
2. rendering phase (TraverseTree)

I thought that we moved the style (inheritance) resolution to phase 2 but we didn't. Inheritance resolution still happens in phase 1.

This was ok in the beginning since we replaced the SVGUseElement with a SVGGElement and re-parsed the element in question. This required that all nodes that get referenced were parsed already, which might not be the case.

So we moved from literally copying sub-trees to the Reference object. Now, we just keep a reference string. On rendering, we see if the element was found during parse time. If yes, we render it.

Apparently we did not change the inheritance resolving with it. As a result, the inheritance information is lost at render time. The referenced element's style was resolved to the root element in DOM tree and not in render tree already.

As a result, the following example shows both rectangles as yellow even though the 2nd should be green:

<g fill="yellow">
    <rect id="r" width="100" height="100"/>
</g>
<use xlink:href="#r" x="120" fill="green"/>

@tjindal You are trying to fix the symptoms but that will just cause even more issues since you don't have all information. From what I get, it seems that you check if the SVGUseElement's (Reference object's) style matches the style of the referenced object and determine by that if the style needs to get overridden or not. This doesn't even work for simple examples:

<rect id="r" width="100" height="100" fill="green"/>
<use x="120" fill="red" xlink:href="#r"/>

With your changes, the 2nd rect would turn red but it should stay green. Inherited properties should only take the style of the ancestor if the value is not set.

One way that I see is moving inheritance resolution to the rendering phase (from phase 1 to phase 2). We could use boost::optional<T> for each of the inherited properties. (So all values within FillStyle and StrokeStyle.) Unsure if we should avoid adding more boost constructs.

Should we go with boost::optional, mFillStyleStack and mStrokeStyleStack would be used for phase 2 and not phase 1 and need to get set on each ElementType::kReference/ElementType::kGroup (maybe as part of SaveRestoreHelper?).

An issue with that approach is rendering only sub-trees via Render(const char* id). The style might not be resolved at the time.

A slightly different approach is traversing the parsed document right after parsing directly and resolving all styles then. In that case, RenderTree could stay untouched. However, this means 3 passes:

1. parsing,
2. style resolving,
3. rendering.

We could optimize style resolving to certain subtrees:

• Only resolve styles for sub-trees where the outermost ancestor has an id attribute. Only those elements can get referenced.
• If there is no ancestor with an id attribute, the style can get resolved during parsing in phase 1.

I am open to ideas :)