Issue 2685113002: Cleanup SVGElement::layoutObjectIsNeeded.

Issue 2685113002: Cleanup SVGElement::layoutObjectIsNeeded. (Closed)

Created:
3 years, 10 months ago by emilio

Modified:
3 years, 10 months ago

Reviewers:
fs, rune

CC:
chromium-reviews, krit, sof, eae+blinkwatch, blink-reviews-dom_chromium.org, dglazkov+blink, f(malita), blink-reviews, gyuyoung2, Stephen Chennney, kouhei+svg_chromium.org, pdr+svgwatchlist_chromium.org, rwlbuis, Manuel Rego

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Cleanup SVGElement::layoutObjectIsNeeded. This spawned off https://codereview.chromium.org/2684833003/, since it's now an unrelated cleanup. Review-Url: https://codereview.chromium.org/2685113002 Cr-Commit-Position: refs/heads/master@{#449618} Committed: https://chromium.googlesource.com/chromium/src/+/28ec0e0c2425d480693e8d40510b8093cef1f89f

Patch Set 1 #

Total comments: 1

Patch Set 2 : Preserving behavior #

Created: 3 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+20 lines, -16 lines)			Patch
M	third_party/WebKit/Source/core/dom/LayoutTreeBuilder.cpp	View		1 chunk	+0 lines, -12 lines	0 comments	Download
M	third_party/WebKit/Source/core/svg/SVGElement.h	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/svg/SVGElement.cpp	View	1	1 chunk	+15 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/svg/SVGGElement.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/core/svg/SVGSVGElement.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/core/svg/SVGTSpanElement.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/core/svg/SVGTextPathElement.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download

Messages

Total messages: 24 (10 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

emilio

Hey rune, this is the cleanup that ended up not being necessary from https://codereview.chromium.org/2684833003/, could ...

3 years, 10 months ago (2017-02-09 14:15:53 UTC) #2

rune

https://codereview.chromium.org/2685113002/diff/1/third_party/WebKit/Source/core/svg/SVGElement.cpp File third_party/WebKit/Source/core/svg/SVGElement.cpp (right): https://codereview.chromium.org/2685113002/diff/1/third_party/WebKit/Source/core/svg/SVGElement.cpp#newcode1034 third_party/WebKit/Source/core/svg/SVGElement.cpp:1034: !parentOrShadowHostElement()->isSVGElement())) { Shouldn't this have been the flat tree ...

3 years, 10 months ago (2017-02-09 18:09:28 UTC) #7

rune

On 2017/02/09 18:09:28, rune wrote: > https://codereview.chromium.org/2685113002/diff/1/third_party/WebKit/Source/core/svg/SVGElement.cpp > File third_party/WebKit/Source/core/svg/SVGElement.cpp (right): > > https://codereview.chromium.org/2685113002/diff/1/third_party/WebKit/Source/core/svg/SVGElement.cpp#newcode1034 > ...

3 years, 10 months ago (2017-02-09 18:17:01 UTC) #8

emilio

On 2017/02/09 18:17:01, rune wrote: > On 2017/02/09 18:09:28, rune wrote: > > > https://codereview.chromium.org/2685113002/diff/1/third_party/WebKit/Source/core/svg/SVGElement.cpp ...

3 years, 10 months ago (2017-02-09 21:29:16 UTC) #9

On 2017/02/09 18:17:01, rune wrote:
> On 2017/02/09 18:09:28, rune wrote:
> >
>
https://codereview.chromium.org/2685113002/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/svg/SVGElement.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2685113002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/svg/SVGElement.cpp:1034:
> > !parentOrShadowHostElement()->isSVGElement())) {
> > Shouldn't this have been the flat tree parent? Although, I don't know if
> there's
> > a way to use slot inside svg.
> 
> There is, actually. As long as one's careful to create the elements in the
right
> namespace. I think this might fail to show anything unless you look at the
flat
> tree parent:
> 
> <!DOCTYPE html>
> <div id="svgWrapper"></div>
> <script>
>     var svgNS = "http://www.w3.org/2000/svg";
>     var path = document.createElementNS(svgNS, "path");
>     path.setAttribute("d",
>
"M34.6,13.6L22.4,1.4C21.5,0.6,20.3,0.1,19,0H2C0.9,0,0,0.9,0,2v44c0,1.1,0.9,2,2,2h32c1.1,0,2-0.9,2-2V17
> C35.9,15.7,35.4,14.5,34.6,13.6z
> M32.5,15.7c0.1,0.1,0.1,0.2,0.2,0.3H21V4.2L32.5,15.7z
> M3,45V3h15v14c0,1.1,0.9,2,2,2h13v26H3z");
>     var slot = document.createElement("slot");
>     svgWrapper.appendChild(path);
>     var svgRoot = svgWrapper.attachShadow({mode:"open"});
>     svgRoot.innerHTML = '<svg version="1.1" viewBox="0 0 36 48" width="36"
> height="48"></svg>';
>     svgRoot.firstChild.appendChild(slot);
> </script>

Hmm, you're definitely right, and that works as of right now (intentionally or
not), but I'm not sure this is consistent with the rest of the SVG code, and
indeed I think it'll break in a lot of other situations.

I thought about this when writing this patch, but I saw that pretty much all the
parent-related checks in SVG code relate to the parentOrShadowHostElement
instead of the flat tree parent (so I assumed you just weren't able/supposed to
use shadow DOM in SVG).

That includes the same kind of check SVGGElement was using (which would be
inconsistent with the rest of SVG checks in current trunk, _not_ creating a
layout object for it right now when we should), and also the
customStyleForLayoutObject check, which we'd need to change too. There are
probably a lot of other inconsistent checks around the core/svg directory that
would need to be changed, like [1].

I just used parentOrShadowHostElement for consistency with those checks. I can
definitely use the flat tree parent for this specific check to restore the
precise behavior we have now, but as I said I believe this is not intended, and
it's buggy.

I'm happy to migrate them all to use the flat tree instead, I guess, though that
was out of scope for this patch initially. Maybe it's worth landing this and
I'll prepare a followup with your test case? 

Alternatively, just let me know if you want me to go through all the core/svg
directory replacing the appropriate calls in this patch.

emilio

On 2017/02/09 21:29:16, emilio wrote: > That includes the same kind of check SVGGElement was ...

3 years, 10 months ago (2017-02-09 21:32:17 UTC) #10

rune

I've added fs@ since this is mostly touching svg, and he would be more qualified ...

3 years, 10 months ago (2017-02-10 09:26:23 UTC) #13

On 2017/02/10 at 09:15:15, rune wrote: > I'd agree with ecobos that this working previously ...

3 years, 10 months ago (2017-02-10 09:27:06 UTC) #14

emilio

On 2017/02/10 09:27:06, fs wrote: > On 2017/02/10 at 09:15:15, rune wrote: > > > ...

3 years, 10 months ago (2017-02-10 12:05:58 UTC) #15

rune

On 2017/02/10 12:05:58, emilio wrote: > On 2017/02/10 09:27:06, fs wrote: > > On 2017/02/10 ...

3 years, 10 months ago (2017-02-10 12:55:09 UTC) #16

On 2017/02/10 at 12:55:09, rune wrote: > On 2017/02/10 12:05:58, emilio wrote: > > On ...

3 years, 10 months ago (2017-02-10 13:02:57 UTC) #17

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2685113002/20001

3 years, 10 months ago (2017-02-10 13:12:12 UTC) #20

emilio

On 2017/02/10 13:02:57, fs wrote: > On 2017/02/10 at 12:55:09, rune wrote: > > On ...

3 years, 10 months ago (2017-02-10 13:12:18 UTC) #21

commit-bot: I haz the power

3 years, 10 months ago (2017-02-10 14:40:20 UTC) #24

Message was sent while issue was closed.

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/28ec0e0c2425d480693e8d40510b...

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages