1469176 - Have nsDocumentViewer::CreateStyleSet load the SVG UA style sheet up front

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[Jonathan Watt [:jwatt]]

In bug 1317581 jaws wants to add some inline SVG to the <button> in the 'datetime-input-base' XBL binding for <input type=datetime> in toolkit/content/widgets/datetimebox.xml. That seems like something that we should encourage since embedding entire SVG images by reference is much more expensive than inline SVG.

The problem currently is that because the binding is applied during layout we end up trying to load the SVG UA sheet on-demand at that time. Loading style sheets during layout is not allowed.

The only reason that we don't currently load the SVG UA sheet in nsDocumentViewer::CreateStyleSet for non-SVG documents is as a minor optimization. svg.css is fairly trivial though, so it's really not that important an optimization. I'd be fine with just always loading svg.css (at least until we remove XBL support).

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8985801 [details] [diff] [review]
patch

Review of attachment 8985801 [details] [diff] [review]:
-----------------------------------------------------------------

This is fine, but I don't think it's complete.

Nothing prevents us from loading it again from SVGSVGElement::BindToTree, right?

So this would need guarding that with something like:

if (doc && doc->IsSVGDocument()) {
  // ...
}

Note that bug 1468133 will bitrot this (or vice versa), though resolving the conflict should be easy.

Comment 3

[Jonathan Watt [:jwatt]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #2)
> Nothing prevents us from loading it again from SVGSVGElement::BindToTree,
> right?

SVGSVGElement::BindToTree calls EnsureOnDemandBuiltInUASheet, which checks that it doesn't add it again. Seems like we should be using that here too.

> So this would need guarding that with something like:
> 
> if (doc && doc->IsSVGDocument()) {
>   // ...
> }

I don't follow this comment. We specifically want to add it to HTML documents, and adding this check would mean that we'd only add it to SVG documents...

Comment 4

[Emilio Cobos Álvarez (:emilio)]

(In reply to Jonathan Watt [:jwatt] from comment #3)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #2)
> > Nothing prevents us from loading it again from SVGSVGElement::BindToTree,
> > right?
> 
> SVGSVGElement::BindToTree calls EnsureOnDemandBuiltInUASheet, which checks
> that it doesn't add it again. Seems like we should be using that here too.

Sure, but nsDocumentViewer doesn't add the sheet to mOnDemandBuiltInUASheets, as you've noticed. We can either use that, or guard the SVGSVGElement bit as I suggested above.

> > So this would need guarding that with something like:
> > 
> > if (doc && doc->IsSVGDocument()) {
> >   // ...
> > }
> 
> I don't follow this comment. We specifically want to add it to HTML
> documents, and adding this check would mean that we'd only add it to SVG
> documents...

I meant guarding with this check before calling EnsureOnDemandBuiltInUASheet in SVGSVGElement, not nsDocumentViewer, sorry if I wasn't clear.

That should work, right? Right now there are three cases that concern us (after bug 1468133 there'd be two):

 (1) SVG-as-an-image: doesn't load svg.css upfront
 (2) SVG: doesn't load svg.css upfront
 (3) Other documents: they load svg.css upfront (with this patch).

Thus calling EnsureOnDemandBuiltInUASheet for IsSVGDocument() in SVGSVGElement::BindToTree should make this sound, by not treating it as an on-demand sheet for non-SVG document, and treating it as on-demand for SVG (which also includes svg-as-an-image).

I think these optimizations are mostly obsolete with bug 77999 and the equivalent Stylo optimization, btw, see my commit message in bug 1468133 about removing EnsureOnDemandBuiltInUASheet completely... I think we should just do that, and not load xul.css in content documents ever, but I want to check with cam after that bug lands.

Comment 5

[Phabricator Automation]

Attachment: Bug 1469176: Load svg.css upfront. r=jwatt

Comment 6

[Phabricator Automation]

Comment on attachment 8986956 [details]
Bug 1469176: Load svg.css upfront. r=jwatt

Jonathan Watt [:jwatt] (mostly away until Tue 10th) has approved the revision.

https://phabricator.services.mozilla.com/D1762

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/55740f35314d
Load svg.css upfront. r=jwatt

Comment 8

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/55740f35314d