1330051 - stylo: Serialize/reparse style attributes when moving nodes between documents with different style backends

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

Maybe the answer is "this should never happen", but right now it can happen pretty easily: adopting between HTML and SVG documents, applying an XBL document with inline styles to an HTML document (hello, <marquee>), etc.

If the long-term plan is to have this not happen, great; we can probably just wait for now.  If we plan to be shipping something while not all documents are on the same style backend, then we need to do something like serialize/reparse during adoption.

Comment 1

[Bobby Holley (:bholley)]

Once we make stylo support xul and svg, we shouldn't have a need to dynamically determine the style backend from the document type.

We will ship stylo as prefable, but I think we can probably get away with the pref being read at startup only. That would let us eliminate the per-document-ness of the style backend, and make this a non-issue.

Let's leave this on file to make sure we're actually doing that by the time we ship to nightly users.

Comment 2

[Bobby Holley (:bholley)]

Chris, how do you want to track bugs that block enabling on nightly? A metabug or a whiteboard tag?

Comment 3

[Chris Peterson [:cpeterson]]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #2)
> Chris, how do you want to track bugs that block enabling on nightly? A
> metabug or a whiteboard tag?

I have been meaning to create some meta bugs.

Comment 4

[Bobby Holley (:bholley)]

After talking with some people today, I think it's probably best to keep our options open for shipping stylo content-only. Given that, it seems worth fixing the adopt bug.

IIRC, Manish and Boris were talking about doing this today. Manish, is this bug where that work is happening, or somewhere else?

Comment 5

[Manish Goregaokar [:manishearth]]

That work is just part 6 of bug 1329093. That involves a very specific case of moving an associated entry in a hashmap across documents, I don't think that generalizes.

Comment 6

[Manish Goregaokar [:manishearth]]

I suspect we need to do this anyway, I was having trouble with adopting relative URIs across documents.

Comment 7

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1330051; Reparse style attribute when adopting across style backends;

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

Comment 8

[Manish Goregaokar [:manishearth]]

r?

Do we need to serialize/parse it or can we just re-parse from the attribute contents?

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

I don't think we store the original attribute content in the dom tree, do we?

Comment 10

[Manish Goregaokar [:manishearth]]

.getAttribute has to work, no? We seem to do this.

Comment 11

[Manish Goregaokar [:manishearth]]

Comment on attachment 8851139 [details]
Bug 1330051; Reparse style attribute when adopting across style backends;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123530/diff/1-2/

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Do we need to serialize/parse it or can we just re-parse from the attribute contents?

Just reparse.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8851139 [details]
Bug 1330051; Reparse style attribute when adopting across style backends;

https://reviewboard.mozilla.org/r/123530/#review126058

::: dom/base/Element.h:1161
(Diff revision 2)
>     * The new document can be reached via OwnerDoc().
>     */
> -  virtual void NodeInfoChanged(nsIDocument* aOldDoc)
> -  {
> -  }
> +  virtual void NodeInfoChanged(nsIDocument* aOldDoc) {}
> +
> +  /**
> +   * Will be called before NodeInfoChanged

Why can't we just handle this in NodeInfoChanged itself?  Subclasses would obviously need to call up to their superclass, but...

::: dom/base/Element.cpp:2598
(Diff revision 2)
>  }
>  
> +void
> +Element::ForceReparseStyleAttributeIfBackendChanged(nsIDocument* aOldDoc)
> +{
> +  if (OwnerDoc()->GetStyleBackendType() != aOldDoc->GetStyleBackendType()) {

Arguably, we should reparse even if the style backend didn't change, because base URLs changed.  But it's worth checking what the specs say.  And maybe what other browsers do.

The behavior difference being observable for adoption across backends vs adoption within a backend is a bit unfortunate...  I wonder whether we have any XBL anon content that uses relative URIs in style attributes.  I really hope not.  :(

::: dom/base/Element.cpp:2599
(Diff revision 2)
>  
> +void
> +Element::ForceReparseStyleAttributeIfBackendChanged(nsIDocument* aOldDoc)
> +{
> +  if (OwnerDoc()->GetStyleBackendType() != aOldDoc->GetStyleBackendType()) {
> +    if (MayHaveStyle() && !IsXULElement()) {

And then we could put this in nsStyledElement's NodeInfoChanged(), and not need the !IsXULElement() thing.

Though really, we should reparse for XULElement too...  Followup is OK there.

Comment 14

[Manish Goregaokar [:manishearth]]

> Why can't we just handle this in NodeInfoChanged itself?  Subclasses would obviously need to call up to their superclass, but...

That's what I was doing initially, but then subclasses have to call up, which isn't great. But then I can place it on nsStyledElement, so I'll do that.

> Arguably, we should reparse even if the style backend didn't change, because base URLs changed.  But it's worth checking what the specs say.  And maybe what other browsers do.

I was asking about this earlier -- Firefox currently does not reparse even when base URLs change, and neither does Chrome.

> Though really, we should reparse for XULElement too...  Followup is OK there.


Oh, it seemed like XULElement doesn't have content styles, but it does. By moving this to nsStyledElement that should get fixed.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> but then subclasses have to call up, which isn't great.

What's not great about it?  Serious question.

> Firefox currently does not reparse even when base URLs change, and neither does Chrome.

Right.  So we would not reparse on adopt if the style backend is unchaged, but would reparse if it's changed...

Hopefully this is a transient state and we won't actually ship with multiple style backends, but I'm not 100% sure we'll avoid that.  I don't have a better suggestion so far, though.  :(

Comment 16

[Manish Goregaokar [:manishearth]]

Comment on attachment 8851139 [details]
Bug 1330051; Reparse style attribute when adopting across style backends;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123530/diff/2-3/

Comment 17

[Manish Goregaokar [:manishearth]]

Addressed. r?

> What's not great about it?

Someone overriding it has to remember to call it basically. I added something about this to the documentation; should suffice.

> Right.  So we would not reparse on adopt if the style backend is unchaged, but would reparse if it's changed...

Yeah, but I suspect the use case of adopting relative URIs in style attrs across backends is rare. I doubt it's specced, at any rate.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> Yeah, but I suspect the use case of adopting relative URIs in style attrs across backends is rare.

I really hope so...

> I doubt it's specced, at any rate.

It's specced.  Parsing happens when the attribute is added or its value is changed, per <https://html.spec.whatwg.org/multipage/dom.html#the-style-attribute>.  So technically this reparsing behavior is a spec violation.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8851139 [details]
Bug 1330051; Reparse style attribute when adopting across style backends;

https://reviewboard.mozilla.org/r/123530/#review126064

HTMLImageElement::NodeInfoChanged needs to call nsGenericHTMLElement::NodeInfoChanged.

Same for nsGenericHTMLFormElementWithState::NodeInfoChanged.

r=me with those fixed and the nits below addressed.

::: dom/base/Element.h:1158
(Diff revision 3)
>     * Called when we have been adopted, and the information of the
>     * node has been changed.
>     *
>     * The new document can be reached via OwnerDoc().
> +   *
> +   * If you inherit from nsStyledElement and override this method,

Fwiw, I think people overriding methods like this should as a matter of course call their superclass.  If we want to document this, we should just say to do that here for all Element subclasses, without reference to nsStyledElement.

::: dom/svg/nsSVGElement.cpp:912
(Diff revision 3)
>  }
>  
>  void
>  nsSVGElement::NodeInfoChanged(nsIDocument* aOldDoc)
>  {
> +  nsStyledElement::NodeInfoChanged(aOldDoc);

This should be nsSVGElementBase::NodeInfoChanged(aOldDoc).

Comment 20

[Manish Goregaokar [:manishearth]]

Comment on attachment 8851139 [details]
Bug 1330051; Reparse style attribute when adopting across style backends;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123530/diff/3-4/

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d4bde3419b68
; Reparse style attribute when adopting across style backends; r=bz

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d4bde3419b68