Bug 45693

Created attachment 67443 [details] Patch

There was some talk of doing this at the WebKit layer instead. Maybe that's better? The parsing algorithm isn't really set up to skip over states in the state machine.

Comment on attachment 67443 [details] Patch This is the wrong layer for this hack. We shouldn't poop on the parser just to support some long-deprecated version of Mail.

(In reply to comment #3) > This is the wrong layer for this hack. We shouldn't poop on the parser just to support some long-deprecated version of Mail. I don’t agree. I’m not sure how to resolve this. It seems the wrong process to remove the compatibility mode Apple added, and then debate how to add it back. Maybe we could have this debate before removing it next time?

Adding an application-specific hack to language bindings seems even worse to me.

Seriously, if you find a better place to put this, that’s fine, but please add it back ASAP.

> It seems the wrong process to remove the compatibility mode Apple added, and then debate how to add it back. Maybe we could have this debate before removing it next time? To be clear, the patch earlier today didn't remove the compatibility mode. That code wasn't doing what it claimed. We inadvertently removed the mode when we originally switched to the new tree builder because there wasn't any test coverage to tell us that we broke something.

(In reply to comment #7) > We inadvertently removed the mode when we originally switched to the new tree builder Yes. > because there wasn't any test coverage to tell us that we broke something. I’m not going to debate you on this point. It would be great to have tests for every case; certainly make it a lot easier to rewrite a module that has thorough test coverage. So anyway, while redoing the parser you removed this. Please add it back! Or if you won’t add it back, let me know so I can assign engineers to undo the damage. I can’t ship WebKit until we add it back.

> So anyway, while redoing the parser you removed this. Please add it back! > > Or if you won’t add it back, let me know so I can assign engineers to undo the damage. I can’t ship WebKit until we add it back. I believe that path attached to this bug does add it back. It lacks a test, but I think that's matter of figuring out how to tell DRT to turn on the quirk early enough / adjusting the #ifdefs to allow enabling the quirk in non-Tiger/Leopard builds. If Eric thinks that it should be done at another layer, perhaps he'd be willing to post a patch showing how that would work so we can compare the two and decide which is the better approach?

Could someone post a stack trace of the exception? I don't have access to a Leopard or Tiger machine anymore.

5/6/09 4:54:45 PM Mail[229] An exception was thrown '*** NOT_FOUND_ERR: DOMException 8'. Stack trace: ( 0 CoreFoundation 0x00007fff88552924 __exceptionPreprocess + 180 1 libobjc.A.dylib 0x00007fff81eea4b7 objc_exception_throw + 45 2 CoreFoundation 0x00007fff88500689 -[NSException raise] + 9 3 WebCore 0x00007fff80681fb7 _ZN7WebCore17raiseDOMExceptionEi + 279 4 WebCore 0x00007fff80844ad1 -[DOMRange setEndAfter:] + 65 5 Mail 0x00000001001e6f03 0x0 + 4296961795 6 Mail 0x00000001000d3901 0x0 + 4295833857 7 Mail 0x00000001001e5848 0x0 + 4296955976 8 AppKit 0x00007fff83e08669 -[NSApplication sendAction:to:from:] + 95 9 AppKit 0x00007fff83e085c9 -[NSControl sendAction:to:] + 94 10 AppKit 0x00007fff83e28f9f -[NSCell trackMouse:inRect:ofView:untilMouseUp:] + 1715 11 AppKit 0x00007fff83e65dff -[NSButtonCell trackMouse:inRect:ofView:untilMouseUp:] + 555 12 AppKit 0x00007fff83e27a49 -[NSControl mouseDown:] + 624 13 AppKit 0x00007fff83dd8805 -[NSWindow sendEvent:] + 5409 14 AppKit 0x00007fff83d9d53a -[NSApplication sendEvent:] + 4440 15 Mail 0x000000010002ad02 0x0 + 4295142658 16 AppKit 0x00007fff83c7f6e1 -[NSApplication run] + 474 17 AppKit 0x00007fff83c6df6e NSApplicationMain + 364 18 Mail 0x00000001000e2f30 0x0 + 4295896880 )

Thank you.

There might be a slightly cleaner approach to the parser change if we introduce a new pseudo state. I'll try another version of the patch.

@darin: Is the content being displayed in mail at this time arbitrary HTML or is it mail-generated? I'm looking into just moving the code into WebKit and having it simply reparent all children before the body element into the body element just before onload fires. That should be possible to do entirely in WebKit and not involve WebCore or the parser (or other platforms) at all.

Looks like implementing this as a user script injected at "document end" would be the most elegant. I just have to figure out when to inject it into the page group and which page group to inject into. :)

Created attachment 67482 [details] Patch

I've tested the quirk on my Snow Leopard machine by commenting out the "is mail" check and validating that all pages loaded have document.firstChild.firstChild == document.body. However I can't take this to completion as I don't have a Leopard or Tiger machine to test on (not to mention access to Radar or Mail's source code). Someone from Apple will have to carry this bird home. The patch as-is, should solve mail's problems based on Darin's description above. :)

<rdar://problem/8439720>

Comment on attachment 67482 [details] Patch I'm nominating Eric's patch for review. I'm not sure how to test it, both to make sure it works for Mail and to make sure we don't break it in the future...

@darin, should someone review this patch so we can make progress on this issue? This is one of the few remaining issues with the new parser. I'd like to clean them out and be done with this project. Thanks.

(In reply to comment #20) > @darin, should someone review this patch so we can make progress on this issue? This is one of the few remaining issues with the new parser. I'd like to clean them out and be done with this project. Thanks. I have a Leopard machine and can test this patch. Will report back shortly.

(In reply to comment #19) > (From update of attachment 67482 [details]) > I'm nominating Eric's patch for review. I'm not sure how to test it, both to make sure it works for Mail and to make sure we don't break it in the future... I'm still seeing the NOT_FOUND_ERR logged to Console. I did verify in gdb that the script was being injected, but perhaps it's not being executed by the time Mail does its DOM operations. I'll spend a little time investigating. This patch will also need a layout test and the DRT machinery necessary to enable the quirk.

Created attachment 70557 [details] Patch

I just got a chance to look at this again and figured out why it wasn't working. Mail doesn't specify a group name when initializing its WebViews, so calling _addUserScriptToGroup doesn't work in this case. I changed the patch to get the single-page page group for the WebView and call PageGroup::addUserScriptToWorld() directly, which works.

> ..., which works. Cool! Shall we go with this approach then?

(In reply to comment #25) > > ..., which works. > > Cool! Shall we go with this approach then? Could there be issues with applying this quirk to arbitrary HTML email content? I assume things that typically go in head retain their meaning when head is a child of body rather than html. Also, this quirk seems to only be needed by the WebView created for the signatures pref panel, so it would be nice if this could be isolated to that WebView only.(In reply to comment #25) > > ..., which works. > > Cool! Shall we go with this approach then? I wish there was a way to isolate this quirk to the signature pref pane's WebView. It seems strange to be reparenting the head element in arbitrary HTML email.

> Could there be issues with applying this quirk to arbitrary HTML email content? I assume things that typically go in head retain their meaning when head is a child of body rather than html. In theory, yes, but in practice, it's probably fine. One thing to check is that scripts don't get executed twice if moved in this way. (They shouldn't, but one should check these things.) > I wish there was a way to isolate this quirk to the signature pref pane's WebView. It seems strange to be reparenting the head element in arbitrary HTML email. Is there some distinguishing characteristic of that WebView we can key off of? Maybe it's URL? Maybe something about the content?

(In reply to comment #27) > > Could there be issues with applying this quirk to arbitrary HTML email content? I assume things that typically go in head retain their meaning when head is a child of body rather than html. > > In theory, yes, but in practice, it's probably fine. One thing to check is that scripts don't get executed twice if moved in this way. (They shouldn't, but one should check these things.) I believe Mail disables scripting. > > > I wish there was a way to isolate this quirk to the signature pref pane's WebView. It seems strange to be reparenting the head element in arbitrary HTML email. > > Is there some distinguishing characteristic of that WebView we can key off of? Maybe it's URL? Maybe something about the content? Perhaps; I'll take a look in the debugger.

> Is there some distinguishing characteristic of that WebView we can key off of? Maybe it's URL? Maybe something about the content? The only distinguishing characteristic I can come up with is that the WebView is decoded from a NIB file (initWithCoder) as opposed to email WebViews (initWithFrame). The WebView doesn't have a frame name, group name, or main document URL, so I'm not sure what else to key off of.

Comment on attachment 70557 [details] Patch I think the risk of moving the <head> into the <body> of all emails in Leopard/Tiger versions of mail, is fantastically small. But I may also be used to the chromium update-every-few-days model where regressions are easier to fix when you get them wrong. I'm ready to r+ this patch, but I shouldn't really... since I wrote it. :)

Comment on attachment 70557 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=70557&action=review r=me but I’d like to see the style issues fixed > WebKit/mac/WebView/WebView.mm:648 > ++ (NSString*)_mailQuirksUserScript > +{ > + static NSString* mailQuirksScript = nil; > + if (!mailQuirksScript) { > + NSString *scriptPath = [[NSBundle bundleForClass:[WebView class]] pathForResource:@"MailQuirksUserScript" ofType:@"js"]; > + mailQuirksScript = [[NSString alloc] initWithContentsOfFile:scriptPath]; > + } > + return mailQuirksScript; > +} The one-time initialization code should not use an if statement. Instead it should be in a separate function. > WebKit/mac/WebView/WebView.mm:658 > + core(self)->group().addUserScriptToWorld(core([WebScriptWorld world]), > + [WebView _mailQuirksUserScript], > + KURL(), > + 0, > + 0, > + InjectAtDocumentEnd, > + InjectInAllFrames); We don’t line up code with parentheses like this. This should be indented normally and not formatted vertically like this.

Thanks for the review Darin. I'll clean up the style issues before landing.

Committed r69622: <http://trac.webkit.org/changeset/69622>