Bug 1157592: Make InsertAnonymousContent load all the relevant stylesheets in SVG documents. r?bz draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Mon, 11 Jun 2018 04:45:10 +0200
changeset 807785 677e4418ab108cb566d73244d52e9da31437b9b9
parent 807714 0b5495dc100dd3bfda0886a4ad563a3c729c9b72
child 807786 efae8a63fbf304cb735ce14ba41627205bbb20da
push id113215
push userbmo:emilio@crisal.io
push dateFri, 15 Jun 2018 18:57:51 +0000
reviewersbz
bugs1157592, 1462742
milestone62.0a1
Bug 1157592: Make InsertAnonymousContent load all the relevant stylesheets in SVG documents. r?bz The underlying issue here is that the rule that makes the custom content container abspos is in ua.css, and we don't load that for SVG documents. Similarly, this fixes all the markers and such, that would be inline because of the lack of html.css etc. This fixes the root cause of bug 1462742, as well, so I added a few assertions that should replace the wallpaper there. The reason we're guaranteed to be oof is because top-layer implies that via StyleAdjuster::adjust_for_top_layer. As an aside, the fact that AccessibleCaret calls into InsertAnonymousContent from frame construction makes me extremely nervous, but it already does all sort of other pretty nasty stuff... I'll file and fix. MozReview-Commit-ID: 7ofKNGR8E20
dom/base/nsDocument.cpp
dom/svg/SVGDocument.cpp
dom/svg/SVGDocument.h
layout/generic/ViewportFrame.cpp
--- a/dom/base/nsDocument.cpp
+++ b/dom/base/nsDocument.cpp
@@ -243,16 +243,17 @@
 #include "nsWindowSizes.h"
 #include "mozilla/dom/Location.h"
 #include "mozilla/dom/FontFaceSet.h"
 #include "gfxPrefs.h"
 #include "nsISupportsPrimitives.h"
 #include "mozilla/ServoStyleSet.h"
 #include "mozilla/StyleSheet.h"
 #include "mozilla/StyleSheetInlines.h"
+#include "mozilla/dom/SVGDocument.h"
 #include "mozilla/dom/SVGSVGElement.h"
 #include "mozilla/dom/DocGroup.h"
 #include "mozilla/dom/TabGroup.h"
 #ifdef MOZ_XUL
 #include "mozilla/dom/ListBoxObject.h"
 #include "mozilla/dom/MenuBoxObject.h"
 #include "mozilla/dom/ScrollBoxObject.h"
 #include "mozilla/dom/TreeBoxObject.h"
@@ -4187,19 +4188,23 @@ nsIDocument::AddOnDemandBuiltInUASheet(S
   // Prepend here so that we store the sheets in mOnDemandBuiltInUASheets in
   // the same order that they should end up in the style set.
   mOnDemandBuiltInUASheets.InsertElementAt(0, aSheet);
 
   if (aSheet->IsApplicable()) {
     // This is like |AddStyleSheetToStyleSets|, but for an agent sheet.
     if (nsIPresShell* shell = GetShell()) {
       // Note that prepending here is necessary to make sure that html.css etc.
-      // do not override Firefox OS/Mobile's content.css sheet. Maybe we should
-      // have an insertion point to match the order of
+      // does not override Firefox OS/Mobile's content.css sheet.
+      //
+      // Maybe we should have an insertion point to match the order of
       // nsDocumentViewer::CreateStyleSet though?
+      //
+      // FIXME(emilio): We probably should, randomly prepending stuff here is
+      // very prone to subtle bugs, behavior differences...
       shell->StyleSet()->PrependStyleSheet(SheetType::Agent, aSheet);
       shell->ApplicableStylesChanged();
     }
   }
 
   NotifyStyleSheetAdded(aSheet, false);
 }
 
@@ -5388,16 +5393,22 @@ already_AddRefed<AnonymousContent>
 nsIDocument::InsertAnonymousContent(Element& aElement, ErrorResult& aRv)
 {
   nsIPresShell* shell = GetShell();
   if (!shell || !shell->GetCanvasFrame()) {
     aRv.Throw(NS_ERROR_UNEXPECTED);
     return nullptr;
   }
 
+  // We're about to insert random content here that will be rendered. We're
+  // going to need more than svg.css here...
+  if (IsSVGDocument()) {
+    AsSVGDocument()->EnsureNonSVGUserAgentStyleSheetsLoaded();
+  }
+
   nsAutoScriptBlocker scriptBlocker;
   nsCOMPtr<Element> container = shell->GetCanvasFrame()
                                      ->GetCustomContentContainer();
   if (!container) {
     aRv.Throw(NS_ERROR_UNEXPECTED);
     return nullptr;
   }
 
--- a/dom/svg/SVGDocument.cpp
+++ b/dom/svg/SVGDocument.cpp
@@ -103,21 +103,26 @@ SVGDocument::EnsureNonSVGUserAgentStyleS
     // style sheets in this case, but we'll need B2G/Fennec's
     // content.css. We could load all the sheets registered with the
     // nsIStyleSheetService (and maybe we should) but most likely it isn't
     // desirable or necessary for foreignObject in SVG-as-an-image. Instead we
     // only load the "agent-style-sheets" that nsStyleSheetService::Init()
     // pulls in from the category manager. That keeps memory use of
     // SVG-as-an-image down.
     //
-    // We do this before adding UASheet() etc. below because
-    // EnsureOnDemandBuiltInUASheet prepends, and B2G/Fennec's
-    // content.css must come after UASheet() etc.
+    // We do this before adding the other sheets below because
+    // EnsureOnDemandBuiltInUASheet prepends, and B2G/Fennec's/GeckoView's
+    // content.css must come after those UASheet() etc.
+    //
+    // FIXME(emilio, bug 1468133): We may already have loaded some of the other
+    // on-demand built-in UA sheets, including svg.css, so this looks somewhat
+    // bogus... Also, this should probably just use the stylesheet service which
+    // also has the right sheets cached and parsed here...
     nsCOMPtr<nsICategoryManager> catMan =
-    do_GetService(NS_CATEGORYMANAGER_CONTRACTID);
+      do_GetService(NS_CATEGORYMANAGER_CONTRACTID);
     if (catMan) {
       nsCOMPtr<nsISimpleEnumerator> sheets;
       catMan->EnumerateCategory("agent-style-sheets", getter_AddRefs(sheets));
       if (sheets) {
         bool hasMore;
         while (NS_SUCCEEDED(sheets->HasMoreElements(&hasMore)) && hasMore) {
           nsCOMPtr<nsISupports> sheet;
           if (NS_FAILED(sheets->GetNext(getter_AddRefs(sheet))))
--- a/dom/svg/SVGDocument.h
+++ b/dom/svg/SVGDocument.h
@@ -18,16 +18,17 @@ class SVGContextPaint;
 
 namespace dom {
 
 class SVGForeignObjectElement;
 
 class SVGDocument final : public XMLDocument
 {
   friend class SVGForeignObjectElement; // To call EnsureNonSVGUserAgentStyleSheetsLoaded
+  friend class nsIDocument; // Same reason.
 
 public:
   SVGDocument()
     : XMLDocument("image/svg+xml")
     , mHasLoadedNonSVGUserAgentStyleSheets(false)
   {
     mType = eSVG;
   }
--- a/layout/generic/ViewportFrame.cpp
+++ b/layout/generic/ViewportFrame.cpp
@@ -179,29 +179,20 @@ ViewportFrame::BuildDisplayListForTopLay
       BuildDisplayListForTopLayerFrame(aBuilder, frame, aList);
     }
   }
 
   nsIPresShell* shell = PresShell();
   if (nsCanvasFrame* canvasFrame = shell->GetCanvasFrame()) {
     if (Element* container = canvasFrame->GetCustomContentContainer()) {
       if (nsIFrame* frame = container->GetPrimaryFrame()) {
-        // Enter this frame for display list building, but only if it is
-        // actually a top layer frame. There is a bug affecting SVG documents
-        // that makes the custom content container not be a top layer frame in
-        // them, because SVG documents don't load `ua.css` when the custom
-        // content container is created. `ua.css` contains the rule that makes
-        // this a top layer frame. This bug is being fixed in bug 1157592.
-        // We have to do this workaround because otherwise we risk building
-        // display items for this frame twice; if the custom content container
-        // frame is not a top layer frame, it's not out-of-flow, so we'll have
-        // built display items for it already when we entered its parent frame.
-        if (frame->StyleDisplay()->mTopLayer != NS_STYLE_TOP_LAYER_NONE) {
-          BuildDisplayListForTopLayerFrame(aBuilder, frame, aList);
-        }
+        MOZ_ASSERT(frame->StyleDisplay()->mTopLayer != NS_STYLE_TOP_LAYER_NONE,
+                   "ua.css should ensure this");
+        MOZ_ASSERT(frame->GetStateBits() & NS_FRAME_OUT_OF_FLOW);
+        BuildDisplayListForTopLayerFrame(aBuilder, frame, aList);
       }
     }
   }
 }
 
 #ifdef DEBUG
 void
 ViewportFrame::AppendFrames(ChildListID     aListID,