196290 - The tables on the left hand edge of the screen are displayed with a much greater height in Mozilla that IE 5/6 or NS 4.8. It is difficult to select some options as they display off-screen.

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[David Bowerman]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4a) Gecko/20030306
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4a) Gecko/20030306

Not sure if this is due to the use of the a mix of absolute table widths in the
first column and a percentage width for the second column or simply that since
no table height is specified, Mozilla is using a default which is much larger
than other browsers.  As it sits, Mozilla requires me to hit page downs to bring
options into view for selection.  Other nits are that the line which should be
at the bottom of the windows with the Legal, Privacy, etc. items is overwritten
by the table blocks and the page has a large chunk of white space at the bottom.

Reproducible: Always

Steps to Reproduce:
1.connect to support.novell.com
2.place the cursor over the > in the free product support box
3.repeat as necessary

Actual Results:  
The tables are displayed in overheight boxes

Expected Results:  
The boxes in the first column should be slightly larger than the text with the
arrow on the same line as the text.  When the cursor is placed over the arrow in
a box, the new table should be displayed in a box just large enough to contain
the text.

Comment 1

[David Bowerman]

Attachment: Novell Support page displayed by Mozilla 1.4a (2003030608)

Comment 2

[David Bowerman]

Attachment: Novell Support page displayed by Netscape 4.8 (other browsers similar results)

Comment 3

[Boris Zbarsky [:bzbarsky]]

The site sets a "200px" height on each of those menuitems.  I tried to debug
their script to figure out why they do it, and it looks like they use a global
variable and recurse, then try to use it again to mean the thing it pointed to
before the recursion.  Which it no longer does at that point.

In any case, given the CSS they provide the layout is correct.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Oh, and they sniff browser stuff all over, of course.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

Comment 6

[Susie Wyshak]

I see no problem on this site in Mozilla 1.3 or the commercial build of 1.0.2 on
Windows 2000. It looks perfect and does *not* look like the attached screenshots.

Looks like this could be a mozilla 1.4a bug.
Please test other Gecko based builds to see if you see the problem.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Yes, I also only see the problem in 1.4a builds.  But again, I don't see any way
it could be rendered differently given that CSS.

Comment 8

[Bob Clary [:bc] (inactive)]

taking

Comment 9

[Boris Zbarsky [:bzbarsky]]

I looked at the thing some more (God I hate obfuscated JS; Venkman saved my life
here)... in 1.4a, the height is 200px and the line-height is 180px.  In 1.3,
those are 20px and 18px respectively.

The code in question seems to be in http://www.novell.com/inc/nav/main_dom.js
and the line(s) in question are:

r_s.height = 20 + ((row.isHeading) ? 32 : 0) + "px";
r_s.lineHeight = 18 + ((row.isHeading) ? 16 : 0) + "px";

In 1.4a, the "px" seems to be coercing the whole expression to strings, so you
get "20" + ((row.isHeading) ? 32 : 0) + "px" which evaluates to "200px".  In
1.3, the same expression evaluates as (20 + ((row.isHeading) ? 32 : 0)) + "px".

Simple test, just type:

  javascript:alert(20 + 0 + "px")

in the URL bar.  It'll return different things in 1.4a and 1.3.

This behavior changed between trunk build 2003-02-27-08 and trunk build
2003-02-28-08.  Looking at the checkins in that range, the fix for bug 174341
screams out as the culprit.

So I believe I must tender my apologies to Susie...

Comment 10

[Boris Zbarsky [:bzbarsky]]

To brendan; since '+' is left-associative, I'd think that |foo + bar + baz| and
|(foo + bar) + baz| must always return the same result...

Comment 11

[Phil Schwartau]

I think Boris is right. From ECMA-262 Edition 3:

11.6 Additive Operators

Syntax
AdditiveExpression :
MultiplicativeExpression
AdditiveExpression + MultiplicativeExpression
AdditiveExpression - MultiplicativeExpression

11.6.1 The Addition operator ( + )
The addition operator either performs string concatenation or numeric addition.
Production AdditiveExpression : AdditiveExpression + MultiplicativeExpression
is evaluated as follows:

1. Evaluate AdditiveExpression.
2. Call GetValue(Result(1)).
3. Evaluate MultiplicativeExpression.
4. Call GetValue(Result(3)).
5. Call ToPrimitive(Result(2)).
6. Call ToPrimitive(Result(4)).
           etc.
           etc.


In a sum |foo + bar + baz|, |baz| is the MultiplicativeExpression
(including UnaryExpressions as a sub-case), making |foo + bar|
the AdditiveExpression which must be evaluated first in step 1.

Comment 12

[Phil Schwartau]

Testcase added to JS testsuite:

      mozilla/js/tests/ecma_3/Expressions/11.6.1-1.js

Currently failing in the SpiderMonkey shell, passing in Rhino.
Also passes in SpiderMonkey if I pull source from 2003-02-25,
which was before the fix for bug 174341 landed.

The testsuite had contained tests for section 11.6.1 in its
ecma/ directory, but none of them tested left associativity -

Comment 13

[Brendan Eich [:brendan]]

Heavy sigh -- my constant folding fixes for bug 174341 were obviously bad in a
basic way, although they passed the testsuite and saved a lot of space for
lengthy string literal concatenations.  Sorry about that, and thanks for finding
this.

The problem is in js_FoldConstants, the TOK_PLUS case, which aggressively folds
type from number to string, even though later on there's a variable term that
defeats constant-folding altogether.  Patch in a few minutes.

/be

Comment 14

[Brendan Eich [:brendan]]

Attachment: proposed fix

Shaver, remember that loop you suggested could be replaced by cumulative
pn_strcat computation in NewBinary?  It can, but if any term is non-constant,
even though a term is a string literal, then pn_strcat must be cleared.

Boris, thanks for the arduous diagnosis.

Good thing it's still 1.4alpha.

/be

Comment 15

[Brendan Eich [:brendan]]

Attachment: better fix

The last patch would not fold 20 + 32 + "pt", simply because neither of the
first two terms is a string.

This patch simplifies everything via independent flags for whether a TOK_PLUS
list contains at least one string literal term (PNX_STRCAT), and whether such a
list contains at least one non-string, non-number term (my cat will forgive me:
PNX_BADCAT).  When the constant folder considers such a list, it insists on
only the PNX_STRCAT flag being set.

/be

Comment 16

[Phil Schwartau]

The latest patch causes approximately 40 test regressions in the
JS testsuite on both Linux and WinNT, yielding a total of roughly
60 failures instead of the normal 20. I'll attach the results below.

Note: I checked that I applied the patch successfully, by doing a
"diff of diffs" -

Comment 17

[Phil Schwartau]

Attachment: Test failures (regressions only = 38 on WinNT, smopt)

Comment 18

[Phil Schwartau]

Haven't gone into all those tests yet, but here's something funny
about the patched JS shell:

---------------------------  PATCHED SHELL  ---------------------------
js> typeof ('' + 3 + 0)
string
js> '' + 3 + 0
30
js> typeof ('' + 3 + ''.length)
number                           <<<---------------- ?????
js> '' + 3 + ''.length
3                                <<<---------------- ?????


--------------------------  UNPATCHED SHELL  --------------------------
js> typeof ('' + 3 + 0)
string
js> '' + 3 + 0
30
js> typeof ('' + 3 + ''.length)
string
js> '' + 3 + ''.length
30

Comment 19

[Phil Schwartau]

The testcase for this bug itself is failing in the patched shell,
in a way very different from the unpatched shell:


--------------------------  UNPATCHED SHELL  --------------------------
*-* Testcase ecma_3/Expressions/11.6.1-1.js failed:
Bug Number 196290
STATUS: Testing left-associativity of the + operator
Failure messages were:
FAILED!: Section 1 of test -
FAILED!: Expected value '2px', Actual value '11px'

                         etc.
                         etc.


---------------------------  PATCHED SHELL  ---------------------------
*-* Testcase ecma_3/Expressions/11.6.1-1.js failed:
Expected exit code 0, got 3
Testcase terminated with signal 0
Complete testcase output was:
.\ecma_3\Expressions\11.6.1-1.js:149: SyntaxError: missing ; before statement:
.\ecma_3\Expressions\11.6.1-1.js:149: 1 + 1 + NaNa" + 1 + 1 + NaNb"
.\ecma_3\Expressions\11.6.1-1.js:149: ............^


That part of the file tests the + operator via an eval string:

var sEval = '1 +  1 + "a" + 1 + 1 + "b"';
eval(sEval);

It does so via a generic function:

sumThese(1, 1, 'a', 1, 1, 'b');

function sumThese()
{
  var sEval = '';
  var arg;
  var i;

  var L = arguments.length;
  for (i=0; i<L; i++)
  {
    arg = arguments[i];
    if (typeof arg === 'string')
      arg = quoteThis(arg);

    if (i < L-1)
      sEval += arg + ' + ';
    else
      sEval += arg;
  }

  return eval(sEval);
}

function quoteThis(x)
{
  return '"' + x + '"';
}

Comment 20

[Brendan Eich [:brendan]]

Attachment: best fix

All better now.  I renamed PDX_BADCAT to PDX_CANTFOLD and added an early test
for it under the TOK_PLUS/PN_LIST case in js_FoldConstants, to cause that
invocation of the constant folder to bail out.

/be

Comment 21

[Brendan Eich [:brendan]]

Comment on attachment 117525 [details] [diff] [review]
best fix

Oops, this has the followup patch to jsobj.c from bug 94693 in it.  Please
ignore that file, the only ones to review and test are jsparse.[ch].

/be

Comment 22

[Phil Schwartau]

The latest patch doesn't seem to provide left-associativity; the
testcase for this bug fails in the same way it does in the trunk.
Or, more directly:

PATCHED SHELL:

[//d/JS_EXP/mozilla/js/src/WINNT4.0_DBG.OBJ] ./js
js> 1 + 1 + 'a';
11a

I think the patch applied successfully; I did a diff of diffs
to check it. I did not include the jsobj.c part of the patch -

Comment 23

[Brendan Eich [:brendan]]

Comment on attachment 117525 [details] [diff] [review]
best fix

Oh, I suck today.

Engage brain, then patch.  Better luck tomorrow.

/be

Comment 24

[Brendan Eich [:brendan]]

Attachment: an ok fix; i'll reserve superlatives

I'm trying to avoid writing a full-blown constant folder still, and the
simplest way to fix the remaining bug in the last patch is to fold numerical
literal adds aggressively, in NewBinary.  That relieves js_FoldConstants from
having to deal with mixed modes in a long + list that contains only string and
numeric literals, but no more than one numeric literal between any sequence of
string literals.

So 1 + 2 + "pt" is parsed first as (1 + 2), which folds immediately to 3, then
parsed further as 3 + "pt", which folds later to "3pt".

Here's a diff of this patch against the last one, new on the left:

7,8c7,8
< +++ jsparse.c 18 Mar 2003 05:43:15 -0000
< @@ -232,24 +232,51 @@ NewBinary(JSContext *cx, JSTokenType tt,
---
> +++ jsparse.c 18 Mar 2003 05:34:26 -0000
> @@ -232,24 +232,36 @@ NewBinary(JSContext *cx, JSTokenType tt,
42,56d41
< +	   return left;
< +    }
< +
< +    /*
< +	* Fold constant addition immediately, to conserve node space and,
what's
< +	* more, so js_FoldConstants never sees mixed addition and concatenation

< +	* operations with more than one leading non-string operand in a PN_LIST

< +	* generated for expressions such as 1 + 2 + "pt" (which should evaluate

< +	* to "3pt", not "12pt").
< +	*/
< +    if (tt == TOK_PLUS &&
< +	   left->pn_type == TOK_NUMBER &&
< +	   right->pn_type == TOK_NUMBER) {
< +	   left->pn_dval += right->pn_dval;
< +	   RecycleTree(right, tc);
65c50
< @@ -3309,19 +3336,25 @@ js_FoldConstants(JSContext *cx, JSParseN
---
> @@ -3309,19 +3321,25 @@ js_FoldConstants(JSContext *cx, JSParseN
99c84
< +++ jsparse.h 18 Mar 2003 05:43:15 -0000
---
> +++ jsparse.h 18 Mar 2003 05:34:26 -0000

If anyone can think of a smaller way to code a solution, I'm all ears.

/be

Comment 25

[Phil Schwartau]

The latest patch passes the JS testsuite on both WinNT and Linux.
I found no test regressions in either the debug or optimized shell.

Furthermore, I re-checked the JS string concatenation stress test
at bug 174341 comment 15. 

That test runs successfully with this patch applied, and I see no
degradation or difference in performance time vis-à-vis the trunk -

Comment 26

[Brendan Eich [:brendan]]

I checked in with telepathic r=shaver.

/be

Comment 27

[Phil Schwartau]

Verified FIXED.

js> 1 + 1 + 'a'
2a
js> eval("1 + 1 + 'a'");
2a

I ran the entire JS testsuite on WinNT4.0, Linux RH7.2, and Mac OSX
and found no test regressions in either the debug or optimized shell.

I re-checked the JS string concatenation test at bug 174341 comment 15,
and once again found no degradation in time-to-complete.

I will also check http://support.novell.com with tomorrow's Mozilla
trunk build to confirm that the dropdown menus now appear correctly,
and report back here -

Comment 28

[Phil Randal]

Verifed fixed in Mozilla build 2003032004 :-)

Comment 29

[Phil Schwartau]

Also using Mozilla trunk binary 2003032004, on WinNT.

Confirming what Phil reported above; the dropdown menus
at http://support.novell.com now look perfect -

Comment 30

[Phil Schwartau]

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

Comment 31

[Matthias Versen [:Matti]]

Comment on attachment 117558 [details] [diff] [review]
an ok fix; i'll reserve superlatives

r= shaver (transmitted with modern communikation)