890193 - XML prettyprinter no longer drops the tree styling when the DOM is modified

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Stephan Hradek]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130618035212

Steps to reproduce:

For a long time I used a greasemonkey script to add some CSS to an XML file because it was a bit tedious to read the pure XML.

The script goes like this (stripped down a bit):

// ==UserScript==
// @name           checkloaded with style
// @namespace      checkloaded
// @include        */...my url here
// @grant          GM_setValue
// @grant          GM_getValue
// @grant          GM_deleteValue
// ==/UserScript==

if (GM_getValue(document.location.href, 0) == 1) {
     GM_deleteValue (document.location.href);
}
else {
    pi=document.createProcessingInstruction('xml-stylesheet', 'href="#" type="text/css"')
   
    document.documentElement.parentNode.insertBefore( pi, document.documentElement );
   
    var resultcode= document.getElementsByTagName('result-code')[0];
    resultcode.addEventListener("click", function() {
            GM_setValue(document.location.href, 1);
            history.go(0);
    }, true);

    resultcode.setAttribute("title", "Click to see raw XML");
    window.setTimeout(
        function() {
            var i=0;
            var css=document.styleSheets[0];
            css.rel='stylesheet';
            css.insertRule("response { \
                display: block;        \
                font-family: Arial;    \
                padding:2px;           \
                font-size: 8pt;        \
                color: white;             \
                background-color:#e60000; \
            }", i++);
            css.insertRule("response:after { \
                content: \"© by me \"; \
            }", i++);
            css.insertRule("result-code { \
                display: inline;           \
                padding:2px 2px 2px 20px; \
                color: white;             \
                font-size: 3em;           \
                cursor: pointer;          \
            }", i++);
            css.insertRule("loaded-list {            \
                display: table;                      \
                margin-left:auto; margin-right:auto; \
                border-collapse: collapse;           \
                color: black;             \
            }", i++);
            css.insertRule("property:nth-child(even) { \
                display: table-row;                   \
                background-color: #d0d0d0;            \
            }", i++);
            css.insertRule("property:nth-child(odd) { \
                display: table-row;                    \
                background-color: white;               \
            }", i++);
            css.insertRule("name {            \
                display: table-cell;          \
                font-weight: bold;            \
                padding: 3px;                 \
                min-width:20px;               \
                border-right: grey 1px solid; \
            }", i++);
            css.insertRule("mem-value { \
                display: table-cell;       \
                padding: 3px;              \
                min-width: 40px;           \
            }", i++);
    }, 1000);
}

And the XML looks like this:

<?xml version="1.0" encoding="UTF-8"?>
<response>
    <result-code>OK</result-code>
    <loaded-list>
        <property>
            <name>sp.invalid.request.message</name>
            <mem-value>The request is invalid.  Please contact the Service Portal administrator.</mem-value>
        </property>
        <property>
            <name>maintenance.thread.sleep.seconds</name>
            <mem-value>30</mem-value>
        </property>
        <property>
            <name>log_level</name>
            <mem-value>ERROR</mem-value>
        </property>
        <property>
            <name>cookie.secure</name>
            <mem-value>false</mem-value>
        </property>
        <property>
            <name>metrics.enable</name>
            <mem-value>true</mem-value>
        </property>
    </loaded-list>
</response>

The strange thing is: Partly it works as I can see the font change and shrink one second after loading the page.

The script is confirmed to still work with FF18: https://groups.google.com/forum/#!topic/greasemonkey-users/GmltyTCvdDU


Actual results:

I see the XML tree with just a changed font.


Expected results:

I should see a table containing all my XML entries

Comment 1

[Boris Zbarsky [:bzbarsky]]

Does it work if you do href="data:text/css," instead of href="#"?  In my local test in an XML file (not using GreaseMonkey) that seems to work just fine.

Can you attach the actual stripped-down GM script you're using using https://bugzilla.mozilla.org/attachment.cgi?bugid=890193&action=enter just so we don't run into weird whitespace issues cutting and pasting and I can make sure I'm testing what you're testing?

Comment 2

[Stephan Hradek]

Attachment: Greasemonkey script to apply CSS to some XML

As requested in https://bugzilla.mozilla.org/show_bug.cgi?id=890193 by Boris Zbarsky (:bz)

Comment 3

[Stephan Hradek]

Hi Boris!
I'm not doing any "href" at all.

I receive the XML from a servlet and only apply the CSS by adding some elements to the XML DOM tree. I don't change the XML in any other way and I also do not have the possibility to change t.

I uploaded the stripped down GreaseMonkey code. Stripped down in this case means: Removed the (C) remark added in the final output and removed an image which also is usually added. So just 2 CSS rules are removed.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> I'm not doing any "href" at all.

Sure you are.  It's right here:

    pi=document.createProcessingInstruction('xml-stylesheet', 'href="#" type="text/css"')

Thank you for the script; I'll take a look.

Comment 5

[Boris Zbarsky [:bzbarsky]]

OK, I can reproduce the behavior in Firefox 22: we keep showing the XML tree viewer instead of dropping the tree binding.  I had missed that part of comment 0; my test in comment 1 involved an XML file which did not get the tree viewer at all.

Looks like the behavior first changed in Firefox 21, in particular changed in this checkin range back in January 2013: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=98ea4d294369&tochange=a207f33adc1a

This looks like a likely regression from bug 833412.  Apparently we don't have sane tests for the XML prettyprinter dropping the binding?  :(

Comment 6

[Boris Zbarsky [:bzbarsky]]

So we land in nsBindingManager::ClearBinding, but binding->IsStyleBinding() is true so we return without doing anything.

In fact, it looks like there are no more callers of SetIsStyleBinding.  The only caller used to be in LoadBindings when aAugmentFlag was true, but that got nuked in bug 833412.

Comment 7

[Boris Zbarsky [:bzbarsky]]

And I can't find a way to write an automated test for this, because the prettyprinter only triggers on toplevel windows....

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Make the XML prettyprinter actually drop its binding when it unhooks.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Stephan, thank you for reporting this!

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 772702 [details] [diff] [review]
Make the XML prettyprinter actually drop its binding when it unhooks.

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

r=bholley

Comment 11

[Bobby Holley (:bholley)]

And can you please add some comments explaining what IsStyleBinding means?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Alternate approach: just treat all bindings as style bindings

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Alternate approach diff -w

Comment 14

[Stephan Hradek]

(In reply to Boris Zbarsky (:bz) from comment #4)
> > I'm not doing any "href" at all.
> 
> Sure you are.  It's right here:
> 
>     pi=document.createProcessingInstruction('xml-stylesheet', 'href="#"
> type="text/css"')

Oops :( I wrote this long and partly borrowed it. I should have looked more carefully.

Comment 15

[Boris Zbarsky [:bzbarsky]]

For what it's worth, you may still want to change the script: it's loading the entire XML document a second time just to throw it away....  href="data:text/css," should work way better.

Comment 16

[Stephan Hradek]

(In reply to Boris Zbarsky (:bz) from comment #15)
> For what it's worth, you may still want to change the script: it's loading
> the entire XML document a second time just to throw it away.... 
> href="data:text/css," should work way better.

I will try that. At the moment I'm doing XSLT transformations to ge the desired result.

But I don't understand your comment "it's loading the entire XML document a second time just to throw it away". Do you mean because of the "history.go(0);"?

This is just upon a click of the user in case he wants to see the XML tree. In that case the css is not applied.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> I will try that. 

It won't help this bug's issue, just the general performance of the script.

> Do you mean because of the "history.go(0);"?

No, because <?xml-stylesheet type="text/css" href="#"?> will load the entire XML document again from the network (or cache, depending), try to parse it as CSS, discover it's not CSS, and leave you with an empty stylesheet...

Comment 18

[Stephan Hradek]

(In reply to Boris Zbarsky (:bz) from comment #17)
> > I will try that. 
> 
> It won't help this bug's issue, just the general performance of the script.
Tried it and it's really great. If the bug were not there It would be fantastic.

Thanks for telling me!

Comment 19

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/86702cdd814c

Comment 20

[Boris Zbarsky [:bzbarsky]]

And https://hg.mozilla.org/integration/mozilla-inbound/rev/e46098561cb1 because this unhooking business asserts when it happens.  See bug 892638.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/86702cdd814c
https://hg.mozilla.org/mozilla-central/rev/e46098561cb1

Comment 22

[Carsten Book [:Tomcat]]

(In reply to [e-mail] from comment #4 in Bug 892638)
> This appears to have had the same cause as bug 943804, so I backed out the
> assertion annotations:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/eef2361b1f9b
> 
> The test case no longer asserts either.

also hg commit comment Backout of changeset e46098561cb1 because bug 943804 fixed bug 892638 CLOSED TREE

Comment 23

[Carsten Book [:Tomcat]]

and so also backed out from m-c in https://hg.mozilla.org/mozilla-central/rev/eef2361b1f9b

Comment 24

[Boris Zbarsky [:bzbarsky]]

_One_ of the changesets was backed out.  The one that doesn't actually fix this bug.  So it's still fixed just fine.