Closed Bug 1437956 Opened 7 years ago Closed 6 years ago

Recreate XML Pretty Print UI using shadow DOM instead of XBL

Categories

(Core :: XML, task, P3)

Product:
Core
Component:
XML
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: bgrins, Assigned: timdream)

References

Details

(Whiteboard: [xbl-in-content])

Attachments

(1 file)

Bug 1437956 - Pretty print XML with Shadow DOM
59 bytes, text/x-review-board-request
smaug
: review+
Details 
+++ This bug was initially created as a clone of Bug #1432636 +++

In Bug 1432636 we are planning to remove the JS handler needed in XBL for XML pretty printing. Once this is gone we can look for another way to put the HTML over the page's content.

One option may be to use Native Anonymous Content. From https://bugzilla.mozilla.org/show_bug.cgi?id=1432636#c16:

> We could have something like nsIDocument::InsertAnonymousContent but only
> callable from C++ that just takes the given node as-is instead of cloning. 
> We'd need to fix nsXMLPrettyPrinter::Unhook to work too, of course.
Blocks: war-on-xbl-in-content
Priority: -- → P3
If bug 1451983 gets implemented, then we might possibly be able to re-use some of the code from that.
See Also: → 1451983
See Also: 1451983 → 1356397
As a distraction, I looked at this bug yesterday.

I think it is possible to replace all the feature used in the XBL binding with the following:

1) Attempt to bind the transformed DOM as an anonymous tree directly from nsXMLPrettyPrinter::PrettyPrint()
2) Apply a UA stylesheet to the document with, simply, |:root > *:not(:-moz-native-anonymous) { display: none }|

and undo both when Unhook() happens.

So far I've failed to do (1). I can do AppendChildTo/AppendChild etc. to put the document fragment as (and/or <div id="top"> from XSLT) into the DOM as a non-anonymous tree, but these method does not allow me to bind anonymous tree with SetIsNativeAnonymousRoot() set. I tried to call BindToTree() directly there but it doesn't seem to actually put the DOM into the tree. I don't understand why that didn't work yet given that's what exactly nsXBLBinding::BindAnonymousContent() do ...

BTW, nsIDocument::InsertAnonymousContent() can't be used there because we don't have a canvas frame yet.

As a desperate attempt, I ended up putting the document fragment into a Shadow DOM and it worked. I can also hide the real DOM directly by not putting any <slot/> in the shadow DOM. However, I couldn't find any way to style the Shadow DOM other than the inline <style>; using @import in the <style> block will hit another assertion. I will also lose the ability switch to XMLMonoPrint.css with that.

So it remains to find out what's needed for BindToTree to work there...
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #2)
> As a desperate attempt, I ended up putting the document fragment into a
> Shadow DOM and it worked. I can also hide the real DOM directly by not
> putting any <slot/> in the shadow DOM. However, I couldn’t find any way to
> style the Shadow DOM other than the inline <style>; using @import in the
> <style> block will hit another assertion. I will also lose the ability
> switch to XMLMonoPrint.css with that.

That’s because of bug 1410578.
Depends on: 1410578
See Also: 1356397 → 
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #2)
> As a desperate attempt, I ended up putting the document fragment into a
> Shadow DOM and it worked. I can also hide the real DOM directly by not
> putting any <slot/> in the shadow DOM. However, I couldn't find any way to
> style the Shadow DOM other than the inline <style>; using @import in the
> <style> block will hit another assertion.

Should be fixed in bug 1464936. If it's not please file. Which assertion is it?
(In reply to ExE Boss from comment #3)
> That’s because of bug 1410578.

Thank for linking. I wasn't in luck yesterday to find the bug :) I am still not sure which is the right way to go (NAC v.s. Shadow DOM), so perhaps we don't really need that bug fixed.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #4)
> Should be fixed in bug 1464936. If it's not please file. Which assertion is
> it?

Will rebase and try again. Thanks for noting that. It was not the assert in that bug; I didn't keep the log though...
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #5)
> Will rebase and try again. Thanks for noting that. It was not the assert in
> that bug; I didn't keep the log though...

I no longer met with an assertion. Will clean up my changes and put it up.

That does, however, mean removing XMLMonoPrint.css as a feature. I don't know if we want that.
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #9)
> Another problem with the Shadow DOM approach is that the closed shadow root
> can be overwritten by content script if the root element meets the allowed
> condition, i.e. with an allowed tag name in HTML namespace, like this XML
> document: <div
> xmlns="http://www.w3.org/1999/xhtml"><child>foobar</child></div>.
> 
> This is a showstopper I guess...

How? If there's already a shadow root, calling attachShadow throws. Also, can XML documents run script? (not sure, I fear the answer is "yes" but..)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #10)
> > This is a showstopper I guess...
> 
> How? If there's already a shadow root, calling attachShadow throws. Also,
> can XML documents run script? (not sure, I fear the answer is "yes" but..)

No, calling attachShadow() won't throw but would replace the current shadow root, whereas there is no way to access the NAC from the content script. This can be verified from DevTools from the page.

The XML document cannot run any script, but same-origin script can tap into the window of the document.
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #11)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #10)
> > > This is a showstopper I guess...
> > 
> > How? If there's already a shadow root, calling attachShadow throws. Also,
> > can XML documents run script? (not sure, I fear the answer is "yes" but..)
> 
> No, calling attachShadow() won't throw but would replace the current shadow
> root, whereas there is no way to access the NAC from the content script.
> This can be verified from DevTools from the page.

Pretty sure it does not replace the current shadow root:

  https://searchfox.org/mozilla-central/rev/38bcf897f1fa19c1eba441a611cf309482e0d6e5/dom/base/Element.cpp#1222

But yeah, at least one of the two (either the page, or the pretty-printing) would be broken. Arguably it's a degenerate case but...
We drop prettyprinting if you reach into it and mutate the DOM.

We should likewise drop it if you try to attach shadowroots anywhere in that DOM.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #12)
> Pretty sure it does not replace the current shadow root:
> 
>  
> https://searchfox.org/mozilla-central/rev/
> 38bcf897f1fa19c1eba441a611cf309482e0d6e5/dom/base/Element.cpp#1222
> 
> But yeah, at least one of the two (either the page, or the pretty-printing)
> would be broken. Arguably it's a degenerate case but...

OK. I don't really know what I saw in comment 9 anymore; We actually don't pretty print at all if we met an xHTML element (including the root) in an XML document other than XBL or XSLT. I guess pretty print was designed for Gecko source code viewing :-P?

https://searchfox.org/mozilla-central/rev/38bcf897f1fa19c1eba441a611cf309482e0d6e5/dom/xml/nsXMLContentSink.cpp#187
https://searchfox.org/mozilla-central/rev/38bcf897f1fa19c1eba441a611cf309482e0d6e5/dom/xml/nsXMLContentSink.cpp#523
https://searchfox.org/mozilla-central/rev/38bcf897f1fa19c1eba441a611cf309482e0d6e5/dom/xml/nsXMLContentSink.cpp#956

With that out of the door, having to remove XMLMonoPrint.css is the only drawback I can think of now. I don't think bug 1410578 will be implemented in a way that <link> in shadow DOM can affect View -> Page Style selection of the whole document.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #13)
> We drop prettyprinting if you reach into it and mutate the DOM.
> 
> We should likewise drop it if you try to attach shadowroots anywhere in that
> DOM.

For XBL or XSLT pretty print, I can indeed attach a shadow DOM into an HTML node without dropping off from pretty print, LOL.

Just visit, for example,

https://hg.mozilla.org/mozilla-central/raw-file/9900cebb1f90/dom/xml/resources/XMLPrettyPrint.xml

and do

document.documentElement.firstElementChild.firstElementChild.firstElementChild.attachShadow({ mode: 'open'});

in the DevTools console.
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #15)

This happens on Nightly currently, to clarify.
My point is that right now attaching shadow roots does not drop prettyprinting.  It should.  Prettyprinting is designed for display of XML "data documents".  If the page is messing with the document, we need to stop prettyprinting it, because the prettyprinted version no longer reflects the document.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #17)
> My point is that right now attaching shadow roots does not drop
> prettyprinting.  It should.  Prettyprinting is designed for display of XML
> "data documents".  If the page is messing with the document, we need to stop
> prettyprinting it, because the prettyprinted version no longer reflects the
> document.

Right. Given that this happens on the current Nightly and on a small set of documents where we do pretty print HTML elements, I think it belongs to another bug.

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #14)
> With that out of the door, having to remove XMLMonoPrint.css is the only
> drawback I can think of now. I don't think bug 1410578 will be implemented
> in a way that <link> in shadow DOM can affect View -> Page Style selection
> of the whole document.

BTW, speaking of dropping out of pretty print, my patch currently doesn't do that and it should.
We should probably be using the UA shadow roots that are being implemented in bug 1431255.
Depends on: 1431255
Sorry, they are not related. The proposed UA Shadow Root in bug 1431255 would not have any special ability than usual Shadow Roots.
No longer depends on: 1431255
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #18)
> BTW, speaking of dropping out of pretty print, my patch currently doesn't do
> that and it should.

Given that there is no way to remove an already attached shadow root, the way to undo its effect would be to clean it up and put a <slot /> into it. It doesn't seem to work in an XML document though, so I've filed bug 1466756.
I would like to reiterate that XML prettyprinting needs to not be page-detectable if they use various DOM manipulation APIs.  That means we need the ability to remove it completely as needed.  In particular, at the point at which we are officially shipping Shadow DOM, we need to have the ability to replace the shadow root we attach with a page-provided one as needed.
According to what I've found in comment 14, we will never pretty print an XML document with an HTML root element. There won't be a pretty-print shadow DOM in the first place.
> we will never pretty print an XML document with an HTML root element.

That's correct.

And non-HTML elements don't allow shadow DOM to be added to them by pages?
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #25)
> I would like to reiterate that XML prettyprinting needs to not be
> page-detectable if they use various DOM manipulation APIs.  That means we
> need the ability to remove it completely as needed.  In particular, at the
> point at which we are officially shipping Shadow DOM, we need to have the
> ability to replace the shadow root we attach with a page-provided one as
> needed.

That’s why I suggested using the UA shadow roots that are being developed in bug 1431255 for this in comment #19.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #27)
> And non-HTML elements don't allow shadow DOM to be added to them by pages?

That's correct, content can only do |attachShadow()| on a handful of allowed HTML elements.

(In reply to ExE Boss from comment #28)
> That’s why I suggested using the UA shadow roots that are being developed in
> bug 1431255 for this in comment #19.

Let me explain comment 20 a bit more. The "UA shadow root" does not have any ability beyond skipping the requirement above (i.e. we can attach shadow root to whatever element we want), from C++. We are already using "UA shadow root" conceptually in the proposed WIP.

The other big unachieved part in bug 1431255 was about being able to run JS within the tree, which is not needed in this bug, because functional JS has been removed in bug 1432636 already.
Hi Bobby,

Could you take a look at this patch for me? Original I thought this is not related to UA Shadow Root (bug 1431255), but our discussion about putting the DOM nodes inside it into another reflector etc makes me feel that perhaps that's a usable defence-in-depth here too.

I guess the question to ask is that do we want that here also? I don't understand if we do any similar protection in our current implementation given that the document fragment was created by the XSLT and only stick into the XBL context later.
According to Harald its fine to remove mono print. Let me get comment 30 answered and the patch reviewed.
Assignee: nobody → timdream
Status: NEW → ASSIGNED
For what it's worth, mono print was added because it makes some things much more readable...
(In reply to Boris Zbarsky [:bz] (Vacation Jun 16-24) (no decent commit message means r-) from comment #33)
> For what it's worth, mono print was added because it makes some things much
> more readable...

We could (should?) monospace it by default. I don't think it's worth keeping both options though since it isn't possible to do with the Shadow DOM approach.
I think you will discover that people have extremely strong disagreements regarding whether monospaced or not is more readable...

We could have in-page UI to switch the sheet instead of using the UA sheet switcher, if we really wanted to.
We don't turn on Shadow DOM in the test profile, so Try breaks

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5dd7aaeeaadd731550178d42828642e3535c915

This can't land until we ship shadow DOM for sure, and we also want to make sure pref'ing off shadow DOM won't crash the browser.
(In reply to Boris Zbarsky [:bz] (Vacation Jun 16-24) (no decent commit message means r-) from comment #35)
> We could have in-page UI to switch the sheet instead of using the UA sheet
> switcher, if we really wanted to.

That would involve running JS within that Shadow DOM, which means we will need bug 1431255 here too.
Note to myself, TBD:

* Don't delete the binding for now and only switch to shadow root when it is enabled
* Replace the sad SetInnerHTML()
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #38)
> Note to myself, TBD:
> 
> * Don't delete the binding for now and only switch to shadow root when it is
> enabled

Didn't Emilio have an idea for how we could always use SD regardless of pref value and drop the binding?
(In reply to Brian Grinstead [:bgrins] from comment #39)
> Didn't Emilio have an idea for how we could always use SD regardless of pref
> value and drop the binding?

:smaug has different ideas. He said Shadow DOM is closed to shipping. The pref will be gone by then.
Comment on attachment 8982493 [details]
Bug 1437956 - Pretty print XML with Shadow DOM

https://reviewboard.mozilla.org/r/248466/#review257782

::: dom/xml/resources/jar.mn
(Diff revision 7)
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  toolkit.jar:
>      content/global/xml/XMLPrettyPrint.css                 (XMLPrettyPrint.css)
> -    content/global/xml/XMLMonoPrint.css                   (XMLMonoPrint.css)
> -    content/global/xml/XMLPrettyPrint.xml                 (XMLPrettyPrint.xml)

Probably didn't mean to remove this file, right? This will still be referenced if SD is disabled.
Attachment #8982493 - Flags: review?(bugs)
Please let me know if moving the DOM from XBL NAC to shadow root breaks any security protection, and whether or not this can land as-is.
Closed shadow DOM is of course not any kind of security protection. It is designed to be just a rather simple encapsulation from the light DOM.
Need to think this a but, but probably this approach is fine. Pretty printer will just use shadow DOM, and if some shadow DOM is accidentally leaked, that doesn't matter.

But, we don't actually seem to have NAC around here, right?
The original xml document just has binding on the root element, then all its content is hidden and transformed document shown as a child of anonymous id="top".

Modifications to the document are supposed to drop the styling.
Though, looks like bug 64945 didn't cover the case when text data is modified. I guess we didn't have notifications for text data changes 16 years ago.
Comment on attachment 8982493 [details]
Bug 1437956 - Pretty print XML with Shadow DOM

https://reviewboard.mozilla.org/r/248466/#review258174

::: dom/xml/nsXMLPrettyPrinter.cpp:144
(Diff revision 8)
> +        shadowRoot->SetIsComposedDocParticipant(true);
> +        rootElement->SetShadowRoot(shadowRoot);
> +
> +        // Attach the first and only child of the fragment to the root element.
> +        RefPtr<nsINode> node = resultFragment->GetFirstChild();
> +        resultFragment->RemoveChild(*node, err);

Why you need to remove the node explicitly?

::: dom/xml/nsXMLPrettyPrinter.cpp:252
(Diff revision 8)
> +    RefPtr<ShadowRoot> shadowRoot = mDocument->GetRootElement()->GetShadowRoot();
> +    if (shadowRoot) {
> +        shadowRoot->GetFirstChild()->Remove();
> +
> +        nsNodeInfoManager *nodeInfoManager = mDocument->NodeInfoManager();
> +        RefPtr<NodeInfo> nodeInfo = nodeInfoManager->GetNodeInfo(nsGkAtoms::slot,

So the issue with this approach is that display: contents is inherited to assigned nodes, if they have display: inherit.
I wonder what it would take to uninstall ShadowRoot altogether. Something to at least think about a bit.

::: dom/xml/resources/XMLPrettyPrint.xml:19
(Diff revision 8)
>  
>      <handlers>
>        <handler event="prettyprint-dom-created" allowuntrusted="false">
>          <![CDATA[
> -          document.getAnonymousNodes(this).item(0).appendChild(event.detail);
> +          let container = document.getAnonymousNodes(this).item(0);
> +          for (let el of event.detail.childNodes) {

Why this change? event.detail should be documentFragment, so appending it to another element just moves its child nodes.
Attachment #8982493 - Flags: review?(bugs) → review-
Comment on attachment 8982493 [details]
Bug 1437956 - Pretty print XML with Shadow DOM

https://reviewboard.mozilla.org/r/248466/#review258346

::: dom/xml/nsXMLPrettyPrinter.cpp:144
(Diff revision 8)
> +        shadowRoot->SetIsComposedDocParticipant(true);
> +        rootElement->SetShadowRoot(shadowRoot);
> +
> +        // Attach the first and only child of the fragment to the root element.
> +        RefPtr<nsINode> node = resultFragment->GetFirstChild();
> +        resultFragment->RemoveChild(*node, err);

Please look at the XSLT change; XSTL now generates the `<div id="top">` node. The code here remove it from the document fragment and append it to the Shadow DOM.

I cannot attach document fragment to the Shadow DOM directly.

::: dom/xml/nsXMLPrettyPrinter.cpp:252
(Diff revision 8)
> +    RefPtr<ShadowRoot> shadowRoot = mDocument->GetRootElement()->GetShadowRoot();
> +    if (shadowRoot) {
> +        shadowRoot->GetFirstChild()->Remove();
> +
> +        nsNodeInfoManager *nodeInfoManager = mDocument->NodeInfoManager();
> +        RefPtr<NodeInfo> nodeInfo = nodeInfoManager->GetNodeInfo(nsGkAtoms::slot,

Could you elaborate? I don't understand how the behavior of `display` value will change here. I also can seem to be able to set inline styles on these XML nodes.

I've tried to unset the shadow root with `SetShadowRoot(nullptr)` but I will hit the assertion at https://searchfox.org/mozilla-central/source/dom/base/Element.cpp#4371 . I don't know if allowing shadow root to be removed is something we want to support.

::: dom/xml/resources/XMLPrettyPrint.xml:19
(Diff revision 8)
>  
>      <handlers>
>        <handler event="prettyprint-dom-created" allowuntrusted="false">
>          <![CDATA[
> -          document.getAnonymousNodes(this).item(0).appendChild(event.detail);
> +          let container = document.getAnonymousNodes(this).item(0);
> +          for (let el of event.detail.childNodes) {

Again, plainly attach the document fragement will result two `<div id="top">`s in the tree.
Flags: needinfo?(bugs)
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #47)

> 
> I cannot attach document fragment to the Shadow DOM directly.
hmm, why not


> 
> Could you elaborate? I don't understand how the behavior of `display` value
> will change here. I also can seem to be able to set inline styles on these
> XML nodes.
slot element has display: contents. If assigned node has display: inherit, it will inherit display: contents, and not display value of the root element, as it should
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #48)
> (In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment
> #47)
> > I cannot attach document fragment to the Shadow DOM directly.
> hmm, why not

You are right, I should use nsINode::AppendChild() here.

> > Could you elaborate? I don't understand how the behavior of `display` value
> > will change here. I also can seem to be able to set inline styles on these
> > XML nodes.
> slot element has display: contents. If assigned node has display: inherit,
> it will inherit display: contents, and not display value of the root
> element, as it should

How could the assigned node has |display: inherit|? I assume we are operating on unstyled XML document and there shouldn't be any other web content styles here.
(In reply to Tim Guan-tin Chien [:timdream] from comment #49)
> (In reply to Olli Pettay [:smaug] from comment #48)
> > (In reply to Tim Guan-tin Chien [:timdream] from comment #47)
> > > Could you elaborate? I don't understand how the behavior of `display` value
> > > will change here. I also can seem to be able to set inline styles on these
> > > XML nodes.
> > slot element has display: contents. If assigned node has display: inherit,
> > it will inherit display: contents, and not display value of the root
> > element, as it should
> 
> How could the assigned node has |display: inherit|? I assume we are
> operating on unstyled XML document and there shouldn't be any other web
> content styles here.

That sort of a situation can occur when the XML document gets modified by a script, which has to turn off the XML pretty print.

However, there’s currently no way to actually detach a Shadow Root, and so the Shadow DOM has to be replaced with a `<slot/>` element.

In the case when an element which is a direct child of the root element has `display: inherit;`, it will inherit the `display: contents;` of the `<slot/>` element, rather than whatever display value the root element has, at least until https://github.com/whatwg/html/issues/3748 gets resolved in a way which would require CSS inheritance to follow the light tree, rather than the flattened tree.
Comment on attachment 8982493 [details]
Bug 1437956 - Pretty print XML with Shadow DOM

https://reviewboard.mozilla.org/r/248466/#review258612

::: dom/base/Element.cpp:1290
(Diff revision 9)
> +  }
> +  MOZ_ASSERT(!GetPrimaryFrame());
> +
> +  // Simply unhook the shadow root from the element.
> +  // XXX: This assumes there is no <slots> within it
> +  SetShadowRoot(nullptr);

Could you please assert that ShadowRoot's mSlotMap is empty.

::: dom/xml/nsXMLPrettyPrinter.h:47
(Diff revision 9)
>  private:
>      virtual ~nsXMLPrettyPrinter();
>  
>      /**
>       * Signals for unhooking by setting mUnhookPending if the node changed is
> -     * non-anonymous content.
> +     * not in the shadow root tree.

nor in anonymous content.

::: dom/xml/nsXMLPrettyPrinter.cpp:220
(Diff revision 9)
>  {
> +    // We had to ignore DOM changes in the shadow tree and the <scrollbar>s,
> +    // or the XBL binding.
> +    bool isGeneratedContent = !aContent ?
> +        false :
> +        aContent->GetBindingParent() || aContent->IsInShadowTree();

nodes in shadow tree have binding parent, but I guess this is actually easier to understand anyhow.
But you could drop !aContent check in 'if' and have just
!isGeneratedContent && !mUnhookPending

::: dom/xml/nsXMLPrettyPrinter.cpp:247
(Diff revision 9)
> +
> +        // Remove the bound XBL binding
>          mDocument->BindingManager()->ClearBinding(element);
>      }
>  
> +    mDocument->RemoveObserver(this);

I don't understand this code move.

::: dom/xml/resources/XMLPrettyPrint.xml:19
(Diff revision 9)
>  
>      <handlers>
>        <handler event="prettyprint-dom-created" allowuntrusted="false">
>          <![CDATA[
> -          document.getAnonymousNodes(this).item(0).appendChild(event.detail);
> +          let container = document.getAnonymousNodes(this).item(0);
> +          for (let el of event.detail.childNodes) {

Please add a comment why you need to explicitly iterate the child nodes.
Attachment #8982493 - Flags: review?(bugs) → review+
Comment on attachment 8982493 [details]
Bug 1437956 - Pretty print XML with Shadow DOM

https://reviewboard.mozilla.org/r/248466/#review258630

::: dom/base/ShadowRoot.h:142
(Diff revision 12)
>    void InvalidateStyleAndLayoutOnSubtree(Element*);
>  
>  public:
>    void AddSlot(HTMLSlotElement* aSlot);
>    void RemoveSlot(HTMLSlotElement* aSlot);
> +  bool HasSlots() { return !mSlotMap.IsEmpty(); };

Drive-by nit: `bool HasSlots() const` :)
Pushed by timdream@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5fc9867c3330
Pretty print XML with Shadow DOM r=smaug
https://hg.mozilla.org/mozilla-central/rev/5fc9867c3330
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
For what it's worth, this isn't the type of patch I would've been comfortable with landing during soft code freeze, especially because this is a less used feature so it may get less test coverage than usual. However, I don't think it needs to be backed out either now that it landed. It's safe to revert on Beta if any issue arises.
(In reply to :Paolo Amadini from comment #60)
> For what it's worth, this isn't the type of patch I would've been
> comfortable with landing during soft code freeze, especially because this is
> a less used feature so it may get less test coverage than usual. However, I
> don't think it needs to be backed out either now that it landed. It's safe
> to revert on Beta if any issue arises.

The old pretty-printer is still in use when Shadow DOM is disabled, so this code shouldn't run in beta at all, right?
I see some code that is conditional on nsContentUtils::IsShadowDOMEnabled(), but the rest of the changes, like the way the styles are loaded, seem to still apply. It's true that most of the logic will probably stay the same, however.
Also, bug 1410578 got fixed literally seconds after this, so the `<style>@import "…";</style>` can now be changed back to `<link rel="stylesheet" src="…"/>` and XMLMonoPrint.css can probably be restored.
(In reply to ExE Boss from comment #63)
> Also, bug 1410578 got fixed literally seconds after this, so the
> `<style>@import "…";</style>` can now be changed back to `<link
> rel="stylesheet" src="…"/>` and XMLMonoPrint.css can probably be restored.

Yeah, I just tried it, and I was right at comment 21: <link> in Shadow DOM will not be able to affect the page style selection.

(In reply to :Paolo Amadini from comment #60)
> For what it's worth, this isn't the type of patch I would've been
> comfortable with landing during soft code freeze, especially because this is
> a less used feature so it may get less test coverage than usual. However, I
> don't think it needs to be backed out either now that it landed. It's safe
> to revert on Beta if any issue arises.

Sorry I overlooked the merge schedule. We can aggressively back this out if problems arise.
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #64)
> (In reply to :Paolo Amadini from comment #60)
> > For what it's worth, this isn't the type of patch I would've been
> > comfortable with landing during soft code freeze, especially because this is
> > a less used feature so it may get less test coverage than usual. However, I
> > don't think it needs to be backed out either now that it landed. It's safe
> > to revert on Beta if any issue arises.
> 
> Sorry I overlooked the merge schedule. We can aggressively back this out if
> problems arise.

It seems unlikely there will be an issue on Beta 62, since Shadow DOM will be disabled and the current XBL binding will continue to be used.
This has a nice positive effect on the DevTools Inspector, in that it now shows the original markup inside the root element, whereas before it presented it as an empty element (unless you turned on devtools.inspector.showAllAnonymousContent, at which point you got the anon content with the original markup inside, so still not great).
Type: enhancement → task
Summary: Find a way to put the XML Pretty Print UI into the page without using XBL → Recreate XML Pretty Print UI using shadow DOM instead of XBL
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: