Bug 32706

Summary:

Product:

WebKit

Reporter:

Charles Wei <charles.wei>

Component:

WebCore Misc.

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

charles.wei, commit-queue, dbates, staikos, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
patch	manyoso: review-
Layout tests	none
another patch	none

Charles Wei

Reported 2009-12-18 03:19:43 PST

Attachments
patch (3.01 KB, patch) 2009-12-20 23:46 PST, Charles Wei	manyoso: review-	Details Formatted Diff Diff
Layout tests (3.03 KB, patch) 2009-12-22 16:50 PST, Daniel Bates	no flags	Details Formatted Diff Diff
another patch (4.98 KB, patch) 2009-12-22 19:44 PST, Charles Wei	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Charles Wei

Comment 1 2009-12-20 23:46:47 PST

Created attachment 45308 [details] patch This patch fixes the problem "contents inside <noscript> is taking effect even when Javascript is enabled, this happens only when XHTMLMP is enabled". <noscript> is enabled when XHTMLMP is enabled, but the implementation only hides the contents inside <noscript> when Javascript is turned on, which is incorrect. The correct solution is to skip the parsing of all the contents inside <noscript> if Javascript is turned on .

WebKit Review Bot

Comment 2 2009-12-20 23:49:18 PST

style-queue ran check-webkit-style on attachment 45308 [details] without any errors.

Adam Treat

Comment 3 2009-12-21 06:13:02 PST

Comment on attachment 45308 [details] patch Can you programmatically turn on/off JavaScript and test rather than just assume it is on and only test that case? Also it isn't clear to me why the change to HTMLParser should be XHTMLMP only... If JS is on, then noscript should be skipped regardless of whether XHTMLMP is enabled or not, right? Also what about the previous changes to HTMLNoSCriptElement?

Daniel Bates

Comment 4 2009-12-22 16:50:09 PST

Created attachment 45411 [details] Layout tests A DRT test that checks that both <meta> and <script> are not processed within <noscript> under XHTML-MP.

Charles Wei

Comment 5 2009-12-22 19:44:37 PST

Created attachment 45415 [details] another patch Another patch that addresses the comments of the reviewer

WebKit Review Bot

Comment 6 2009-12-22 19:48:50 PST

style-queue ran check-webkit-style on attachment 45415 [details] without any errors.

Eric Seidel (no email)

Comment 7 2009-12-22 20:25:24 PST

Comment on attachment 45308 [details] patch Clearing cq+ on this obsolete patch.

Eric Seidel (no email)

Comment 8 2009-12-22 22:43:13 PST

Comment on attachment 45415 [details] another patch The change looks fine, but I can't understand what you're trying to say in the ChangeLog.

Daniel Bates

Comment 9 2009-12-23 10:13:00 PST

(In reply to comment #1) > The correct solution is to skip the parsing of all the contents inside > <noscript> if Javascript is turned on . Note, by section 9.2.1.2 of the XHTML-MP 1.1 spec., we must process <noscript> even when scripting is enabled so long as we have encountered an script with an unsupported scripting language <http://www.openmobilealliance.org/Technical/release_program/docs/Browsing/V2_2-20061020-A/OMA-WAP-XHTMLMP-V1_1-20061020-A.pdf>.

Daniel Bates

Comment 10 2009-12-23 11:05:10 PST

(In reply to comment #0) > <noscript> is designed for browsers that doesn't support Javascript , or > Javascript is disabled. But It's working even when Javascript is enabled > with WebKit, this happens when XHTMLMP is enabled. > > to verify , use the following html snippets: > > <html> > <noscript> > <meta http-equiv=refresh content="0; URL=http://www.google.com" /> > </noscript> > > <body> > If you see this text and not re-directed to google when your browser is > Javascript enabled, it succeeds, otherwise it fails . > </body> > </html> I am unclear from your snippet whether this is a valid XHTML-MP document and whether this document would or should be interpreted as being an XHTML-MP document. I mean, it's missing a DOCTYPE and an xml namespace. So, how does the browser know to interpret this as XHTML-MP? by file extension? by MIME type "application/vnd.wap.xhtml+xml"? when XHTML-MP is enabled in WebKit do we just interpret all documents as XHTML-MP? some combination? (I briefly looked through some of the XHTML-MP code, and from my understanding, it looks for the DOCTYPE and xml namespace declarations as well as accepts the MIME type "application/vnd.wap.xhtml+xml".) I would suggest only using the XHTML-MP logic when we clearly know that the document we received is an XHTML-MP document since by the HTML 5 spec. <noscript> has a precisely defined behavior under HTML, but has no meaning under XHTML (*). Hence, the expected result of interpreting your example as XHTML should be a redirect (regardless of whether JavaScript is enabled). And the expected result of interpreting the document as HTML with JavaScript enabled is no redirect. Assuming JavaScript is enabled, then interpreting your example as an XHTML-MP document should not cause a redirect. But, consider the slightly modified example: <html> <script type="unsupported-script-lang">...</script> <noscript> <meta http-equiv=refresh content="0; URL=http://www.google.com" /> </noscript> <body> If you see this text and not re-directed to google when your browser is Javascript enabled, it succeeds, otherwise it fails . </body> </html> Assume JavaScript is enabled and suppose this is interpreted as being an XHTML-MP document. Then by section 9.2.1.2 of the XHTML-MP 1.1 spec.<http://www.openmobilealliance.org/Technical/release_program/docs/Browsing/V2_2-20061020-A/OMA-WAP-XHTMLMP-V1_1-20061020-A.pdf>, we must process the <noscript> since we first encountered a <script> with an unsupported scripting language (i.e. "unsupported-script-lang"). So, the expected result is a redirect. (*) Section 4.3.2 of the HTML 5 spec. states "...the noscript element is handled differently by the HTML parser based on whether scripting was enabled or not when the parser was invoked....The noscript element is only effective in the HTML syntax, it has no effect in the XHTML syntax" (http://dev.w3.org/html5/spec/Overview.html#the-noscript-element).

Charles Wei

Comment 11 2009-12-24 06:50:06 PST

> I am unclear from your snippet whether this is a valid XHTML-MP document and > whether this document would or should be interpreted as being an XHTML-MP > document. I mean, it's missing a DOCTYPE and an xml namespace. So, how does the > browser know to interpret this as XHTML-MP? by file extension? by MIME type > "application/vnd.wap.xhtml+xml"? when XHTML-MP is enabled in WebKit do we just > interpret all documents as XHTML-MP? some combination? > > (I briefly looked through some of the XHTML-MP code, and from my understanding, > it looks for the DOCTYPE and xml namespace declarations as well as accepts the > MIME type "application/vnd.wap.xhtml+xml".) > > I would suggest only using the XHTML-MP logic when we clearly know that the > document we received is an XHTML-MP document since by the HTML 5 spec. > <noscript> has a precisely defined behavior under HTML, but has no meaning > under XHTML (*). Hence, the expected result of interpreting your example as > XHTML should be a redirect (regardless of whether JavaScript is enabled). And > the expected result of interpreting the document as HTML with JavaScript > enabled is no redirect. > > Assuming JavaScript is enabled, then interpreting your example as an XHTML-MP > document should not cause a redirect. But, consider the slightly modified > example: > > <html> > <script type="unsupported-script-lang">...</script> > <noscript> > <meta http-equiv=refresh content="0; URL=http://www.google.com" /> > </noscript> > > <body> > If you see this text and not re-directed to google when your browser is > Javascript enabled, it succeeds, otherwise it fails . > </body> > </html> > > Assume JavaScript is enabled and suppose this is interpreted as being an > XHTML-MP document. Then by section 9.2.1.2 of the XHTML-MP 1.1 > spec.<http://www.openmobilealliance.org/Technical/release_program/docs/Browsing/V2_2-20061020-A/OMA-WAP-XHTMLMP-V1_1-20061020-A.pdf>, > we must process the <noscript> since we first encountered a <script> with an > unsupported scripting language (i.e. "unsupported-script-lang"). So, the > expected result is a redirect. > > (*) Section 4.3.2 of the HTML 5 spec. states "...the noscript element is > handled differently by the HTML parser based on whether scripting was enabled > or not when the parser was invoked....The noscript element is only effective in > the HTML syntax, it has no effect in the XHTML syntax" > (http://dev.w3.org/html5/spec/Overview.html#the-noscript-element). Thanks, Daniel, for the comment. This bug is introduced by XHTML-MP for HTML document -- when XHTML-MP is enabled, it's causing regressions for HTML documents , and that's what my patch tries to fix. When XHTML-MP is enabled, it still distinguishes XHTML-MP document and HTML documents, and uses different parsers for different documents. The documents will be distinguished by MIME type /file extension. You are right that we should only use XHTML-MP logic when we know that the document IS a XHTML-MP document. The problem is, when the author of XHTML-MP introduced this feature to WebKit, it causes regressions to HTML document by disabling some code of HTMLParser with : #if !ENABLE(XHTMLMP) original HTMLParser code for <noscript> processing #endif That causes problems with <noscript> processing for HTML documents -- contents inside <noscript> are always parsed and processed no matter Javascript is enabled or not, which is incorrect for HTML document. That's my patch tries to fix -- to make contents inside <noscript> ignored when Javascript is enabled for HTML document. Hope this makes it clear . Thanks

Maciej Stachowiak

Comment 12 2009-12-28 01:56:09 PST

Comment on attachment 45415 [details] another patch Tests look correct and I agree the #ifdef is bogus. r=me

WebKit Commit Bot

Comment 13 2009-12-28 22:41:36 PST

Comment on attachment 45415 [details] another patch Clearing flags on attachment: 45415 Committed r52609: <http://trac.webkit.org/changeset/52609>

WebKit Commit Bot

Comment 14 2009-12-28 22:41:41 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.