Bug 48493 – Rendered <noscript> tag if XHTMLMP feature is enable.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Heedol Lee]

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.

[Heedol Lee]

Created attachment 72129 [details]
Render problem for noscript tag.

[kyounga]

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?

[kyounga]

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.

[Adam Barth]

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?

[kyounga]

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.

[WebKit Review Bot]

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.

[kyounga]

Comment on attachment 73314 [details]
patch 

>Index: WebCore/ChangeLog
>===================================================================
>--- WebCore/ChangeLog	(revision 71478)
>+++ WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,13 @@
>[phone]  kyounga  <[e-mail]>
>+
>+        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  <[e-mail]>
> 
>         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;
>

[kyounga]

Created attachment 73321 [details]
patch - including ChangeLog and ChangeLog's style

tab is removed in changeLog

[kyounga]

(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.

[Eric Seidel (no email)]

Comment on attachment 73321 [details]
patch - including ChangeLog and ChangeLog's style

Looks OK.

[Eric Seidel (no email)]

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
> [phone]  kyounga  <[e-mail]>

Your ChangeLog entry should have your full name.

[kyounga]

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.

[Adam Barth]

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.

[WebKit Commit Bot]

Comment on attachment 76515 [details]
Including my full name in ChangeLog

Clearing flags on attachment: 76515

Committed r74059: <http://trac.webkit.org/changeset/74059>