88243 - text is not correctly layouted (problems with TABLE WIDTH=)

Status: VERIFIED FIXED

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

Description

[Eugene Savitsky]

Look at the bottom of this page. Starting at * BAD POINTS text is alligned not
as it should.

Also look at here http://forum.spylog.ru/
And here http://www.virusbtn.com/100/vb100sum.html

BTW I bet I alredy filled a bug for it, but couldn't find it. :(

Comment 1

[Jacek Piskozub]

Attachment: Testcase for the second URL

Comment 2

[Jacek Piskozub]

Attachment: Simplified testcase. It's the <td> tags that create this

Comment 3

[Jacek Piskozub]

Sorry for the <td> remark. I have it now:

<table WIDTH=50%" BORDER=0 ALIGN="CENTER" CELLSPACING=2>

The " sign after 50% makes Mozilla understand the text as WIDTH=50. To make it
evcen stranger, if you write <table "WIDTH=50%">  the effect is identical. Other
browsers show the one " case correct while ignoring the "" enclosd WIDTH completely.

Comment 4

[Jacek Piskozub]

In the first URL (http://forum.spylog.ru) is is the following line 
<table width= cellspacing=0 cellpadding=0 border=0>
that causes trouble. Take note of the width= attribute with no value. It seems
Mozilla gives it a random value. An uninitiated variable? or the value of an
earlier WIDTH attribute?

Comment 5

[Jacek Piskozub]

Bug 88342 about http://msdn.microsoft.com is identical. I'm not sure which
should be duped against which.

Comment 6

[Jacek Piskozub]

I meant bug 88242. Sorry.

Comment 7

[Chris Waterson]

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

Comment 8

[Chris Waterson]

Sounds like the parser is bungling this? harishd, could you take a look? You can 
assign it back to me if it's a core layout problem.

Comment 9

[Jacek Piskozub]

Attachment: Similar testcase but with <TABLE WIDTH=""> like at msdn.microsoft.com

Comment 10

[Jacek Piskozub]

It seems this is a dup. Forget my remarks about %50 read as 50. It seems in all
the testcases width is read as 0, after all.

*** This bug has been marked as a duplicate of 75250 ***

Comment 11

[Bernd]

This not a exact duplicate so I reopen it. The first issues look like

"width=50%"  
the string tmp at
http://lxr.mozilla.org/mozilla/source/content/html/content/src/nsGenericHTMLElement.cpp#2419
is 50%"

if I change the width spec to "width="50%" the tmp string will be 50%.
If we decide to support that kind of loosy html then parser should not return a
" in the value string.

Comment 12

[Bernd]

Attachment: parser issue demonstrated at cells and hr

Comment 13

[harishd]

IMO, the problem is

PRBool
nsGenericHTMLElement::ParseValueOrPercentOrProportional(...)
{
  nsAutoString tmp(aString);
  tmp.CompressWhitespace(PR_TRUE, PR_TRUE);
  PRInt32 ec, val = tmp.ToInteger(&ec);
......
  if (tmp.Length() && tmp.Last() == '%') {/* XXX not 100% compatible with
ebina's code */   <<<<<<<< ----- HERE ------ <<<<<<<<<

Comment 14

[harishd]

Attachment: Tentative patch [ a bit expensive :-( ].

Comment 15

[Bernd]

r = bernd, i had the same idea , but I was as afraid as you about performance
issues  

Comment 16

[harishd]

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

Comment 17

[Heikki Toivonen (remove -bugzilla when emailing directly)]

r=heikki as well

Comment 18

[harishd]

Moving to more realistic target - m0.9.4!

Comment 19

[vidur (gone)]

sr=vidur

Comment 20

[Eugene Savitsky]

http://www.hp.com/ - look at the left bottom. The same issue or a new one?

Comment 21

[Christopher Hoess (gone)]

Hold up a sec.  Does this patch only apply to quirks mode?  The width attribute 
is CDATA and as such should NOT be stripped of whitespace in strict mode (see 
my comments in bug 42287).

Comment 22

[Jacek Piskozub]

Christopher: How do you propose to parse WIDTH=50%" in strict mode? Presently it
is parsed as WIDTH="0". I understand the strict way would be to ignore it, due to
the lack of the first " character (making it equal to WIDTH="100%"). If so, the
present behavior is wrong.

Comment 23

[Jacek Piskozub]

Attachment: Testcase (WIDTH=50%") with a DOCTYPE parsed as strict

Comment 24

[Christopher Hoess (gone)]

I'm shooting from the hip on this (I returned Goldfarb to the library :-), but 
I think you are correct: "attribute value delimiters" have to occur in matched 
pairs.  So <table width='50%">, <table width="50%> and <table width=50%"> 
should all (I think) be equivalent to <table>.  I'll let you know more 
definitively once I've looked over the SGML Handbook again (or would a member 
of the viewing audience care to oblige? :-)

Comment 25

[Johnny Stenback (:jst)]

I don't think Goldfarbs SGML handbook has an answer to this question that is
relevant to HTML, SGML is very strict about how attributes (and everything else)
is parsed, any of the above would simply be parsing errors in SGML, but since
there is no concept of parsing errors in HTML :-) we'll haveto go with common
practice.

Comment 26

[Jacek Piskozub]

Also in strict (or "standards") mode?

Comment 27

[Christopher Hoess (gone)]

Johnny: in quirks mode, we can do whatever we need to support the problem.  (I
assume it's been complained about or this patch wouldn't be appearing.)  My
point is that in strict mode, we should parse this like SGML, get a busted
attribute we don't understand, and silently ignore it, which would produce a
different result than our current behavior (see the latest testcase).  The
reason I'm fussing with Goldfarb is to get a better idea of exactly what is
considered "broken".

As for my earlier tests, I provide the strictly parsed interpretation:
<table width='50%">
<table width="50%>
Tag isn't closed, rest of document interpreted as part of attribute value,
disappears.  An Evangelism bug is filed. :-)

<table width=50%">
attributes containing % signs, etc. need to be quoted.  Attribute not
understood-should drop silently.  This is the case we're currently dealing with.

Comment 28

[Yancey Yeargan]

This seems to be my problem too... BUG 92721

It is my opinion that this behavior is generated inside of the
CAttributeToken::Consume function 
at http://lxr.mozilla.org/seamonkey/source/htmlparser/src/nsHTMLTokens.cpp#1608

I worked around the problem by including a |PRUnichar('"')|  [which is a double
quote inside single quotes] at nsHTMLTokens.cpp#43 but am still not convinced
this is the best approach.  I offer it up here so that others may experiment.  

I will continue to work through the logic of this and get back to you with my
findings.

Comment 29

[harishd]

Fix landed on 08/07/01. Marking FIXED.

FYI: If there are other issues please track it down with a new bug.

Comment 30

[bsharma]

Verified on 2001-08-08-Trunk build on WinNT.

The above url has the same problem as described.

Comment 31

[harishd]

Bindu: could you tell me why you reopened the bug?

Comment 32

[harishd]

The fix was to correct cases like <TABLE WIDTH="80%    ">.

Try attachment (id=41542). Marking FIXED again. 

Comment 33

[Jacek Piskozub]

I fully agree with closing this bug. 

The chip-online URL is just a case of totally bad HTML which got here by
accident. I'll attach a simplified testcase. The narrow column is the result of
putting two 200 pixel wide tables inside a 448 wide table. The thing would not
happen if aditionally the two table did not have an unnecessary <td
colspan="3"></td> element and if the absurd <li> (without a <ul>) element were
not added to make it completely non-standard. Mozilla gave it interpretation if
the absurd code which by accident was different from the interpretations of
other browsers. If it is a bug it should be IMHO filed under Evangelsm.

Comment 34

[Jacek Piskozub]

Attachment: Simplified testcase of the absurd chip-online code (Evangelism?)

Comment 35

[Jacek Piskozub]

Verifying FIXED (WindowsME 2001080804 trunk build)

See above for my comments on the remaining bad URL.

Comment 36

[Randell Jesup [:jesup] (needinfo me)]

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