Fix textpath is not displayed on use element

Fix textpath is not displayed on use element

If textpath on use element that don't has path on Treescope,
the content of <textPath> element is not displayed.
This reason is that search <path> element on the shadow root.
Because use Treescope of the real element for the id lookup.

BUG=505546
Committed: https://crrev.com/a5e8a760a9457ad814990c5cad81c6dba79d141f
Cr-Commit-Position: refs/heads/master@{#381447}

[hyunjunekim2, 2016-03-13 08:13:29]

Description was changed from

==========
WIP

BUG=505546
==========

to

==========
Fix textpath is not displayed on use element 

...

BUG=505546
==========

[hyunjunekim2, 2016-03-13 09:33:46]

Description was changed from

==========
Fix textpath is not displayed on use element 

...

BUG=505546
==========

to

==========
Fix textpath is not displayed on use element 

If textpath on use element that don't has path on Treescope,
the content of text path is not displayed.

BUG=505546
==========

[hyunjunekim2, 2016-03-13 09:35:39]

Description was changed from

==========
Fix textpath is not displayed on use element 

If textpath on use element that don't has path on Treescope,
the content of text path is not displayed.

BUG=505546
==========

to

==========
Fix textpath is not displayed on use element 

If textpath on use element that don't has path on Treescope(shadow tree),
the content of text path is not displayed.

BUG=505546
==========

[hyunjunekim2, 2016-03-13 09:48:41]

Description was changed from

==========
Fix textpath is not displayed on use element 

If textpath on use element that don't has path on Treescope(shadow tree),
the content of text path is not displayed.

BUG=505546
==========

to

==========
Fix textpath is not displayed on use element 

If textpath on use element that don't has path on Treescope(shadow tree),
the content of text path is not displayed.
Because shouldn't search in the same coordinate system as the current <text>
element.

BUG=505546
==========

[hyunjunekim2, 2016-03-13 09:49:23]

Description was changed from

==========
Fix textpath is not displayed on use element 

If textpath on use element that don't has path on Treescope(shadow tree),
the content of text path is not displayed.
Because shouldn't search in the same coordinate system as the current <text>
element.

BUG=505546
==========

to

==========
Fix textpath is not displayed on use element 

If textpath on use element that don't has path on Treescope(shadow tree),
the content of text path is not displayed.
Because shouldn't search in the same coordinate system as
the current <text> element.

BUG=505546
==========

[hyunjunekim2, 2016-03-13 09:53:44]

Description was changed from

==========
Fix textpath is not displayed on use element 

If textpath on use element that don't has path on Treescope(shadow tree),
the content of text path is not displayed.
Because shouldn't search in the same coordinate system as
the current <text> element.

BUG=505546
==========

to

==========
Fix textpath is not displayed on use element 

If textpath on use element that don't has path on Treescope,
the content of text path is not displayed.
Because shouldn't search in the same coordinate system
as the current <text> element.

BUG=505546
==========

[hyunjunekim2, 2016-03-13 09:55:37]

Description was changed from

==========
Fix textpath is not displayed on use element 

If textpath on use element that don't has path on Treescope,
the content of text path is not displayed.
Because shouldn't search in the same coordinate system
as the current <text> element.

BUG=505546
==========

to

==========
Fix textpath is not displayed on use element 

If textpath on use element that don't has path on Treescope,
the content of text path is not displayed.
Because shouldn't search in the same coordinate system
as the current <text> element.

And the path data coordinates within the referenced <path> element
are assumed to be in the same coordinate system as the current <text> element,
not in the coordinate system where the <path> element is defined[1].

[1]

BUG=505546
==========

[hyunjunekim2, 2016-03-13 09:56:32]

Description was changed from

==========
Fix textpath is not displayed on use element 

If textpath on use element that don't has path on Treescope,
the content of text path is not displayed.
Because shouldn't search in the same coordinate system
as the current <text> element.

And the path data coordinates within the referenced <path> element
are assumed to be in the same coordinate system as the current <text> element,
not in the coordinate system where the <path> element is defined[1].

[1]

BUG=505546
==========

to

==========
Fix textpath is not displayed on use element 

If textpath on use element that don't has path on Treescope,
the content of text path is not displayed.
Because shouldn't search in the same coordinate system
as the current <text> element.

And the path data coordinates within the referenced <path> element
are assumed to be in the same coordinate system as the current <text>
element, not in the coordinate system where the <path> 
element is defined[1].

[1]

BUG=505546
==========

[hyunjunekim2, 2016-03-13 09:56:57]

Description was changed from

==========
Fix textpath is not displayed on use element 

If textpath on use element that don't has path on Treescope,
the content of text path is not displayed.
Because shouldn't search in the same coordinate system
as the current <text> element.

And the path data coordinates within the referenced <path> element
are assumed to be in the same coordinate system as the current <text>
element, not in the coordinate system where the <path> 
element is defined[1].

[1]

BUG=505546
==========

to

==========
Fix textpath is not displayed on use element 

If textpath on use element that don't has path on Treescope,
the content of text path is not displayed.
Because shouldn't search in the same coordinate system
as the current <text> element.

And the path data coordinates within the referenced <path> element
are assumed to be in the same coordinate system as the current <text>
element, not in the coordinate system where the <path> element
is defined[1].

[1]

BUG=505546
==========

[hyunjunekim2, 2016-03-13 09:57:19]

Description was changed from

==========
Fix textpath is not displayed on use element 

If textpath on use element that don't has path on Treescope,
the content of text path is not displayed.
Because shouldn't search in the same coordinate system
as the current <text> element.

And the path data coordinates within the referenced <path> element
are assumed to be in the same coordinate system as the current <text>
element, not in the coordinate system where the <path> element
is defined[1].

[1]

BUG=505546
==========

to

==========
Fix textpath is not displayed on use element 

If textpath on use element that don't has path on Treescope,
the content of text path is not displayed.
Because shouldn't search in the same coordinate system
as the current <text> element.

And the path data coordinates within the referenced <path> element
are assumed to be in the same coordinate system as the current <text>
element, not in the coordinate system where the <path> element
is defined[1].

[1] https://www.w3.org/TR/SVG/text.html#TextPathElementHrefAttribute

BUG=505546
==========

[hyunjunekim2, 2016-03-13 10:01:25]

hyunjune.kim@samsung.com changed reviewers:
+ fs@opera.com, pdr@chromium.org

[hyunjunekim2, 2016-03-13 10:02:01]

fs, pdr
Could you check PS3?

[fs, 2016-03-14 12:54:10]

I think the approach look reasonable, but I'd like to hear pdr's opinion on it
too.

> Because shouldn't search in the same coordinate system
> as the current <text> element.
> 
> And the path data coordinates within the referenced <path> element
> are assumed to be in the same coordinate system as the current <text>
> element, not in the coordinate system where the <path> element
> is defined[1].
> 
> [1] https://www.w3.org/TR/SVG/text.html#TextPathElementHrefAttribute

None of this is relevant to the problem or the fix. Suggest you remove and
replace with something that is relevant. (I.e which tree scope is used for the
id lookup.)

https://codereview.chromium.org/1792673005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGTextPath.cpp (right):

https://codereview.chromium.org/1792673005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGTextPath.cpp:64: if
(textPathElement.inUseShadowTree()) {
I think we could simplify this a bit (and preferably move it to a helper so that
we can pick one TreeScope& or the other:

TreeScope& treeScopeForIdResolution(SVGElement& element)
{
    if (SVGElement* correspondingElement = element.correspondingElement())
        return correspondingElement->treeScope();
    return element.treeScope();
}

https://codereview.chromium.org/1792673005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGTextPathElement.cpp (right):

https://codereview.chromium.org/1792673005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGTextPathElement.cpp:129: if
(inUseShadowTree()) {
Not sure this is actually needed. When the "real" element changes it will notify
the instance - which will then update. If anything we should probably just do
nothing here if this is an instance...

[hyunjunekim2, 2016-03-14 14:48:41]

Description was changed from

==========
Fix textpath is not displayed on use element 

If textpath on use element that don't has path on Treescope,
the content of text path is not displayed.
Because shouldn't search in the same coordinate system
as the current <text> element.

And the path data coordinates within the referenced <path> element
are assumed to be in the same coordinate system as the current <text>
element, not in the coordinate system where the <path> element
is defined[1].

[1] https://www.w3.org/TR/SVG/text.html#TextPathElementHrefAttribute

BUG=505546
==========

to

==========
Fix textpath is not displayed on use element 

If textpath on use element that don't has path on Treescope,
the content of text path is not displayed.
Because shouldn't search in the same coordinate system
as the current <text> element.


BUG=505546
==========

[hyunjunekim2, 2016-03-14 14:48:50]

Description was changed from

==========
Fix textpath is not displayed on use element 

If textpath on use element that don't has path on Treescope,
the content of text path is not displayed.
Because shouldn't search in the same coordinate system
as the current <text> element.


BUG=505546
==========

to

==========
Fix textpath is not displayed on use element 

If textpath on use element that don't has path on Treescope,
the content of text path is not displayed.
Because shouldn't search in the same coordinate system
as the current <text> element.

BUG=505546
==========

[hyunjunekim2, 2016-03-15 10:18:35]

Description was changed from

==========
Fix textpath is not displayed on use element 

If textpath on use element that don't has path on Treescope,
the content of text path is not displayed.
Because shouldn't search in the same coordinate system
as the current <text> element.

BUG=505546
==========

to

==========
Fix textpath is not displayed on use element 

If textpath on use element that don't has path on Treescope,
the content of <textpath> element is not displayed.
Because searched <path> element on the shadow root.

BUG=505546
==========

[hyunjunekim2, 2016-03-15 10:20:17]

Description was changed from

==========
Fix textpath is not displayed on use element 

If textpath on use element that don't has path on Treescope,
the content of <textpath> element is not displayed.
Because searched <path> element on the shadow root.

BUG=505546
==========

to

==========
Fix textpath is not displayed on use element 

If textpath on use element that don't has path on Treescope,
the content of <textPath> element is not displayed.
This reason is that search <path> element on the shadow root.
Because

BUG=505546
==========

[hyunjunekim2, 2016-03-15 10:21:35]

Description was changed from

==========
Fix textpath is not displayed on use element 

If textpath on use element that don't has path on Treescope,
the content of <textPath> element is not displayed.
This reason is that search <path> element on the shadow root.
Because

BUG=505546
==========

to

==========
Fix textpath is not displayed on use element 

If textpath on use element that don't has path on Treescope,
the content of <textPath> element is not displayed.
This reason is that search <path> element on the shadow root.
Because use Treescope of the real element.

BUG=505546
==========

[hyunjunekim2, 2016-03-15 10:22:27]

Description was changed from

==========
Fix textpath is not displayed on use element 

If textpath on use element that don't has path on Treescope,
the content of <textPath> element is not displayed.
This reason is that search <path> element on the shadow root.
Because use Treescope of the real element.

BUG=505546
==========

to

==========
Fix textpath is not displayed on use element 

If textpath on use element that don't has path on Treescope,
the content of <textPath> element is not displayed.
This reason is that search <path> element on the shadow root.
Because use Treescope of the real element for the id lookup.

BUG=505546
==========

[hyunjunekim2, 2016-03-15 23:11:32]

pdr,
Could you check this patch?
Thank you.

https://codereview.chromium.org/1792673005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGTextPath.cpp (right):

https://codereview.chromium.org/1792673005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGTextPath.cpp:64: if
(textPathElement.inUseShadowTree()) {
On 2016/03/14 12:54:10, fs (traveling) wrote:
> I think we could simplify this a bit (and preferably move it to a helper so
that
> we can pick one TreeScope& or the other:
> 
> TreeScope& treeScopeForIdResolution(SVGElement& element)
> {
>     if (SVGElement* correspondingElement = element.correspondingElement())
>         return correspondingElement->treeScope();
>     return element.treeScope();
> }

Done.

https://codereview.chromium.org/1792673005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGTextPathElement.cpp (right):

https://codereview.chromium.org/1792673005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGTextPathElement.cpp:129: if
(inUseShadowTree()) {
On 2016/03/14 12:54:10, fs (traveling) wrote:
> Not sure this is actually needed. When the "real" element changes it will
notify
> the instance - which will then update. If anything we should probably just do
> nothing here if this is an instance...

Change before.

[pdr., 2016-03-16 05:16:34]

On 2016/03/15 at 23:11:32, hyunjune.kim wrote:
> pdr,
> Could you check this patch?
> Thank you.
> 
>
https://codereview.chromium.org/1792673005/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/svg/LayoutSVGTextPath.cpp (right):
> 
>
https://codereview.chromium.org/1792673005/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/svg/LayoutSVGTextPath.cpp:64: if
(textPathElement.inUseShadowTree()) {
> On 2016/03/14 12:54:10, fs (traveling) wrote:
> > I think we could simplify this a bit (and preferably move it to a helper so
that
> > we can pick one TreeScope& or the other:
> > 
> > TreeScope& treeScopeForIdResolution(SVGElement& element)
> > {
> >     if (SVGElement* correspondingElement = element.correspondingElement())
> >         return correspondingElement->treeScope();
> >     return element.treeScope();
> > }
> 
> Done.
> 
>
https://codereview.chromium.org/1792673005/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/svg/SVGTextPathElement.cpp (right):
> 
>
https://codereview.chromium.org/1792673005/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/svg/SVGTextPathElement.cpp:129: if
(inUseShadowTree()) {
> On 2016/03/14 12:54:10, fs (traveling) wrote:
> > Not sure this is actually needed. When the "real" element changes it will
notify
> > the instance - which will then update. If anything we should probably just
do
> > nothing here if this is an instance...
> 
> Change before.

LGTM

[hyunjunekim2, 2016-03-16 12:09:00]

pdr,
Go ahead.

[hyunjunekim2, 2016-03-16 12:09:05]

The CQ bit was checked by hyunjune.kim@samsung.com

[commit-bot: I haz the power, 2016-03-16 12:09:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1792673005/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1792673005/100001

[commit-bot: I haz the power, 2016-03-16 13:47:18]

Description was changed from

==========
Fix textpath is not displayed on use element 

If textpath on use element that don't has path on Treescope,
the content of <textPath> element is not displayed.
This reason is that search <path> element on the shadow root.
Because use Treescope of the real element for the id lookup.

BUG=505546
==========

to

==========
Fix textpath is not displayed on use element 

If textpath on use element that don't has path on Treescope,
the content of <textPath> element is not displayed.
This reason is that search <path> element on the shadow root.
Because use Treescope of the real element for the id lookup.

BUG=505546
==========

[commit-bot: I haz the power, 2016-03-16 13:47:19]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-03-16 13:48:53]

Description was changed from

==========
Fix textpath is not displayed on use element 

If textpath on use element that don't has path on Treescope,
the content of <textPath> element is not displayed.
This reason is that search <path> element on the shadow root.
Because use Treescope of the real element for the id lookup.

BUG=505546
==========

to

==========
Fix textpath is not displayed on use element

If textpath on use element that don't has path on Treescope,
the content of <textPath> element is not displayed.
This reason is that search <path> element on the shadow root.
Because use Treescope of the real element for the id lookup.

BUG=505546

Committed: https://crrev.com/a5e8a760a9457ad814990c5cad81c6dba79d141f
Cr-Commit-Position: refs/heads/master@{#381447}
==========

[commit-bot: I haz the power, 2016-03-16 13:48:54]

Patchset 6 (id:??) landed as
https://crrev.com/a5e8a760a9457ad814990c5cad81c6dba79d141f
Cr-Commit-Position: refs/heads/master@{#381447}