Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
6 months ago
3 months ago

People

(Reporter: bz, Assigned: manishearth)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

6 months ago
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.
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.
Chris, how do you want to track bugs that block enabling on nightly? A metabug or a whiteboard tag?
Flags: needinfo?(cpeterson)
Summary: stylo: sort out what should happen with inline style when an element is adopted across documents with different style backend types → stylo: Determine the style backend for all documents at application startup
(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.
Flags: needinfo?(cpeterson)
Blocks: 1330412
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?
Flags: needinfo?(manishearth)
(Assignee)

Comment 5

5 months ago
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.
Flags: needinfo?(manishearth)
Priority: -- → P3
Summary: stylo: Determine the style backend for all documents at application startup → stylo: Serialize/reparse style attributes when moving nodes between documents with different style backends
(Assignee)

Comment 6

5 months ago
I suspect we need to do this anyway, I was having trouble with adopting relative URIs across documents.
Priority: P3 → P1
(Assignee)

Updated

5 months ago
Assignee: nobody → manishearth
Status: NEW → ASSIGNED
Blocks: 1338964
Comment hidden (mozreview-request)
(Assignee)

Comment 8

4 months ago
r?

Do we need to serialize/parse it or can we just re-parse from the attribute contents?
Flags: needinfo?(bzbarsky)
I don't think we store the original attribute content in the dom tree, do we?
(Assignee)

Comment 10

4 months ago
.getAttribute has to work, no? We seem to do this.
Comment hidden (mozreview-request)
(Reporter)

Comment 12

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

Just reparse.
Flags: needinfo?(bzbarsky)
(Reporter)

Comment 13

4 months ago
mozreview-review
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.
(Assignee)

Comment 14

4 months ago
> 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.
(Reporter)

Comment 15

4 months ago
> 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 hidden (mozreview-request)
(Assignee)

Comment 17

4 months ago
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.
Flags: needinfo?(bzbarsky)
(Reporter)

Comment 18

4 months ago
> 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.
Flags: needinfo?(bzbarsky)
(Reporter)

Comment 19

4 months ago
mozreview-review
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).
Attachment #8851139 - Flags: review+
Comment hidden (mozreview-request)

Comment 21

4 months ago
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d4bde3419b68
; Reparse style attribute when adopting across style backends; r=bz
https://hg.mozilla.org/mozilla-central/rev/d4bde3419b68
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
status-firefox53: affected → ---
You need to log in before you can comment on or make changes to this bug.