570309 - [HTML5] CNNMoney unclickable links ("-- >" terminates comment in HTML5)

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect, P2)

Description

[Ryan VanderMeulen [:RyanVM]]

Ever since the HTML5 parser was enabled on trunk, CNNMoney has a strip down the left size where the text is visible, but the links aren't clickable. Furthermore, if I enabled Adblock Plus, there appears to be some sort of overlay in the affected region that's causing the problem.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Attachment: Screenshot with Adblock Plus disabled

Notice that rendering seems OK. However, the links on the left size are not clickable.

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Attachment: Screenshot with Adblock Plus enabled

Notice that with Adblock Plus enabled, it exposes the unclickable region as some sort of overlay.

Comment 3

[j.j.]

Seems related with comment parsing.

<div class="adAreaLC"><!--<b>Sponsored Links:</b>
  <div id="adItem">
  < !--include virtual="/fn_adspaces/personal_finance/credit_debt_mgmt/rate_box/quigo.260x70.ad"-- >
  < !--include virtual="/fn_adspaces/personal_finance/credit_debt_mgmt/rate_box/text1.260x25.ad"-- >
  < !--include virtual="/fn_adspaces/personal_finance/credit_debt_mgmt/rate_box/text2.260x25.ad"-- >
  </div>-->
</div>

Removing < !-- ... -- > fixes it. Will investigate more tomorrow.

Comment 4

[j.j.]

Attachment: unclickable links with and without HTML5 parser

Looks like the comment parsing issue coincides with an other bug.

Comment 5

[j.j.]

Attachment: unckickable links only with HTML5 parser

Comment 6

[j.j.]

Attachment: testcase 3

This is a reduced version of attachment 449496 [details].

Looks like bug 427088.

Comment 7

[j.j.]

There are two problems here:

1. The HTML5 parser diplays parts of the code of comment 3 as text. For example:

   <!--
   < !--x-- >
   < !--y-- >
   -->

   is displayed as   < !--y-- > -->


2. This additional content moves floats in an unexpected manner and
   triggers the bug described in testcase 3 (attachment id=449551).

Comment 8

[Henri Sivonen (:hsivonen)]

zcorpan, Do you recall the rationale for making -- > terminate a comment? Without reparsing, would we be worse if -- > didn't terminate comments?

Comment 9

[Simon Pieters [:zcorpan]]

From research, closing a comment for --\s*> fixes 23 pages out of 129 that have an unclosed comment (and no pages would be worse, according to what I said on irc). (Closing --!> fixes 20.)

http://lists.w3.org/Archives/Public/public-html/2009Apr/0270.html
http://lists.w3.org/Archives/Public/public-html/2009Jun/0191.html
http://krijnhoetmer.nl/irc-logs/whatwg/20090605#l-180

Comment 10

[j.j.]

Just for clarification:  money.cnn.com  is in quirksmode.

Old Gecko parser behavior (which is somewhat unexpected for me):

"-- >" does terminate in standards mode
"-- >" does /not/ terminate in quirksmode

Comment 11

[j.j.]

Actually no, it's not unexpected. It's that funky SGML comment terminating thing. 

Is it really a good idea to terminate at "-- >" since IE don't do this?
Furthermore, it seems insane in a non-SGML world.
What about XML compatibility?
IMHO HTML5 should change for a better future :-)

Comment 12

[j.j.]

(In reply to comment #9)
> http://lists.w3.org/Archives/Public/public-html/2009Apr/0270.html

  and 23 are fixed by making --\s*> 
  terminate comments (matches Gecko, IE, Opera quirks).

Btw, that sentence is wrong (does not match IE8 and Opera pre HTML5 versions).

Comment 13

[Henri Sivonen (:hsivonen)]

(In reply to comment #9)
> From research, closing a comment for --\s*> fixes 23 pages out of 129 that have
> an unclosed comment (and no pages would be worse, according to what I said on
> irc). (Closing --!> fixes 20.)
> 
> http://lists.w3.org/Archives/Public/public-html/2009Apr/0270.html
> http://lists.w3.org/Archives/Public/public-html/2009Jun/0191.html
> http://krijnhoetmer.nl/irc-logs/whatwg/20090605#l-180

Thanks. I think we should treat this as an evangelism bug.

There are three options here:
 1) Breaking some sites.
 2) Breaking other sites.
 3) Reparsing (potential XSS problem, also would involve more code complexity).

(In reply to comment #12)
> (In reply to comment #9)
> > http://lists.w3.org/Archives/Public/public-html/2009Apr/0270.html
> 
>   and 23 are fixed by making --\s*> 
>   terminate comments (matches Gecko, IE, Opera quirks).
> 
> Btw, that sentence is wrong (does not match IE8 and Opera pre HTML5 versions).

Yeah, IE8 and Opera 10.5x don't terminate on --\s*> when not reparsing:
http://software.hixie.ch/utilities/js/live-dom-viewer/saved/499

Comment 14

[Simon Pieters [:zcorpan]]

I think it was a loose statement taking into account reparsing (Opera doesn't reparse in standards mode).

I don't think it was noted which URLs break without --\s*> closing comments, unfortunately, but it should be possible to find them again by inspecting http://philip.html5.org/data/pages-with-unclosed-comments.txt

Comment 15

[Boris Zbarsky [:bzbarsky]]

We should probably keep track of the html5-parser-related evang bugs somehow so we know what the scope of the fallout is...

Comment 16

[Henri Sivonen (:hsivonen)]

(In reply to comment #15)
> We should probably keep track of the html5-parser-related evang bugs somehow so
> we know what the scope of the fallout is...

These can now be tracked by searching for evang bugs that have "[HTML5]" in the summary.

Comment 17

[j.j.]

I really think HTML5 is wrong here. This behavior is just a leftover of Gecko's SGML-style comment parsing, which is widely considered as a wrong decision. [http://ln.hixie.ch/?start=1137799947]

<!-- xxx -- > yyy -->

Testing the above code with various browsers:

"-- >" terminates the comment?

                         strict mode     quirksmode
Opera 9.6, 10            no              no
Opera 10.6               yes             yes
Fx 3.6                   yes             no
WebKit 532, Chrome5      no              no
IE 6,7,8                 no              no
Konqueror 3.5, 4.4       no              no
Dillo 2.2                no              no

Comment 18

[Simon Pieters [:zcorpan]]

The problem is with pages that have

<!-- xxx -- > yyy EOF

Such pages show the yyy in today's browsers thanks to reparsing. The yyy can be all content on the page.

Comment 19

[Simon Pieters [:zcorpan]]

Pages that are broken if -- > doens't close a comment:

From http://philip.html5.org/data/pages-with-unclosed-comments.txt I see:

http://www.robertasstable.com/ - It's only a splash page, though.


From http://philip.html5.org/data/comments-not-closed-but-with-a-gt-after-them.txt I see:

http://ff.netease.com/search.php - only affects the tracker script at the end.
same with other pages from netease.com.
There were also a few that had a comment at the end that ended with -- >, but those would work either way.


Conclusion:
The breakage is less than concluded back in 2009. Either the affected pages have gone away or changed, or James' analysis was incorrect (or both). We might be able to get away with not closing comments for -- > after all. (There were still many pages that depend on --!> closing comments, though.)

Comment 20

[Simon Pieters [:zcorpan]]

http://www.google.com/codesearch?q=%5C-%5C-%5Cs%2B%5C%3E+lang:html
results: 19, seems to be mostly test cases or scripts.

http://www.google.com/codesearch?q=%5C-%5C-%5C%21%5C%3E+lang:html
results: 655

Comment 21

[j.j.]

(In reply to comment #6)
> Created an attachment (id=449551) [details]
> testcase 3
> This is a reduced version of attachment 449496 [details].

Filed bug 570647 for this.

Comment 22

[Henri Sivonen (:hsivonen)]

(In reply to comment #19)
> The breakage is less than concluded back in 2009. Either the affected pages
> have gone away or changed, or James' analysis was incorrect (or both). We might
> be able to get away with not closing comments for -- > after all. (There were
> still many pages that depend on --!> closing comments, though.)

Thank you!

Let's treat this as a potential spec bug and see what happens if we ship and alpha/beta with --\s*> not closing comments.

Comment 23

[Henri Sivonen (:hsivonen)]

Attachment: Experimental patch for testing if things get overall better or worse

Need to see the tryserver results for this, still. The patch makes the minimal changes that make --\s+> not terminate comments. The patch leaved dead code around to ease backing out in case fixing this CNN Money case breaks more stuff elsewhere.

Comment 24

[Eric Seidel]

WebKit has also seen issues with CNN Money with our HTML5 parser implementation: https://bugs.webkit.org/show_bug.cgi?id=41371

Comment 25

[Henri Sivonen (:hsivonen)]

Comment on attachment 452252 [details] [diff] [review]
Experimental patch for testing if things get overall better or worse

Oops. I forgot to request review on this one.

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 452252 [details] [diff] [review]
Experimental patch for testing if things get overall better or worse

I don't really have an opinion on what we should do. But if you think we should test with this patch, then r=me on landing it.

Comment 27

[Henri Sivonen (:hsivonen)]

Comment on attachment 452252 [details] [diff] [review]
Experimental patch for testing if things get overall better or worse

Requesting approval in order to get beta testing on this patch. We need data to know if we want to ship the final release with or without this fix.

Comment 28

[Benjamin Smedberg]

Yeah, let's get this checked in. Please do not mark the bug FIXED until we've decided and made the appropriate spec changes.

Also, shouldn't this have automated tests, no matter which way we ultimately go?

Comment 29

[Henri Sivonen (:hsivonen)]

(In reply to comment #28)
> Yeah, let's get this checked in. Please do not mark the bug FIXED until we've
> decided and made the appropriate spec changes.

Thanks. Landed as http://hg.mozilla.org/mozilla-central/rev/609d101acf8e . Not marking FIXED.

If we want to keep this behavior, I want to clean up the code anyway.

> Also, shouldn't this have automated tests, no matter which way we ultimately
> go?

OK. I'll write one. (I was thinking of this as a testless trial balloon.)

Comment 31

[Eric Seidel]

Once you file the spec bug, WebKit will be happy to change to match.   We've also had the same issue with CNN money (https://bugs.webkit.org/show_bug.cgi?id=41371).

Comment 32

[Henri Sivonen (:hsivonen)]

Posted as a spec bug.

Comment 33

[Henri Sivonen (:hsivonen)]

http://www.w3.org/Bugs/Public/show_bug.cgi?id=10578

Comment 34

[Adam Barth]

I'm looking into matching this patch in WebKit.

Comment 35

[Henri Sivonen (:hsivonen)]

The spec has changed and this is now in test suite. What remains is cleaning up the code that I left in to make it easy to revert this change.

Comment 36

[Henri Sivonen (:hsivonen)]

Attachment: Remove the remaining dead code

Comment 37

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 485337 [details] [diff] [review]
Remove the remaining dead code

rs=me

Comment 38

[Henri Sivonen (:hsivonen)]

http://hg.mozilla.org/mozilla-central/rev/bbec3799beb9