Bug 56263 - XML Viewer: extensions can't render original XML
Summary: XML Viewer: extensions can't render original XML
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-03-12 23:50 PST by Pavel Feldman
Modified: 2011-04-01 19:05 PDT (History)
7 users (show)

See Also:


Attachments
Patch (1.05 KB, patch)
2011-03-14 09:08 PDT, Vsevolod Vlasov
abarth: review-
Details | Formatted Diff | Diff
Patch with fixes (1.65 KB, patch)
2011-03-14 12:11 PDT, Vsevolod Vlasov
abarth: review-
Details | Formatted Diff | Diff
Patch that appends xml source to transformed document (3.16 KB, patch)
2011-03-24 07:25 PDT, Vsevolod Vlasov
pfeldman: review+
pfeldman: commit-queue-
Details | Formatted Diff | Diff
Patch with fixes (3.25 KB, patch)
2011-03-24 08:18 PDT, Vsevolod Vlasov
pfeldman: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch with css fixes (3.25 KB, patch)
2011-03-24 11:02 PDT, Vsevolod Vlasov
abarth: review-
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch with fixes (12.44 KB, patch)
2011-03-25 05:17 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Test HTML for extension processing of native webkit xml rendering (1.72 KB, text/html)
2011-04-01 19:02 PDT, alan.stroop
no flags Details
Screen shot of extension's rendering via replacing the document with the source xml from native webkit xml rendering (166.39 KB, image/png)
2011-04-01 19:04 PDT, alan.stroop
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Feldman 2011-03-12 23:50:39 PST
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.
Comment 1 Adam Barth 2011-03-13 00:07:44 PST
This should probably be a Chromium bug.
Comment 2 Pavel Feldman 2011-03-13 10:47:33 PDT
(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.
Comment 3 Vsevolod Vlasov 2011-03-14 09:08:38 PDT
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.
Comment 4 Adam Barth 2011-03-14 11:15:10 PDT
Comment on attachment 85682 [details]
Patch

I like the approach, but we require a ChangeLog with every patch.
Comment 5 Adam Barth 2011-03-14 11:17:34 PDT
> 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...  :(
Comment 6 Pavel Feldman 2011-03-14 11:30:30 PDT
> (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.
Comment 7 Pavel Feldman 2011-03-14 11:32:02 PDT
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.
Comment 8 Vsevolod Vlasov 2011-03-14 11:37:33 PDT
> > 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.
Comment 9 Vsevolod Vlasov 2011-03-14 12:11:19 PDT
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();
Comment 10 Adam Barth 2011-03-14 12:25:43 PDT
(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).
Comment 11 Adam Barth 2011-03-14 12:27:46 PDT
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.
Comment 12 Vsevolod Vlasov 2011-03-24 07:25:47 PDT
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.
Comment 13 Vsevolod Vlasov 2011-03-24 08:18:42 PDT
Created attachment 86775 [details]
Patch with fixes
Comment 14 WebKit Commit Bot 2011-03-24 10:05:45 PDT
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
Comment 15 Vsevolod Vlasov 2011-03-24 11:02:44 PDT
Created attachment 86799 [details]
Patch with css fixes
Comment 16 WebKit Commit Bot 2011-03-24 11:36:07 PDT
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
Comment 17 Adam Barth 2011-03-24 14:03:59 PDT
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.
Comment 18 Vsevolod Vlasov 2011-03-25 05:17:12 PDT
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?
Comment 19 WebKit Commit Bot 2011-03-25 08:08:45 PDT
The commit-queue encountered the following flaky tests while processing attachment 86919 [details]:

animations/suspend-resume-animation.html bug 48161 (author: cmarrin@apple.com)
The commit-queue is continuing to process your patch.
Comment 20 WebKit Commit Bot 2011-03-25 08:10:49 PDT
Comment on attachment 86919 [details]
Patch with fixes

Clearing flags on attachment: 86919

Committed r81962: <http://trac.webkit.org/changeset/81962>
Comment 21 WebKit Commit Bot 2011-03-25 08:10:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 WebKit Review Bot 2011-03-25 09:48:04 PDT
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
Comment 23 Adam Barth 2011-03-25 10:53:02 PDT
> 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.
Comment 24 Pavel Feldman 2011-03-25 10:56:00 PDT
> 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?
Comment 25 Pavel Feldman 2011-03-25 10:58:36 PDT
> @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)
Comment 26 Adam Barth 2011-03-25 12:00:03 PDT
(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.
Comment 27 Adam Barth 2011-03-25 12:00:47 PDT
> 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.
Comment 28 Vsevolod Vlasov 2011-04-01 13:31:28 PDT
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.
Comment 29 alan.stroop 2011-04-01 19:02:58 PDT
Created attachment 87954 [details]
Test HTML for extension processing of native webkit xml rendering
Comment 30 alan.stroop 2011-04-01 19:04:58 PDT
Created attachment 87955 [details]
Screen shot of extension's rendering via replacing the document with the source xml from native webkit xml rendering
Comment 31 alan.stroop 2011-04-01 19:05:37 PDT
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.