Summary: | Solar Walk For Mac: Info window is blank (HTML5 parser) | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adele Peterson <adele> | ||||||||
Component: | New Bugs | Assignee: | Adele Peterson <adele> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, aestes, eric, mrowe, simon.fraser, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 64777 | ||||||||||
Attachments: |
|
Description
Adele Peterson
2011-05-11 20:22:45 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.
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. Good question - I'm not that familiar with that technique. I'll look into it. 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. 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. We have a js-based quirks system using user-script injection. Sam or Andy should know more. 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 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.
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. :) (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.) When you use these user scripts, is there any way to test the quirk in DRT? 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. :) Created attachment 93377 [details]
patch
This patch uses the User Script approach. I've manually tested Solar Walk with this change.
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. 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 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.) Committed http://trac.webkit.org/changeset/86407 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! 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! This caused some build warnings: http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Build%29/builds/29796/steps/compile-webkit/logs/warnings%20%282%29 Dan fixed the warning for me: http://trac.webkit.org/changeset/86639 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>. |