90664 - [quirks] Problem with nested links and tables

Status: RESOLVED FIXED

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

Description

[Tony DeJoie]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:0.9.1+) Gecko/20010628
BuildID:    201062822

Clicking on any of the left side links(on the www.nsxuppereast.com site) (for
example "EVENTS") results in error about url not found - seems the space in the
url is not being parsed correctly. If the EVENTS link is selected to open in a
new window, then it works ok.

Reproducible: Always
Steps to Reproduce:
1.Go to http://www.nsxuppereast.com
2.Click on "Events" (left side of page)

Comment 1

[Francisco León]

Works for me on today's branch build, linux
http://www.nsxuppereast.com/NewFiles/Upcoming%20Events.html %20 being the char
that handles the space

Comment 2

[Jens-Uwe]

WFM: linux 2001071223

Comment 3

[Ashley Bischoff (blog at handcoding.com)]

WFM 2001071214 Win2k. Reporter: Try a more recent build.

FWIW, this bug did seem to occur on builds from a couple days ago, but I don't
see it in the current builds.

Comment 4

[Garth Wallace]

Technically, accroding to RFC 1738, a URL with an embedded space is an invalid 
URL.

Comment 5

[Ashley Bischoff (blog at handcoding.com)]

Garth: Good point on the RFC. Either way, though, this bug is still RESOLVED.

Comment 6

[Tony DeJoie]

This still fails for me (Build ID: 2001071803 Windows 2000) and (Build ID:
2001071722 Solaris)

When I click on the 'Events' link on www.nsxuppereast.com I get the following
message:

Not Found
The requested URL /NSX UpperEast.data/Components/Component.html was not found on
this server.

Comment 7

[Asa Dotzler [:asa]]

hard to believe that we do this right on mac, win32 and linux and not solaris.
Over to necko.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Okee.  Here is the deal.  the page has:

<a href="../NSX%20UpperEast.data/Components/Component.html">
<table> <tr> <td>
<a href="NewFiles/Upcoming%20Events.html">EVENTS</a>
</table></tr></td>
</a>

Note the <a> tag around the table.  Now comes the interesting part.  On Linux,
clicking on that link goes to "NewFiles/Upcoming%20Events.html" (linux build
2001-07-26-08).  On Solaris (solaris build 2001-07-25-22) it goes to
"../NSX%20UpperEast.data/Components/Component.html", which gives a 404.

_But_ only the actual links in the table are active.  Over to parser, but it
could be that the code that's supposed to get the href of the parent anchor is
failing...

Comment 9

[harishd]

I can swear ( umpt.. )that this is not a parser issue! 

Comment 10

[Boris Zbarsky [:bzbarsky]]

Okee.  :)  Over to XP Apps then.

Comment 11

[Boris Zbarsky [:bzbarsky]]

um. reassign for real.

Comment 12

[benc]

Garth: I think a space that is encoded is acceptable, unecoded spaces are what
is prohibited...

Probably the summary should be updated as well.

Comment 13

[Denis Antrushin]

I can reproduce this problem on both Solaris and Linux (intel).
When I move mouse over link, status bar displays
'http://www.nsxuppereast.com/NewFiles/Upcoming%20Events.html',
but when I click the link, browser goes to
http://www.nsxuppereast.com/NSX%20UpperEast.data/Components/Component.html

Mozilla versions:
Linux:   Mozilla/5.0 (X11; U; Linux i686; en-US; rv:0.9.4+) Gecko/20010921
Solaris: Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:0.9.4+) Gecko/20010910

Comment 14

[Paul Chen]

Umm, I don't know why this is an xpapps issue. This is a layout issue. 
Personally, if you're going to do this:

<a href="foopy.html">
<table>
<a href="bugger.html"> ... </a>
</table>
</a>

ou deserve to be slapped upside the head with a cold fish of some kind. Or, 
better yet, don't use GoLive. Can't you use FrontPage like everyone else? 
(Besides, I don't see them disparaging FrontPage nor Microsoft, so they aren't 
violating the new FP 2002 EULA ;-) )

The question is why is the encompassing anchor href getting precedence over the 
actual anchor that the user is clicking. By the way, this works ok under IE, 
surprise.

Reassigning to layout

Comment 15

[Marc Attinasi]

Attachment: testcase from pchen's comment

Comment 16

[Marc Attinasi]

Harish, ol' buddy, I think it is a parser issue, actually.

The markup

<a href="http://www.mozilla.org/">
 <table>
  <a href="http://www.attinasi.org/"> Linq </a>
 </table>
</a>

(also in attached testcase) produces this content model:

   a@02F52470 href=http://www.mozilla.org/ refcount=4<
     Text@02F52300 refcount=3<\n>
     Text@02F53090 refcount=3<Linq \n>
     table@02F521E0 refcount=4<
       Text@02F52120 refcount=2<\n >
     >
     Text@02F52180 refcount=3<\n>
   >

Note that there is only ONE anchor - the inside anchor is not in the content
model at all, so it will be hard for layout to do anything with it ;)

Interestingly, if the markup is changed to put in a TR and TD as it should have,
then the problem goes away.

Comment 17

[harishd]

uh! :-/ ok ok....it's the parser

<a href="http://www.mozilla.org/">
<table>
<a href="http://www.attinasi.org/"> Linq </a>
</table>
</a>

Basically, <a> cannot nest. The inner <a> is misplaced inside the table and
hence is getting thrown out of <table>. That is, inner <a> is getting inserted
right after outer <a>. This causes the parser to discard inner <a>. 

May be we should close out the outer <a> on encountering <table>. 

Comment 18

[Christopher Hoess (gone)]

Changing component.

Comment 19

[harishd]

The problem ( demonstrated with attinasi's testcase ) is caused by
nsHTMLTokenizer::ScanDocStructure, that checks for wellformedness. If well
formed the parser would allow inline ( ex. <A> ) to contain block ( <TABLE> ).
There is yet another bug ( bug 100397 ) that suggests disabling
ScanDocStructure. I'm sort a convinced that this feature should be diabled. Marc?

Comment 20

[harishd]

patch in bug 100397 should fix this problem too.

Comment 21

[harishd]

out of time :-( --> 0.9.7

Comment 22

[harishd]

Unfortunately, disabling the check for wellformedness seems to have a negative
performance impact. So, there is no quick fix to this problem. Moving to 0.9.9

Comment 23

[harishd]

This bug has been marked "future" because the original netscape engineer working 
on this is over-burdened. If you feel this is an error, that you or another
known resource will be working on this bug,or if it blocks your work in some way 
-- please attach your concern to the bug for reconsideration. 

Comment 24

[Jonas Jørgensen]

This has *nothing* to do with spaces. Changing summary.

Comment 25

[Boris 'pi' Piwinger]

*** Bug 145961 has been marked as a duplicate of this bug. ***

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1

This patch actually fixes a couple of bugs. The idea is to enable RS handling
in more situations, namely, those where we are going to have nested anchor
(<a>) tags. This makes us more compatible with the W3C specs and also fixes the
remaining issue in this bug.

Comment 27

[Johnny Stenback (:jst)]

Comment on attachment 171847 [details] [diff] [review]
patch v1

r=jst

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 171847 [details] [diff] [review]
patch v1

Switching this request to brendan since I've seen him sr stuff in htmlparser
code and because dmose is swamped.

Comment 29

[Brendan Eich [:brendan]]

Comment on attachment 171847 [details] [diff] [review]
patch v1

Looks righteous to me.

/be

Comment 30

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked in.

Comment 31

[Blake Kaplan (:mrbkap) (inactive)]

*** Bug 286242 has been marked as a duplicate of this bug. ***

Comment 32

[HJ]

Didn't attachment 171847 [details] [diff] [review] also fix http://secunia.com/advisories/14568/ ? I think
it did, but I could be (totally) wrong, so that's why I ask.

Comment 33

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #32)
> Didn't attachment 171847 [details] [diff] [review] [edit] also fix http://secunia.com/advisories/14568/
? I think
> it did, but I could be (totally) wrong, so that's why I ask.

It partially fixed it, but at a higher level than a correct fix would be
(namely, in the HTML parser rather than in event handling). See bug 288460 for
another version of the same problem.

Comment 34

[Blake Kaplan (:mrbkap) (inactive)]

*** Bug 139626 has been marked as a duplicate of this bug. ***