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]

What I get from the issue, you try to fix the following:

<circle id="myCircle" cx="50" cy="50" r="40" stroke="blue" stroke-width="10"/>
<use href="#myCircle" x="100" fill="blue"/>

Right now I see 2 issues:

1. You may overwrite all properties and reset other properties of the referenced object. See the stroke on above's example. It might get reset with this PR. I suggest that you merge styles into a new object and not replace them.
2. This only covers cases where Reference is referencing a Graphic. It does not cover Groups. The example below might not work. So maybe not limit it to Graphics.

<svg>
<g id="myCircle">
<circle cx="50" cy="50" r="40" stroke="blue" stroke-width="10"/>
</g>
<use href="#myCircle" x="100" fill="blue"/>
</svg>

[dirkschulze]

Is this a typo?

[tjindal]

yes its typo! and i have fixed in latest one @dirkschulze

[dirkschulze]

no trailing whitespaces.

[dirkschulze]

I fell that those newlines are useful for readability. Can we keep them?

[tjindal]

yes, i have kept those newlines in latest one. Mistakenly removed last time. @dirkschulze

[dirkschulze]

The current object needs to take the graphic style of the parent into account to compute the resolved style for the current objet. For that we have a stack of graphic styles. The graphic style of the referencing object should be on the top of this stack.

In general the code should work as is already. Just like the group element, only that we know that we will just have one object. The question is why the style object of the reference object is not taken into account correctly when rendering the object that gets referenced. Could you debug please which style the object that gets referenced is taking into account? Is it taking the graphic style of its own parent? Do we accidentally break the inheritance instead of taking the graphic style of the parent (the reference object)?

Example (https://codepen.io/krit/pen/MWEYvYK):

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

Will the second rect be

1. yellow
2. black
3. green?

The expectation is that the first rect will be yellow and the second green. Should the second rect instead be:

1. yellow, the object that gets referenced takes the style of its parent in DOM into account
2. black, it means the style chain is broken and we take the default.
3. (green, everything works as expected.)

I'd suggest that you start with adding a new test with all the use cases we discussed earlier in this PR. The patch can not land w/o proper testing anyway. Then you can debug it based on the test.

[dirkschulze]

No space aligning please.

[tjindal]

in above example you mentioned , by doing recent changes output is showing 'green' which is desired one. As style object of reference object is taken into account by defining assignment operator , which pass on the fill and stroke properties if any from reference object to object that gets referenced.

i have removed the space aligning.

[tjindal]

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

in above example ,with the original code (not including any recent changes) the output is first rect is yellow and second is also yellow. even our expectation is that the first rect will be yellow and the second is black.
we have a stack of fill and stroke styles. the first rect takes fill style from parent . That FillStyle will be passed as a reference to function “ParseGraphicProperties” as it modifies FillStyle if any fill attribute found.currently in this scenario, it did not able to modify because no fill attribute is present. Then “Graphic” element with its FillStyle is pushed in stack. There is no way to distinguish its parent FillStyle and current one.

when reference("use") element is pointing to any graphic element, simply it will look up into map "mIdToElementMap" and if graphic element is found, it will further draw graphic element. inheritance is broken there after lookup from above map, all styling attributes which are applied to reference object they are ignored.
simply graphic element will be drawn as it was previously stored in stack. so “yellow” rect will be drawn.

i have also debug the area you pointed in discussion
auto saveRestore = SaveRestoreHelper{mRenderer, reference.graphicStyle};

it simple saves the graphic style , but fill and stroke properties are not stored anywhere which actually fills the rect.

[tjindal]

@dirkschulze can you pls share your thoughts on above comment ?

[dirkschulze]

This group handling will create another chain of issues. Especially with inherited properties that you don't handle. It also assumes that you just need to pass style information to the first child. However, there might be multiple nested groups and the styles need to get inherited through those groups as well.

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

In general, the <use> element is like another group with shifting its content to position x/y. Actually, the shifting should be identical to a translation via transform. The referenced content gets "copied in" here. I am quoting because we do not want to actually copy.

The intention was the following:

1. We have a reference object and the object that gets referenced by it
2. The reference object has styling applied the styling gets passed to the object that it references like we do for the group object.
3. The reference object triggers rendering just like the group does. So it renders the referenced object as normal.... the difference is the style object of the group.

So the code should already work as is. See overall comment.

[tjindal]

Thanks Dirk, i get your point of multiple nested groups if given, and i modified changes recently again to make code more generalised.

[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 :)