305120 - onoverflow event handler not triggered in Deerpark alpha 2

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Colin Bleckner]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b3) Gecko/20050712 Firefox/1.0+

In Deerpark alpha 2, a overflowing multiline textbox doesn't seem to trigger the
onoverflow event handler.  I'll attach a simple XUL file that I used to test this.

Reproducible: Always

Steps to Reproduce:
1. Open the attached XUL page
2. Hit enter until in textbox until scrollbar appears
3. Note difference in behavior in Firefox 1.0 and Deerpark alpha 2

Actual Results:  
In deerpark alpha 2 the label below the textbox doesn't change from "No overflow
yet".

Expected Results:  
In Firefox 1.0 the label below the textbox changes to "Overflow detected" when
the textbox's contents are too tall for the box and to "Underflow detected" when
the textbox's contents fit in the textbox.

This seems like an issue that would have been discovered already, but I couldn't
find anything in bugzilla.  However this is my first ticket filed, so I might
not be searching correctly.  My apologies in advance if this is a dupe.

Comment 1

[Colin Bleckner]

Attachment: Simple XUL file that should trigger onoverflow event

Comment 2

[Hermann Schwab]

Bug seen in:
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20050813 Firefox/1.0+
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20050815 SeaMonkey/1.0a

working:
Mozilla/5.0 (Windows; U; Win98; de-DE; rv:1.7.10) Gecko/20050715 Firefox/1.0.6

Working from Seamonkey, I first tested Firefox 1.0.6, then Seamonkey, and
Seamonkey crashed after I closed Firwefox, started Deerpark and wanted to
copy&paste the testcase URL from Seamonkey to Deerpark. The bug is about a
textbox, and I copied the UA of the browser tested to this Additional Comments
textbox in the bug I was working on in a tab in Seamonkey.

Talkback TB8496600G, DocWatson tells it crashed in  GKWIDGET.DLL, may be related
or not.


Excerpt from testcase:
  <textbox id="overflow-textbox"
           multiline="true"
           rows="2"
           onoverflow="document.getElementById('overflow-results').value =
'Overflow dected';"
           onunderflow="document.getElementById('overflow-results').value =
'Underflow dected';" />
  <label id="overflow-results" value="No overflow yet" />

I can write three lines of text, first two including return, third without.
When I hit the return key after third line is written, the textbox scrolls so
only a few pixels of first line are seen, the 2nd and 3rd line are seen
perfectly, cursor is on an empty 4th line, and a vertical scrollbar has appeared
to the right.
Firefox 1..0.6 shows same behaviour, but hitting the 3rd return changes text
from 'no overflow yet' to 'overflow detected' 

Comment 3

[Martijn Wargers (dead)]

This worked in 2005-04-28 build, but fails in 2005-04-29 build, I think a
regression from fixing bug 289940.

Comment 4

[Brendan Eich [:brendan]]

Regression in 1.0.5+ and Deer Park, nominating.

/be

Comment 5

[Boris Zbarsky [:bzbarsky]]

I suspect is actually fallout from bug 240276.  XUL scrollframes still post the
overflow events, but <textbox> uses an <html:textarea> as the editing area, and
I'm pretty sure that uses an HTML scrollframe (for its anonymous div) as the
scrollframe.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Gah. I guess I'll have to put that back.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Untested! I'm building but I just want to post this before I leave.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 193412 [details] [diff] [review]
fix

OK, I tested this and it works.

I've changed the behaviour of the overflow event so that it only fires when the
state changes from "not overflowing" to "overflowing". This is more efficient,
consistent with the handling of "underflow" and shouldn't cause any problems
... the only user of this event in our tree that I know of (menu scrolling)
works fine with that, and the only other users are presumably a few XUL
authors.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 193412 [details] [diff] [review]
fix

This chunk (with the "-" lines omitted):

>   // first see what changed
>   PRBool vertChanged = PR_FALSE;
>   PRBool horizChanged = PR_FALSE;
> 
>+  if (mVerticalOverflow && childSize.height <= scrollportSize.height) {
>     mVerticalOverflow = PR_FALSE;
>     vertChanged = PR_TRUE;
>+  } else if (!mVerticalOverflow && childSize.height > scrollportSize.height) {
>+    mVerticalOverflow = PR_TRUE;
>     vertChanged = PR_TRUE;
>   }
> 
>+  if (mHorizontalOverflow && childSize.width <= scrollportSize.width) {
>     mHorizontalOverflow = PR_FALSE;
>     horizChanged = PR_TRUE;
>+  } else if (!mHorizontalOverflow && childSize.width > scrollportSize.width) {
>+    mHorizontalOverflow = PR_TRUE;
>     horizChanged = PR_TRUE;
>   }

can now (thanks to your changes) be simplified to:

PRBool newVerticalOverflow = childSize.height > scrollportSize.height;
PRBool vertChanged = mVerticalOverflow != newVerticalOverflow;
mVerticalOverflow = newVerticalOverflow;

PRBool newHorizontalOverflow = childSize.width > scrollportSize.width;
PRBool horizChanged = mHorizontalOverflow != newHorizontalOverflow;
mHorizontalOverflow = newHorizontalOverflow;

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 193412 [details] [diff] [review]
fix

>+  if (vertChanged && horizChanged) {
>+    if (mVerticalOverflow == mHorizontalOverflow)
>+    {
>+      // both either overflowed or underflowed. 1 event
>+      PostScrollPortEvent(mVerticalOverflow, nsScrollPortEvent::both);
>+    } else {
>+      // one overflowed and one underflowed
>+      PostScrollPortEvent(mVerticalOverflow, nsScrollPortEvent::vertical);
>+      PostScrollPortEvent(mHorizontalOverflow, nsScrollPortEvent::horizontal);
>+    }
>+  } else if (vertChanged) { // only one changed either vert or horiz
>+     PostScrollPortEvent(mVerticalOverflow, nsScrollPortEvent::vertical);
>+  } else if (horizChanged) {
>+     PostScrollPortEvent(mHorizontalOverflow, nsScrollPortEvent::horizontal);
>   }

I tend to prefer

if (A) {
  if (B) {
    // 1
  } else {
    // 2
  }
} else {
  if (B) {
    // 3
  }
}

over

if (A && B) {
  // 1
} else if (A) {
  // 2
} else if (B) {
  // 3
}
but that's probably over-optimizing, so it's mainly which style you prefer. 
Either's fine with me.

(But I do think the simplification in my previous comment makes things quite a
bit clearer.)

Please comment that these are state used only by PostScrollEvents so it knows
whether the overflow state changes.
>+  PRPackedBool mHorizontalOverflow:1;
>+  PRPackedBool mVerticalOverflow:1;

r+sr=dbaron

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in with David's comments.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 193412 [details] [diff] [review]
fix

Fairly safe fix to make the branch compatible with 1.7 and trunk

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked into branch

Comment 14

[Mike Schroepfer]

Is this relevant to 1.0.x branch?

Comment 15

[Boris Zbarsky [:bzbarsky]]

> Is this relevant to 1.0.x branch?

No.