1440964 - nsSMILCSSValue could interact a bit better with Shadow DOM.

Status: ASSIGNED

(Core :: SVG, enhancement, P3)

Description

[Emilio Cobos Álvarez [:emilio]]

.

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1440964: Make nsSMILCSSValueType interact slightly better with Shadow DOM.

In particular, use the composed doc (not the uncomposed DOC) so SMIL can work in
Shadow trees.

Also, this is consistent with all the machinery nsComputedDOMStyle and other
places use (GetPresShellForContent).

Review commit: https://reviewboard.mozilla.org/r/222996/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/222996/

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 8953795 [details]
Bug 1440964: Make nsSMILCSSValueType interact slightly better with Shadow DOM.

https://reviewboard.mozilla.org/r/222996/#review229356

r- for now, mostly because I want to have a clearer mental model about point (3) below:

::: commit-message-14f46:3
(Diff revision 1)
> +In particular, use the composed doc (not the uncomposed DOC) so SMIL can work in
> +Shadow trees.
> +

(1) s/DOC/doc/

(2) Please add just after the parenthetical:
  "...for 'are inline styles allowed' CSP checks..."
(since those are the changes involved here)

(3) For the other change (in ValueFromStringHelper), why are we specifically changing to OwnerDoc() there, when we're changing to ComposedDoc() elsewhere?

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8953795 [details]
Bug 1440964: Make nsSMILCSSValueType interact slightly better with Shadow DOM.

Re-requesting review per the comments below, but feel free to re-r- if my comments don't make sense or the explanations are insufficient.

Thanks for taking a look!

(In reply to Daniel Holbert [:dholbert] from comment #2)
> Comment on attachment 8953795 [details]
> Bug 1440964: Make nsSMILCSSValueType interact slightly better with Shadow
> DOM.
> 
> https://reviewboard.mozilla.org/r/222996/#review229356
> 
> r- for now, mostly because I want to have a clearer mental model about point
> (3) below:
> 
> ::: commit-message-14f46:3
> (Diff revision 1)
> > +In particular, use the composed doc (not the uncomposed DOC) so SMIL can work in
> > +Shadow trees.
> > +
> 
> (1) s/DOC/doc/
> 
> (2) Please add just after the parenthetical:
>   "...for 'are inline styles allowed' CSP checks..."
> (since those are the changes involved here)

Well, not _just_ that, it's more subtle. The existing GetPresContextForElement is replaced for GetPresShellForContent, which checks the composed doc instead of the uncomposed one. That means that before these changes an element in a Shadow Tree would bailout early.

That's also the reason I asserted instead of null-checking the CSP stuff (because in practice the document was always non-null, due to the GetPresContextForElement bailout), and because I don't think if someone changes something they want to bypass a security check accidentally.

> (3) For the other change (in ValueFromStringHelper), why are we specifically
> changing to OwnerDoc() there, when we're changing to ComposedDoc() elsewhere?

Because the only reason that function cares about the document for is for CSS parsing, and CSS parsing is well-defined in any document.

Furthermore, the only caller of that function is the other function I'm touching, which bails out for elements outside of the composed doc. I can MOZ_ASSERT(aElement->IsInComposedDoc()) in that function, if you think that's worth it.

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #3)
> > (2) Please add just after the parenthetical:
> >   "...for 'are inline styles allowed' CSP checks..."
> > (since those are the changes involved here)
> 
> Well, not _just_ that, it's more subtle. The existing
> GetPresContextForElement is replaced for GetPresShellForContent,


Fair enough, OK. Never mind about (2) then.

> > (3) For the other change (in ValueFromStringHelper), why are we specifically
> > changing to OwnerDoc() there, when we're changing to ComposedDoc() elsewhere?
> 
> Because the only reason that function cares about the document for is for
> CSS parsing, and CSS parsing is well-defined in any document.

Flipping the question on its head, then: why is OwnerDoc() *not appropriate* for the other checks? (if it's fine here)

(i.e. why do we care about the composed-ness of the document for CSP checks, but we don't need to care when creating a parser context?)

> Furthermore, the only caller of that function is the other function I'm
> touching, which bails out for elements outside of the composed doc. I can
> MOZ_ASSERT(aElement->IsInComposedDoc()) in that function, if you think
> that's worth it.

Might be worth adding an assertion like that, at least from the perspective of satisfying readers' concerns about "what's the significance of us using subtly different Get-The-Document APIs for these two separate things".

(Presumably this assertion would be proving that OwnerDoc() is strictly an optimization/simplification, I think, and that GetComposedDoc() would produce the same result but with a few more checks?)

Comment 5

[Daniel Holbert [:dholbert]]

(note: part of the problem here is that these APIs are really vaguely documented, which I filed bug 1441277 about. That's why I want to be sure I understand the subtleties of mixing/swapping them here.)

Comment 6

[Emilio Cobos Álvarez [:emilio]]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> Flipping the question on its head, then: why is OwnerDoc() *not appropriate*
> for the other checks? (if it's fine here)

In general the computed style of an element is not well defined in a bunch of cases, and "doesn't belong to the composed doc" is one of them (the other being "isn't in the flattened tree").

See https://github.com/w3c/csswg-drafts/issues/1548 / https://github.com/w3c/csswg-drafts/issues/1964.

That being said, we do support getting styles of stuff out of the document and such (and we use it in a couple places). Anyway, the point is, by default you only want to do layout-y stuff if you're connected to the document, accounting for shadow trees (which is what "in composed doc" means). Given this code was already bailing out for unconnected nodes, it seemed reasonable to keep doing that.

On an after thought it was maybe not done on purpose, and nsSMILCSSValueType may be able to work just fine for nodes not in a document, but that's a bigger change than this.

> (i.e. why do we care about the composed-ness of the document for CSP checks,
> but we don't need to care when creating a parser context?)

It's not only for CSP, it's (more importantly) for doing layout stuff, like getting a shell / pres context.

> > Furthermore, the only caller of that function is the other function I'm
> > touching, which bails out for elements outside of the composed doc. I can
> > MOZ_ASSERT(aElement->IsInComposedDoc()) in that function, if you think
> > that's worth it.
> 
> Might be worth adding an assertion like that, at least from the perspective
> of satisfying readers' concerns about "what's the significance of us using
> subtly different Get-The-Document APIs for these two separate things".

Will do.

> (Presumably this assertion would be proving that OwnerDoc() is strictly an
> optimization/simplification, I think, and that GetComposedDoc() would
> produce the same result but with a few more checks?)

Indeed.

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
> > (i.e. why do we care about the composed-ness of the document for CSP checks,
> > but we don't need to care when creating a parser context?)
> 
> It's not only for CSP, it's (more importantly) for doing layout stuff, like
> getting a shell / pres context.

But those other calls -- for layout stuff -- aren't *in this file*.  They're abstracted away inside of a helper function that's implemented elsewhere, and whose returned doc is never seen by this code.  I'm fine trusting that that helper function knows what it's doing.

I'm focusing on the differing needs of the calls that you're directly changing *in this file*, which IIUC are just for CSP for parser context, and I'm questioning whether those use-cases actually have different expectations or not.

Anyway -- it sounds like the right resolution for the time being is to be consistent about using GetComposedDoc(), and to call out the one inconsistent call (to "OwnerDoc") with a comment/assertion to point out that it's really just an optimization because we know the two functions are equivalent if we get there.

Comment 8

[Jet Villegas (inactive)]

[Triage 2018/03/23 - P3]