Closed Bug 305120 Opened 19 years ago Closed 19 years ago

onoverflow event handler not triggered in Deerpark alpha 2


(Core :: DOM: Events, defect)

Not set





(Reporter: colin, Assigned: roc)



(Keywords: fixed1.8, regression, testcase)


(2 files)

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

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.
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

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"
           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' 
Component: General → XP Toolkit/Widgets: XUL
Ever confirmed: true
Product: Firefox → Core
QA Contact: general → xptoolkit.xul
Version: unspecified → Trunk
This worked in 2005-04-28 build, but fails in 2005-04-29 build, I think a
regression from fixing bug 289940.
Assignee: nobody → events
Blocks: 289940
Component: XP Toolkit/Widgets: XUL → DOM: Events
Keywords: regression, testcase
QA Contact: xptoolkit.xul → ian
Regression in 1.0.5+ and Deer Park, nominating.

Flags: blocking1.8b4+
Flags: blocking1.7.12+
Flags: blocking-aviary1.0.7+
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
Blocks: 240276
No longer blocks: 289940
Gah. I guess I'll have to put that back.
Attached patch fixSplinter Review
Untested! I'm building but I just want to post this before I leave.
Comment on attachment 193412 [details] [diff] [review]

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
Attachment #193412 - Flags: superreview?(dbaron)
Attachment #193412 - Flags: review?(dbaron)
Whiteboard: [needs review+SR dbaron]
Comment on attachment 193412 [details] [diff] [review]

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 on attachment 193412 [details] [diff] [review]

>+  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


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;

Attachment #193412 - Flags: superreview?(dbaron)
Attachment #193412 - Flags: superreview+
Attachment #193412 - Flags: review?(dbaron)
Attachment #193412 - Flags: review+
Whiteboard: [needs review+SR dbaron]
checked in with David's comments.
Comment on attachment 193412 [details] [diff] [review]

Fairly safe fix to make the branch compatible with 1.7 and trunk
Attachment #193412 - Flags: approval1.8b4?
Closed: 19 years ago
Resolution: --- → FIXED
Attachment #193412 - Flags: approval1.8b4? → approval1.8b4+
checked into branch
Keywords: fixed1.8
Is this relevant to 1.0.x branch?
Flags: blocking1.7.13-
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8-
Flags: blocking-aviary1.0.8+
> Is this relevant to 1.0.x branch?

Depends on: 336574
You need to log in before you can comment on or make changes to this bug.