Summary: | XML Viewer: extensions can't render original XML | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Pavel Feldman <pfeldman> | ||||||||||||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | abarth, alan.stroop, commit-queue, eric, pfeldman, vsevik, webkit.review.bot | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | PC | ||||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||||
Attachments: |
|
Description
Pavel Feldman
2011-03-12 23:50:39 PST
This should probably be a Chromium bug. (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. 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 on attachment 85682 [details]
Patch
I like the approach, but we require a ChangeLog with every patch.
> 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... :( > (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. 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. > > 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.
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();
(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 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. 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.
Created attachment 86775 [details]
Patch with fixes
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 Created attachment 86799 [details]
Patch with css fixes
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 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.
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?
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 on attachment 86919 [details] Patch with fixes Clearing flags on attachment: 86919 Committed r81962: <http://trac.webkit.org/changeset/81962> All reviewed patches have been landed. Closing bug. 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:
> 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.
> 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?
> @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)
(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. > 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.
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. |