Bug 60685 - Solar Walk For Mac: Info window is blank (HTML5 parser)
Summary: Solar Walk For Mac: Info window is blank (HTML5 parser)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adele Peterson
URL:
Keywords:
Depends on:
Blocks: 64777
  Show dependency treegraph
 
Reported: 2011-05-11 20:22 PDT by Adele Peterson
Modified: 2011-07-18 20:29 PDT (History)
6 users (show)

See Also:


Attachments
patch (7.39 KB, patch)
2011-05-11 20:22 PDT, Adele Peterson
abarth: review-
Details | Formatted Diff | Diff
patch (12.26 KB, patch)
2011-05-12 18:15 PDT, Adele Peterson
no flags Details | Formatted Diff | Diff
patch (12.30 KB, patch)
2011-05-12 18:35 PDT, Adele Peterson
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adele Peterson 2011-05-11 20:22:45 PDT
Created attachment 93238 [details]
patch

<rdar://problem/9253454>

The HTML content in the info window uses a self closing title tag.

Attaching a patch.  I don't normally touch the parser, so I'm not entirely sure of this.  I did test with the app and saw that it fixes the problem.  I also added a test and saw this passes existing tests.
Comment 1 WebKit Review Bot 2011-05-11 20:25:17 PDT
Attachment 93238 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

LayoutTests/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebKit/mac/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 3 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 2 Eric Seidel (no email) 2011-05-11 20:28:11 PDT
Would this be easier to do via one of the JS-based quirks where we auto-inject js into the page?  I'm not sure that could fix this bug though, since I am not sure what would happen if we let everything get parsed inside the <title> and then move it out.
Comment 3 Adele Peterson 2011-05-11 20:29:58 PDT
Good question - I'm not that familiar with that technique.  I'll look into it.
Comment 4 Adam Barth 2011-05-11 20:31:38 PDT
Comment on attachment 93238 [details]
patch

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

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2734
> +        if (m_usePreHTML5ParserQuirks && token.selfClosing()) {

I don't think this what the old parser used to do. The old parser used to "re-parse" the document if the <title> tag ate the whole document.  I wonder if there's a more general solution that would apply to more apps than just solar walk.  In some sense, this is a "double mistake" by the author.  If we can treat the primary mistake (unterminated title tags), that's a mistake that's more likely to have been hit by other apps.
Comment 5 Adam Barth 2011-05-11 20:32:51 PDT
That could lead to a more general solution.  Basically, you'll want to grab the text content of the title tag and document.write it at the end of the document.  That will be a much more faithful simulation of re-parsing than what you've got in this patch.
Comment 6 Eric Seidel (no email) 2011-05-11 20:39:03 PDT
We have a js-based quirks system using user-script injection.  Sam or Andy should know more.
Comment 7 Mark Rowe (bdash) 2011-05-11 20:40:50 PDT
This application is the only case where the difference in <title> parsing behavior has caused a problem since the HTML5 parser was enabled.  If this quirk were to be more widely needed than this single application then it could be beneficial to consider a more general solution.  Since it isn't I think the simple, targeted fix is reasonable.
Comment 8 Adam Barth 2011-05-11 20:43:33 PDT
Comment on attachment 93238 [details]
patch

Marking r- while Adele investigates using the content script approach.  Please feel free to renominate for review if you decide that this approach is better.
Comment 9 Eric Seidel (no email) 2011-05-11 20:46:13 PDT
See:
http://trac.webkit.org/browser/trunk/Source/WebKit/mac/Misc/MailQuirksUserScript.js
http://trac.webkit.org/browser/trunk/Source/WebKit/mac/Misc/OutlookQuirksUserScript.js

For examples of how to do quirks with user-scripts.  I suspect we could do something similar here, with an UnclosedTitleQuirkUserScript.js:
document.write(document.title)

Might just fix the bug with that one line js quirk. :)
Comment 10 Eric Seidel (no email) 2011-05-11 20:46:51 PDT
(If you look at the changesets where those js quirks were added, you can see how WebKit goes about injecting them into Mail and Outlook respectively.)
Comment 11 Adele Peterson 2011-05-12 11:32:39 PDT
When you use these user scripts, is there any way to test the quirk in DRT?
Comment 12 Eric Seidel (no email) 2011-05-12 11:35:19 PDT
Not really.  Part of the point of the user script thing is that it makes the fix platform-only.  One doesn't have to change WebCore at all. :)
Comment 13 Adele Peterson 2011-05-12 18:15:58 PDT
Created attachment 93377 [details]
patch

This patch uses the User Script approach.  I've manually tested Solar Walk with this change.
Comment 14 Adam Barth 2011-05-12 18:25:28 PDT
Comment on attachment 93377 [details]
patch

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

> Source/WebKit/mac/Misc/SolarWalkQuirksUserScript.js:34
> + window.onload = reparseTitle;

I'd use window.addEventListender("load", reparseTitle, false) to make sure we don't get in the way of anyone else who's touching window.onload.

> Source/WebKit/mac/WebView/WebView.mm:678
> +    NSString *scriptPath = [[NSBundle bundleForClass:[WebView class]] pathForResource:@"SolarWalkQuirksUserScript" ofType:@"js"];

Would it make sense to call this reparseTitle.js in case apps besides solar walk need this quirk?  I guess we can rename it in the future were that to happen.
Comment 15 Adele Peterson 2011-05-12 18:35:51 PDT
Created attachment 93384 [details]
patch

Fixes up the JS.  I didn't rename the file because so far all of our user scripts are per-app, and we might need to add further quirks for this application.   Not sure which will come first - more quirks for this app, or more apps that need this quirk.  We'll see.
Comment 16 Darin Adler 2011-05-12 18:36:55 PDT
Comment on attachment 93384 [details]
patch

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

> Source/WebKit/mac/WebView/WebView.mm:686
> +    NSLog(@"contents of user script: %@\n", solarWalkQuirksScriptContents);

Probably should take out this log before landing.

> Source/WebKit/mac/WebView/WebView.mm:688
> +    core(self)->group().addUserScriptToWorld(core([WebScriptWorld world]),
> +                                             solarWalkQuirksScriptContents, KURL(), nullptr, nullptr, InjectAtDocumentEnd, InjectInAllFrames);

WebKit style says not to line subsequent lines up like this. (Too bad editors all seem to want to do it.)
Comment 17 Adele Peterson 2011-05-12 18:51:33 PDT
Committed http://trac.webkit.org/changeset/86407
Comment 18 Eric Seidel (no email) 2011-05-12 18:58:59 PDT
Fantastic!

FYI, I believe these scripts are injected right before onload? (I could be wrong, Sam could correct me.) in which case I doubt the onload hook was needed (aka, we might just have been able to run the scirpt directly).

Thanks for doing this Adele!
Comment 19 Adele Peterson 2011-05-12 19:31:27 PDT
I hit a loader assertion when trying to document.write immediately.  So the event handler fixed that up.

(In reply to comment #18)
> Fantastic!
> 
> FYI, I believe these scripts are injected right before onload? (I could be wrong, Sam could correct me.) in which case I doubt the onload hook was needed (aka, we might just have been able to run the scirpt directly).
> 
> Thanks for doing this Adele!
Comment 21 Adele Peterson 2011-05-16 17:58:07 PDT
Dan fixed the warning for me:
http://trac.webkit.org/changeset/86639
Comment 22 Andy Estes 2011-07-18 20:26:09 PDT
I found a bug with this script where it is accidentally removing stylesheets from the document (since document.write() overwrites the <style> nodes that came before <title />).

I posted a fix at <https://bugs.webkit.org/show_bug.cgi?id=64777>.