Bug 56263 – XML Viewer: extensions can't render original XML

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Pavel Feldman]

Now that there is a native support for XML rendering, existing Chrome extensions no longer work. There needs to be a clear way to opt-out.

[Adam Barth]

This should probably be a Chromium bug.

[Pavel Feldman]

(In reply to comment #1)
> This should probably be a Chromium bug.

There is nothing Chromium-specific in this bug and the fix will need to land into WebKit. Safari has extensions and XML view plugins too. We somewhat entered the 'browser' space when introduced this viewer upstream and need a way to opt-out or go back.

There also should be a way for the user to see original content (i.e. "See original" button in the XML Viewer UI). I am sure we will get one filed against Chromium as well and we'll wire it to this one.

[Vsevolod Vlasov]

Created attachment 85682 [details]
Patch

Since current xml viewer implementation applies XSL transformation internally, custom xml tree extensions encounter two problems:
1. Detecting XML without style using code like 'if (document instanceof XMLDocument)' is not possible anymore.
2. Source XML is hidden from extension - it could only see an XSL transformation result.

Proposed solution:

1. XML Viewer defines global javascript function with pseudo-unique name handleWebKitXMLViewerOnLoadEvent(). Extensions could check if this function is defined.
2. WebKit xml viewer is not shown when window.opener is defined. Running  window.open(document.location, '_self') will turn xml viewer off.

Putting all together, extension with logic like

if (currentDocumentIsXMLWithoutStyle()) 
  showCustomXMLViewer()

Could be rewritten like  

if (typeof(window['handleWebKitXMLViewerOnLoadEvent']) == 'function')
  window.open(document.location, '_self');
else if (currentDocumentIsXMLWithoutStyle()) 
  showCustomXMLViewer();

The worst impact would be triggering one-time page reload for pages having 'handleWebKitXMLViewerOnLoadEvent()' function defined for some reason.

[Adam Barth]

Comment on attachment 85682 [details]
Patch

I like the approach, but we require a ChangeLog with every patch.

[Adam Barth]

> 1. XML Viewer defines global javascript function with pseudo-unique name handleWebKitXMLViewerOnLoadEvent(). Extensions could check if this function is defined.
> 2. WebKit xml viewer is not shown when window.opener is defined. Running  window.open(document.location, '_self') will turn xml viewer off.

(2) Seems like a hack.  I'd rather avoid that.  That make the user experience very unpredictable in normal operation. 

> Could be rewritten like  
> 
> if (typeof(window['handleWebKitXMLViewerOnLoadEvent']) == 'function')
>   window.open(document.location, '_self');
> else if (currentDocumentIsXMLWithoutStyle()) 
>   showCustomXMLViewer();

That will make the page flash, right?  It will also require going back to the network.

Maybe that means I don't like the approach...  :(

[Pavel Feldman]

> (2) Seems like a hack.  I'd rather avoid that.  That make the user experience very unpredictable in normal operation. 

This is not a hack, this is a requirement. We can't enable XMLViewer in case there is a chance that someone holds a reference to the doc. He might apply transformation / rewrite doc, etc.

> That will make the page flash, right?  It will also require going back to the network.
> 

Yes, this does not sound nice. But we should give extensions a chance to not regress. We will think of a better approach, but for now it seems like it is a reasonable workaround.

[Pavel Feldman]

One other thing I'd like to see implemented is a button in the UI that goes back to the original XML view (text-only). Pressing it would go back to xml content and hence trigger extension's rendering.

[Vsevolod Vlasov]

> > That will make the page flash, right?  It will also require going back to the network.
> Yes, this does not sound nice. But we should give extensions a chance to not regress. We will think of a better approach, but for now it seems like it is a reasonable workaround.

It might also worth noting, that all extensions that I've seen also make page flash because they apply their logic after default xml rendering (text only). 
The only reason native xml viewer is not flashing is because in this case XSL is applied before default rendering. So there is no any regression in this part.

[Vsevolod Vlasov]

Created attachment 85699 [details]
Patch with fixes

Added changelog.

An approach is changed a little. Now the suggested approach for extension is to use the following code:

if (typeof(window.reloadWithWebKitXMLViewerDisabled) == 'function')
  window.reloadWithWebKitXMLViewerDisabled();
else if (currentDocumentIsXMLWithoutStyle()) 
  showCustomXMLViewer();

[Adam Barth]

(In reply to comment #6)
> > (2) Seems like a hack.  I'd rather avoid that.  That make the user experience very unpredictable in normal operation. 
> 
> This is not a hack, this is a requirement. We can't enable XMLViewer in case there is a chance that someone holds a reference to the doc. He might apply transformation / rewrite doc, etc.

pfeldman and I discussed this in IRC.  Firefox survives without looking at the opener, so hopefully we'll be able to as well.  The problem is that the opener property isn't dispositive w.r.t. whether someone else is going to interact with this document.  Any pages that have the behavior pfeldman is worried about are already broken in Firefox, so it sounds like we should be ok breaking them as well (he's opening another bug on that topic).

[Adam Barth]

Comment on attachment 85699 [details]
Patch with fixes

View in context: https://bugs.webkit.org/attachment.cgi?id=85699&action=review

This patch seems like a hack.  We're randomly dropping something in the global scope that extensions can key off of to trigger a quirk w.r.t. opener that's we're probably going to remove anyway.

> Source/WebCore/ChangeLog:10
> +        No new tests. (OOPS!)

This line will prevent this patch from landing.  Can we write a test for this change?

> Source/WebCore/xml/XMLViewer.xsl:253
> -                    <span> <xsl:value-of select="$xml_has_no_style_message"/> </span> 
> +                    <span> <xsl:value-of select="$xml_has_no_style_message"/> </span>

This change seems unrelated.

[Vsevolod Vlasov]

Created attachment 86771 [details]
Patch that appends xml source to transformed document

Here is a new patch to fix this problem.
I appended source XML to the transformed document so that extensions could work with it.
I also renamed onload method to make it easier to detect XML viewer for extensions.
We are probably going to change the way XML viewer works, but in any case we could stick with the function name and id of the element with XML source.

This solution will not cause page flashing or reloading data from network.

[Vsevolod Vlasov]

Created attachment 86775 [details]
Patch with fixes

[WebKit Commit Bot]

Comment on attachment 86775 [details]
Patch with fixes

Rejecting attachment 86775 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'build-..." exit_code: 2

Last 500 characters of output:
...........................................................................................................................................................
fast/css/content ........
fast/css/counters ..........................
fast/css/dumpAsText .
fast/css/dumpAsText/xml-stylesheet-pi-not-in-prolog.xml -> failed

Exiting early after 1 failures. 6889 tests run.
159.05s total testing time

6888 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
4 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/8240116

[Vsevolod Vlasov]

Created attachment 86799 [details]
Patch with css fixes

[WebKit Commit Bot]

Comment on attachment 86799 [details]
Patch with css fixes

Rejecting attachment 86799 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'build-..." exit_code: 2

Last 500 characters of output:
...........................................................................................................................................................
fast/css/content ........
fast/css/counters ..........................
fast/css/dumpAsText .
fast/css/dumpAsText/xml-stylesheet-pi-not-in-prolog.xml -> failed

Exiting early after 1 failures. 6889 tests run.
148.53s total testing time

6888 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
4 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/8245079

[Adam Barth]

Comment on attachment 86799 [details]
Patch with css fixes

This patch lacks a test.  You're effectively creating an extension API with this patch.  If we don't have a test, we're likely to break that API in the future.

[Vsevolod Vlasov]

Created attachment 86919 [details]
Patch with fixes

Fixed failing tests.

Adam Barth: 
I've added a test for this, but since it is currently impossible to run JS on XML viewer,
it is disabled until we remove window.opener check (most probably next week).
Are you OK with that?

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 86919 [details]:

animations/suspend-resume-animation.html bug 48161 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 86919 [details]
Patch with fixes

Clearing flags on attachment: 86919

Committed r81962: <http://trac.webkit.org/changeset/81962>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/81962 might have broken Windows 7 Release (Tests)
The following tests are not passing:
http/tests/websocket/tests/frame-length-skip.html

[Adam Barth]

> Adam Barth: 
> I've added a test for this, but since it is currently impossible to run JS on XML viewer,
> it is disabled until we remove window.opener check (most probably next week).
> Are you OK with that?

Where is the test?

@pfeldman: Please don't r+ patches that while we're still discussing them.  That's pretty rude.

[Pavel Feldman]

> Where is the test?
> 
> @pfeldman: Please don't r+ patches that while we're still discussing them.  That's pretty rude.

@abarth: the test has been landed as a part of the patch. However, as vsevik mentioned, neither test nor semi-open API work unless opener check is removed. What do I miss?

[Pavel Feldman]

> @abarth: the test has been landed as a part of the patch. However, as vsevik mentioned, neither test nor semi-open API work unless opener check is removed. What do I miss?

(as I understand, opener check will be removed in another patch and the test will get enabled)

[Adam Barth]

(In reply to comment #24)
> > Where is the test?
> > 
> > @pfeldman: Please don't r+ patches that while we're still discussing them.  That's pretty rude.
> 
> @abarth: the test has been landed as a part of the patch. However, as vsevik mentioned, neither test nor semi-open API work unless opener check is removed. What do I miss?

Ah, I see now that he posted a new patch.  My apologies.

[Adam Barth]

> I've added a test for this, but since it is currently impossible to run JS on XML viewer,
> it is disabled until we remove window.opener check (most probably next week).
> Are you OK with that?

Test looks great.

[Vsevolod Vlasov]

Some notes about changes on this issue:

1. As discussed, test was enabled (http/tests/xmlviewer/extensions-api.html)

2. The api for extension itself was changed. Since extension do not have access to global functions, the extension should look for an element with id 'webkit-xml-viewer-source-xml'. This could be done in onload handler.

Created attachment 87954 [details]
Test HTML for extension processing of native webkit xml rendering

Created attachment 87955 [details]
Screen shot of extension's rendering via replacing the document with the source xml from native webkit xml rendering

While doing some preliminary work to prepare my extension for when this new functionality makes it to the browsers, I noticed a couple of issues:

When the source xml contains a CDATA section, it gets rendered as a comment.  The "<![CDATA[...]]>" gets changed to "<!--[CDATA[...]]-->". When  extensions attempt to process the source xml in the webkit-xml-viewer-source-xml DIV, this change has already occurred. I suppose they could look for this specific type of comment and drop !-- and -- then apply their normal CDATA rendering.

Another issue I'm seeing is that regardless of the source xml's element/attribute name capitalization, I cannot seem to get it to stick. If I serialize the document to a string, the element/attribute names all get lowercased. If I replace the original document with the contents of webkit-xml-viewer-source-xml DIV and attempt to render it, the element names are UPPERCASED, attribute names are lowercased. If I serialize the replaced document, they are lowercased. For example, the source element <RooT/> will either come out as <root/> when serialized or <ROOT/> when the document is replaced. I don't encounter either of these capitalization issues in my extension in Chrome 10, which doesn't include the native webkit xml rendering, when I serialize or replace a document.

I'll attach a test HTML file that shows the CDATA and lowercasing issue and also a screenshot of how my extension renders the xml when the original document is replaced with the content of the webkit-xml-viewer-source-xml DIV.

I'm assuming the attached test HTML will be similar as far as markup is concerned as to what to extensions can expect to work with after the native webkit rendering is done. If I'm off track anywhere, let me know.