We are using webkit engine for our product. I found rendering issue related to XHTMLMP feature. Rendered all <noscript> tag if XHTMLMP feature is enable. Please refer to attached jpg file. Test step. 1. Build WebKit-r68882 nigtly version on window XP after enable ENABLE_XHTMLMP feature. 2. Go to google news site. (http://news.google.com/nwshp?hl=en&tab=wn) 3. You can see all noscript source code on the screen. Our product used WebKit-r56912 version with enable XHTMLMP feature. This version was worked well for <noscript> tag. It seems regression issue. I think <noscript> tag was skipped by HTMLParser::noscriptCreateErrorCheck() on WebKit-r56912 version. As you know, webkit engine was changed about parser part since Changeset 60055. So I tried to change some source code to fix it like as below. bool HTMLElement::rendererIsNeeded(RenderStyle *style) { #if ENABLE(XHTMLMP) // Changed #if !ENABLE(XHTMLMP) to #if ENABLE(XHTMLMP). if (hasLocalName(noscriptTag)) { Frame* frame = document()->frame(); if (frame && frame->script()->canExecuteScripts(NotAboutToExecuteScript)) return false; } else #endif if (hasLocalName(noembedTag)) { Frame* frame = document()->frame(); if (frame && frame->loader()->subframeLoader()->allowPlugins(NotAboutToInstantiatePlugin)) return false; } return StyledElement::rendererIsNeeded(style); } After changed, <noscript> tag didn't render. It's just temp solution. Please fix it soon. If you have any question, please let me know. Thanks.
Created attachment 72129 [details] Render problem for noscript tag.
Even though script is enabled, the noscript element is inserted in HTMLTreeBuilder using processGenericRawTextStartTag(). It seems that the parser is supposed to support HTML5's requirement. Reference : http://www.whatwg.org/specs/web-apps/current-work/#the-noscript-element. So, I think it is right that the presenting issue is handed by rendererIsNeeded(). But, the noscript element must not be used in XML documents. The noscript element is only effective in the HTML syntax, it has no effect in the XHTML syntax. So, we should ignore noscript element or make it a parse error for XHTML, shall we?
Created attachment 72783 [details] If XHTMLMP is enabled, class "document" has an additional member, m_shouldProcessNoScriptElement. I use this to determine if noscript should be rendered or not. If XHTMLMP is enabled, class "document" has an additional member, m_shouldProcessNoScriptElement. I use this to determine if noscript should be rendered or not.
Comment on attachment 72783 [details] If XHTMLMP is enabled, class "document" has an additional member, m_shouldProcessNoScriptElement. I use this to determine if noscript should be rendered or not. View in context: https://bugs.webkit.org/attachment.cgi?id=72783&action=review We need a ChangeLog and a test. > WebCore/html/HTMLElement.cpp:784 > + if (!document()->shouldProcessNoscriptElement()) What is this bit? How is it computed?
Created attachment 73314 [details] patch If XHTMLMP is enabled, class "document" has an additional member, m_shouldProcessNoScriptElement. I use this to determine if noscript should be rendered or not. The bit(m_shouldProcessNoScriptElement) is computed by default as following in Doucment class constructor. #if ENABLE(XHTMLMP) m_shouldProcessNoScriptElement = !(m_frame && m_frame->script()->canExecuteScripts(NotAboutToExecuteScript)); #endif I mean it same as before. and it can be set by calling setShouldProcessNoscriptElement(). It is called only in XMLDocumentParser.
Attachment 73314 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/html/HTMLElement.cpp']" exit_code: 1 WebCore/ChangeLog:6: Line contains tab character. [whitespace/tab] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 73314 [details] patch >Index: WebCore/ChangeLog >=================================================================== >--- WebCore/ChangeLog (revision 71478) >+++ WebCore/ChangeLog (working copy) >@@ -1,3 +1,13 @@ >+2010-11-07 kyounga <kyounga.ra@gmail.com> >+ >+ Reviewed by NOBODY (OOPS!). >+ >+ <noscript> is rendered with enabled XHTMLMP. >+ https://bugs.webkit.org/show_bug.cgi?id=48493 >+ >+ * html/HTMLElement.cpp: >+ (WebCore::HTMLElement::rendererIsNeeded): >+ > 2010-11-06 Pavel Feldman <pfeldman@chromium.org> > > Reviewed by Simon Fraser. >Index: WebCore/html/HTMLElement.cpp >=================================================================== >--- WebCore/html/HTMLElement.cpp (revision 71020) >+++ WebCore/html/HTMLElement.cpp (working copy) >@@ -778,14 +778,16 @@ PassRefPtr<HTMLCollection> HTMLElement:: > > bool HTMLElement::rendererIsNeeded(RenderStyle *style) > { >-#if !ENABLE(XHTMLMP) > if (hasLocalName(noscriptTag)) { > Frame* frame = document()->frame(); >+#if ENABLE(XHTMLMP) >+ if (!document()->shouldProcessNoscriptElement()) >+ return false; >+#else > if (frame && frame->script()->canExecuteScripts(NotAboutToExecuteScript)) > return false; >- } else >-#endif >- if (hasLocalName(noembedTag)) { >+#endif >+ } else if (hasLocalName(noembedTag)) { > Frame* frame = document()->frame(); > if (frame && frame->loader()->subframeLoader()->allowPlugins(NotAboutToInstantiatePlugin)) > return false; >
Created attachment 73321 [details] patch - including ChangeLog and ChangeLog's style tab is removed in changeLog
(In reply to comment #4) > (From update of attachment 72783 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=72783&action=review > > We need a ChangeLog and a test. Sorry I missed it. I attached the patch including ChangeLog. I tested the reported url with XHTMLMP enabled. In case of disabled XHTMLMP, the code is same. > > > WebCore/html/HTMLElement.cpp:784 > > + if (!document()->shouldProcessNoscriptElement()) > > What is this bit? How is it computed? I added a comment #5. The bit(m_shouldProcessNoScriptElement) is computed by default as following in Doucment class constructor. #if ENABLE(XHTMLMP) m_shouldProcessNoScriptElement = !(m_frame && m_frame->script()->canExecuteScripts(NotAboutToExecuteScript)); #endif It can be set by calling setShouldProcessNoscriptElement() called only in XMLDocumentParser.
Comment on attachment 73321 [details] patch - including ChangeLog and ChangeLog's style Looks OK.
Comment on attachment 73321 [details] patch - including ChangeLog and ChangeLog's style View in context: https://bugs.webkit.org/attachment.cgi?id=73321&action=review Please post a new patch with a fixed ChangeLog, or get someone to fix the patch when landing this for you. > WebCore/ChangeLog:1 > +2010-11-07 kyounga <kyounga.ra@gmail.com> Your ChangeLog entry should have your full name.
Created attachment 76515 [details] Including my full name in ChangeLog I posted a new patch only to leave my full name in ChangeLog and set the review flag to [+] because this solution was reviewd by Eric. Is it OK? If I should re-request for review, let me know. anyone please.
Comment on attachment 76515 [details] Including my full name in ChangeLog A reviewer needs to set the review flag to + for the tools to land the change.
Comment on attachment 76515 [details] Including my full name in ChangeLog Clearing flags on attachment: 76515 Committed r74059: <http://trac.webkit.org/changeset/74059>