Closed Bug 1432636 Opened 7 years ago Closed 6 years ago

Implement XML pretty printing without requiring <handler> JS in XBL

Categories

(Core :: XML, task, P2)

Product:
Core
Component:
XML
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: bgrins, Assigned: Kwan)

References

Details

(Whiteboard: [xbl-in-content])

Attachments

(5 files, 2 obsolete files)

prettyprint.html
932 bytes, text/html
Details
Make XMLPrettyPrint use html:details instead of click handler (based on bgrins' prototype)
8.18 KB, patch
bgrins
: feedback+
ntim
: feedback+
Details | Diff | Splinter Review
Test XML document
5.93 KB, text/xml
Details
expander spacing comparison
20.55 KB, image/png
Details
Bug 1432636 - Switch XMLPrettyPrint to use HTML <details> instead of click handlers for expandables.
59 bytes, text/x-review-board-request
ntim
: review+
peterv
: review+
Details 
What currently happens when you pretty print some XML (`data:text/xml,<foo><bar><baz>wat</baz>asdf</bar></foo>`) is we apply xslt to the source xml and then we put it in the DOM via XBL:

- https://searchfox.org/mozilla-central/rev/e3cba77cee3ff1be38313abe9c804d13c51bd95b/dom/xml/nsXMLPrettyPrinter.cpp#99-186
- https://searchfox.org/mozilla-central/source/dom/xml/resources/XMLPrettyPrint.xsl
- https://searchfox.org/mozilla-central/source/dom/xml/resources/XMLPrettyPrint.xml#36

What we could do instead:

1) I think we can turn the XML pretty printing into HTML with no JS by modifying the existing XSLT to render out appropriate <details> / <summary> tags.
2) Then we need a way to place to put that DOM - I was thinking this could be in a UA shadow root (see Bug 1431255), although I believe we also discussed somehow using the existing native anon content / CanvasFrame similar to how the devtools highlighters work.

It may be easiest to start by doing just (1) and continuing to use XBL to mount the content (but without any <handlers> since the HTML tags handle expand/collapse).
Attached file prettyprint.html — 
Here's a simple HTML page outlining the sort of markup that may work for (1)
We could also look at what the FeedWriter does - it used to use XBL to render the content on top of RSS feeds but that was removed in Bug 1432851. I'm not sure exactly all the plumbing required for this and if it would be appropriate here - it appears to be using an AboutRedirector to load subscribe.xhtml in content.
Also note that we have a better "jsonview" for rendering JSON (`data:application/json,{"test":[0,1,2]}`).

https://dxr.mozilla.org/mozilla-central/source/devtools/client/jsonview

If we think that XML pretty printing is a useful feature for developers, eventually we might want to re-implement it with the better user experience that the "jsonview" provides.
The jsonview approach is not spec-compliant, fwiw, and could cause web compat issues too.  Just so people are aware before going with it...
Can you elaborate on the difference between the two approaches that make one of them not spec-compliant? Is this related to how we choose to use the devtools view rather than downloading the file, or something else?
Flags: needinfo?(bzbarsky)
The relevant entry point is https://html.spec.whatwg.org/#process-a-navigate-response

For the JSON case, it says "Follow the steps given in the plain text file section, and then, once they have completed, return."  That links to https://html.spec.whatwg.org/#read-text which specifies what the DOM of the resulting page is allowed to look like.  The DOM we produce for an application/json response looks nothing like that.  That said, there is weasel-wording in the spec about how an "explicitly supported JSON type" as defined in https://html.spec.whatwg.org/#explicitly-supported-json-type can be handled in other ways.  Maybe.

Similar for XML; unless it's an https://html.spec.whatwg.org/#explicitly-supported-xml-type you have to create a DOM per the rules of https://html.spec.whatwg.org/#read-xml

Point being, the current XML viewer preserves the actual DOM the XML parses into (and uses anonymous content for all its fancy viewer machinery).  The JSON viewer just munges the DOM from the thing the spec defines to something totally different.
Flags: needinfo?(bzbarsky)
If we could generate a preview that doesn't rely on JS at all (like using <details> and <summary> HTML tags) then maybe we could attach that as native anonymous content to preserve the actual DOM without requiring some kind of UA shadow DOM support.  I think that could be done by modifying the existing XSL at https://searchfox.org/mozilla-central/source/dom/xml/resources/XMLPrettyPrint.xsl to generate the new HTML and then create the NAC instead of attaching the binding at https://searchfox.org/mozilla-central/source/dom/xml/nsXMLPrettyPrinter.cpp#127-186.
Whiteboard: [xbl-in-content]
(In reply to Brian Grinstead [:bgrins] from comment #7)
> If we could generate a preview that doesn't rely on JS at all (like using
> <details> and <summary> HTML tags)

So basing off your prototype from comment 1 this is very doable.  <details> and <summary> are pretty neat.
Unfortunately the NAC creation is beyond me, but hopefully this patch will save someone some work (Hmm, I suppose it could even land now).

A few adjustments needed:
- swapped to the actual minus symbol
- list-style-position needs to be outside, otherwise lining up expandable and non-expandable elements is not-fun.
- The <details> and partner <span> need to be wrapped in a containing element else two consecutive collapsed elements end up on the same line due to the display: inline-block, though there are already containing divs in the existing XSLT, so that's not much of an adjustment, just a difference to the prototype.

Other notes:
- Slight behaviour change in that clicking the opening tag now toggles expanding, instead of just the +/−.  However it's still possible to select text in said tag without triggering toggling, so that doesn't seem too bad.
- Monospace style seems to have had an error for a while (probably since bug 379683) where collapsed & non-collapsible top-level elements aren't monospaced. This seem most simply fixable by wrapping all the transformed elements in one element (I went for <main>).  The other possibility would be something like #top > :not(#header), but grouping the output seemed neater.
- Expandable XML PIs currently have a space between them and their control that nothing else does, so I removed that since I see no reason for it.
- The original bug actually wanted to use twisties instead of +/−: bug 64945 comment 210
- Do we actually want the current behaviour of top-level non-expandables not lining up with expandables?  It seems kind of ugly.  Actually I think that might be another bug 379683 regression.

As an aside ::-moz-list-bullet not being selectable in the inspector (/not accessible by DeepTreeWalker?) is slightly inconvenient.
(In reply to Ian Moody [:Kwan] (UTC+0) from comment #8)
> - Expandable XML PIs currently have a space between them and their control
> that nothing else does, so I removed that since I see no reason for it.

That might have been another bug 379683 regression.

> - Do we actually want the current behaviour of top-level non-expandables not
> lining up with expandables?  It seems kind of ugly.  Actually I think that
> might be another bug 379683 regression.

On the basis that I'm pretty sure that that's true I've made them line up.  Simplifies the styling too (modulo fussing over margin vs. padding).  I'm not sure if there's still any point in giving expandable tags their own class now that it isn't used for styling anymore.

As another bonus, <details> gets us keyboard accessibility of the expander controls for free.

Brian, do you think it's worth checking this in now (assuming it gets r+ of course) and leave-open until the NAC gen. is done? (or splitting this off into a separate bug, whichever)
Attachment #8949554 - Attachment is obsolete: true
Flags: needinfo?(bgrinstead)
Attached file Test XML document (obsolete) — 
This is a stripped down version of the file I've been using to test this, since I needed something with multiple PIs and comments, with and without expanders, including consecutive expandables.
(hopefully giving it a text/xml Content Type, like loading it from file://, stops the feed view triggering)
Attached file Test XML document — 
Bah, okay that doesn't work.  But testing via actually hosting it, breaking the namespace does seem to.
Attachment #8949712 - Attachment is obsolete: true
Comment on attachment 8949709 [details] [diff] [review]
Make XMLPrettyPrint use html:details instead of click handler (based on bgrins' prototype)

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

Thanks for taking this on! And sure I'd be happy to split this up into multiple bugs - removing the click handler from XBL is a good start and it sounds like there are some UI improvements here (although I haven't tested locally or reviewed the xslt changes).

Maybe :bz has an idea on how to put resultFragment into NAC instead of attaching it via XBL here: https://searchfox.org/mozilla-central/source/dom/xml/nsXMLPrettyPrinter.cpp#127-186.

::: dom/xml/resources/XMLMonoPrint.css
@@ -2,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  @import url("chrome://global/content/xml/XMLPrettyPrint.css");

Can we merge together XMLMonoPrint.css and XMLPrettyPrint.css into a single file? This feature seems to the only place either is used. And we currently double-import XMLPrettyPrint - once here and once as a <link> in the xsl
Attachment #8949709 - Flags: feedback+
ni? for Comment 13
Flags: needinfo?(bgrinstead) → needinfo?(bzbarsky)
(In reply to Brian Grinstead [:bgrins] from comment #13)
> ::: dom/xml/resources/XMLMonoPrint.css
> @@ -2,5 @@
> >  /* This Source Code Form is subject to the terms of the Mozilla Public
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> >  @import url("chrome://global/content/xml/XMLPrettyPrint.css");
> 
> Can we merge together XMLMonoPrint.css and XMLPrettyPrint.css into a single
> file? This feature seems to the only place either is used. And we currently
> double-import XMLPrettyPrint - once here and once as a <link> in the xsl
I don't think we can merge because then there'd be no way to switch to Monospace, we'd have to create a new UI, ie. a checkbox in the header that adds a class or something.
Let me check about removing the import though.  It might be needed if it's unapplied when switching to the alternate, but vaguely remembering how alternate styles are used/work on my own site, I think that's only the case for named default styles, I think unnamed defaults stay, so it should be okay.

Yeah seems to work fine without it since XMLPrettyPrint stays applied (and at the moment, both XMLPrettyPrint and viewsource get double applied when switching to Monospace, as can be observed in the DevTools style editor).
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.
Flags: needinfo?(bzbarsky)
Comment on attachment 8949709 [details] [diff] [review]
Make XMLPrettyPrint use html:details instead of click handler (based on bgrins' prototype)

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

Tim, would you be able to review the changes here?
Attachment #8949709 - Flags: review?(ntim.bugs)
Blocks: 1437956
Comment on attachment 8949709 [details] [diff] [review]
Make XMLPrettyPrint use html:details instead of click handler (based on bgrins' prototype)

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

Works nicely and fixes some bugs.

I have a few comments:
- I noticed there is no longer spacing between the +/- and the opening tag
- The markup is not super explicit now, it also makes the CSS a bit mysterious. Can you add some class names ? For example, "details" could have the class "expander", "summary" could have the class "expander-opening-tag" and the "span" could have "expanding-closing-tag" (feel free to find better names). It would make the CSS/markup easier to read IMO.

This should probably get reviewed by a dom peer as well.
Attachment #8949709 - Flags: review?(ntim.bugs) → feedback+
(In reply to Tim Nguyen :ntim from comment #18)
> Comment on attachment 8949709 [details] [diff] [review]
> Make XMLPrettyPrint use html:details instead of click handler (based on
> bgrins' prototype)
> 
> Review of attachment 8949709 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Works nicely and fixes some bugs.
> 
> I have a few comments:
> - I noticed there is no longer spacing between the +/- and the opening tag

Hmm I don't really see any difference on mine, but I'm guessing it's a font issue.
There's no existing explicit spacing, but the current expander is given a 1em width box, and then the text is centered, so maybe your minus isn't as wide as the one on my system, and has a bigger gap between it and the edge of the box (mine also appears to be slightly to the right of center at default font-size).  Measuring the distance on mine I only find a 1px smaller gap (3px vs 2px with my patch).  Maybe you just have keener eyes :) (Although the difference is more pronounced in Monospace style, definitely too close there).
I don't think it'll be perfectly re-creatable without switching from list-style to a ::before for the +/−, so that it can be sized like the old expander, but that seems of dubious worth for the amount of code, and to get platform/font-dependant behaviour. If it's definite spacing between the symbol and tag you're after I think it'd make more sense to do it explicitly with a px or two of padding (or margin) on the summary marker/bullet.

(The difference in vertical space of the comments is because of moving the comment class from component elements to the containing div, which means the div ends up with a smaller line-height than the original due to comments specifically getting font-family: monospace; This didn't seem worth preserving when the only apparent reason for .comment not being on the div was so that its font effects didn't have to be overridden for the expander, but this patch makes the expander a pseudo element and so it needs overridding anyway)

> - The markup is not super explicit now, it also makes the CSS a bit
> mysterious. Can you add some class names ? For example, "details" could have
> the class "expander", "summary" could have the class "expander-opening-tag"
> and the "span" could have "expanding-closing-tag" (feel free to find better
> names). It would make the CSS/markup easier to read IMO.

Ah, this is a good idea, although I don't think expander works for the details, it seems like that'd be more suited for the summary, since that's the actual controlling element, and expander used to be just for the +/−.
(Ideally the closing tag wouldn't need to be outside the details, then wouldn't even need the enclosing div)
After fiddling for a while I've gone with

div.expandable
- details.expandable-body
- - summary.expandable-opening
- - div.expandable-children
- span.expandable-closing

I'm not 100% happy with -body but can't think of something better for everything-but-closing.
I also gave main the id tree, since the header says it's showing the document tree.


As Brian has split the rest of the work out into a new bug I'll upload the next version to mozreview, to save the sheriff effort on checkin.
Assignee: nobody → moz-ian
Status: NEW → ASSIGNED
Priority: -- → P2
Comment on attachment 8950913 [details]
Bug 1432636 - Switch XMLPrettyPrint to use HTML <details> instead of click handlers for expandables.

https://reviewboard.mozilla.org/r/220166/#review226072

Looks good to me, but please do get a review from a DOM peer.
Attachment #8950913 - Flags: review?(ntim.bugs) → review+
Comment on attachment 8950913 [details]
Bug 1432636 - Switch XMLPrettyPrint to use HTML <details> instead of click handlers for expandables.

https://reviewboard.mozilla.org/r/220166/#review226076

::: dom/xml/resources/XMLPrettyPrint.css:41
(Diff revision 2)
> +  font-family: initial;
> +  font-size: initial;
> +  font-style: initial;

Just `font: initial;` should work
Comment on attachment 8950913 [details]
Bug 1432636 - Switch XMLPrettyPrint to use HTML <details> instead of click handlers for expandables.

https://reviewboard.mozilla.org/r/220166/#review226084
Comment on attachment 8950913 [details]
Bug 1432636 - Switch XMLPrettyPrint to use HTML <details> instead of click handlers for expandables.

https://reviewboard.mozilla.org/r/220166/#review226072

Thanks!  Will do.
Comment on attachment 8950913 [details]
Bug 1432636 - Switch XMLPrettyPrint to use HTML <details> instead of click handlers for expandables.

https://reviewboard.mozilla.org/r/220166/#review226076

> Just `font: initial;` should work

It does. I tried it earlier but decided to stick with being conservative in only setting properties I know needed doing, unless review asked me to change it :)
Attachment #8950913 - Flags: review?(peterv)
Comment on attachment 8950913 [details]
Bug 1432636 - Switch XMLPrettyPrint to use HTML <details> instead of click handlers for expandables.

https://reviewboard.mozilla.org/r/220166/#review227862

Nice!

::: dom/xml/resources/XMLPrettyPrint.xsl:57
(Diff revision 3)
>        <xsl:text>&gt;</xsl:text>
>      </div>
>    </xsl:template>
>  
>    <xsl:template match="*[* or processing-instruction() or comment() or string-length(.) &gt; 50]">
> -    <div class="expander-open">
> +    <div class="expandable">

expandable class seems unused, so maybe remove it?
Attachment #8950913 - Flags: review?(peterv) → review+
Hi Ian, is this ready to land, besides the nit in comment #28?
Flags: needinfo?(moz-ian)
Sorry for the lag, this is good to land now.

Thanks for the reviews Peter and Tim!
Flags: needinfo?(moz-ian)
Keywords: checkin-needed
Comment on attachment 8950913 [details]
Bug 1432636 - Switch XMLPrettyPrint to use HTML <details> instead of click handlers for expandables.

https://reviewboard.mozilla.org/r/220166/#review232620

::: dom/xml/resources/XMLPrettyPrint.css
(Diff revision 4)
>  
>  .comment {
>    font-family: monospace;
>    white-space: pre;
>  }
> -

I’m pretty sure that the last empty line (`\n`) should not be removed, since no other file does this.
(In reply to ExE Boss from comment #32)
> ::: dom/xml/resources/XMLPrettyPrint.css
> (Diff revision 4)
> >  
> >  .comment {
> >    font-family: monospace;
> >    white-space: pre;
> >  }
> > -
> 
> I’m pretty sure that the last empty line (`\n`) should not be removed, since
> no other file does this.

This just removes 1 of the 2 trailing lines at the end of the file, which is change I'm personally fine with.
Usually it shows "\No newline at the end of this file" when there's no trailing line left.
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/48c4d99016ec
Switch XMLPrettyPrint to use HTML <details> instead of click handlers for expandables. r=ntim,peterv
Keywords: checkin-needed
Looks like file_bug590812.xhtml needs to be regenerated with your new markup.
Bah, sorry about that, not sure why I was convinced I'd already checked tests for this.
Updated the ref file.
Flags: needinfo?(moz-ian)
Keywords: checkin-needed
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/863272a3efcc
Switch XMLPrettyPrint to use HTML <details> instead of click handlers for expandables. r=ntim,peterv
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/863272a3efcc
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: