Last Comment Bug 341047 - After "Close other tabs" tab's context menu contains scrollbars/arrows.
: After "Close other tabs" tab's context menu contains scrollbars/arrows.
Status: VERIFIED FIXED
: fixed-seamonkey1.0.5, fixed-seamonkey1.1b, fixed1.8.0.7, fixed1.8.1, regression
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: All All
: -- major (vote)
: mozilla1.8.1
Assigned To: Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
:
Mentors:
: 349334 349897 (view as bug list)
Depends on: 351938
Blocks: 336999
  Show dependency treegraph
 
Reported: 2006-06-09 22:19 PDT by Stephen Donner [:stephend] - PTO; back on 5/28
Modified: 2008-07-31 03:21 PDT (History)
19 users (show)
mbeltzner: blocking1.8.1-
dveditz: blocking1.8.0.7-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
ugly fix1 (3.58 KB, patch)
2006-08-15 03:44 PDT, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
no flags Details | Diff | Review
ugly fix2 (1.34 KB, patch)
2006-08-15 03:45 PDT, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
asaf: review-
Details | Diff | Review
backtrace from branch debug build (6.65 KB, text/plain)
2006-08-16 03:59 PDT, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
no flags Details
ugly fix2b (1.59 KB, patch)
2006-08-17 15:30 PDT, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
no flags Details | Diff | Review
ugly fix3 (2.71 KB, patch)
2006-08-17 15:48 PDT, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
asaf: review+
Details | Diff | Review
for check-in (checked in on trunk) (2.70 KB, patch)
2006-08-21 07:05 PDT, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
mconnor: approval1.8.1+
Details | Diff | Review
patch for 1.8 branch [Checkin: Comment 38] (2.70 KB, patch)
2006-08-24 16:16 PDT, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
no flags Details | Diff | Review
patch for 1.8.0.x branch [Checkin: Comment 52] (2.02 KB, patch)
2006-08-26 10:55 PDT, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
asaf: review+
dveditz: approval1.8.0.7+
Details | Diff | Review
(Gv1-XPFE) <scrollbox.xml> (2.51 KB, patch)
2006-08-27 20:17 PDT, Serge Gautherie (:sgautherie)
jag-mozilla: review+
neil: superreview+
Details | Diff | Review
(Gv2-XPFE) <scrollbox.xml> (1.88 KB, patch)
2006-09-02 03:17 PDT, Serge Gautherie (:sgautherie)
jag-mozilla: review+
jag-mozilla: superreview+
Details | Diff | Review
(Gv2b-XPFE) <scrollbox.xml> [Checkin: Comment 70 & 71] (1.86 KB, patch)
2006-09-06 14:01 PDT, Serge Gautherie (:sgautherie)
dveditz: approval1.8.0.7+
mbeltzner: approval1.8.1+
Details | Diff | Review

Description Stephen Donner [:stephend] - PTO; back on 5/28 2006-06-09 22:19:00 PDT
Build ID: 2006-06-09-18 of SeaMonkey trunk on Windows XP.

Summary: After "Close other tabs" tab's context menu contains scrollbars.

Steps to Reproduce:

1.  On Windows, do a CTRL-T until you fill up the screen horizontally.
2.  Right-click on a tab and choose "Close other tabs".
3.  Open a new tab, context-click it, and look at the context menu.

Expected Results:

Context menu _without_ vertical scrollbars appears.

Actual Results:

Context menu with scrollbars appears.
Comment 1 Frank Wein [:mcsmurf] 2006-06-10 03:32:33 PDT
This regressed between 2006-05-24-09 and 2006-05-25-09.
Comment 2 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-06-10 09:09:01 PDT
I took me a few tries.  Does the tab you don't close have to be not-the-first tab?
Comment 3 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-06-10 09:26:04 PDT
(In reply to comment #2)
> I took me a few tries.  Does the tab you don't close have to be not-the-first
> tab?
> 

Looks like you have to do it on a background tab.
Comment 4 Matthew Kairys 2006-08-11 10:13:06 PDT
Another way to reproduce this bug is:

1. Right-click a Live Bookmark and select 'Open All in Tabs'
2. Right-click on a tab and select 'Close Other Tabs'
3. Open a new tab, Right-click it and look at the menu.

Tested on Windows XP and Linux and bug is easily reproduced. If 'Always show the tab bar' is selected in preferences, this bug does not occur.
Comment 5 Tracy Walker [:tracy] 2006-08-11 10:16:32 PDT
I was also able to more reliably reproduce the bug if the Open and Close multiple tabs warnings were enabled.
Comment 6 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-08-15 03:38:40 PDT
This was caused by the fix for bug 336999. I can see this bug now also in Firefox1.5.0.6 (2006-8-14 build).

With a 1.8.1 branch build, I get this error in the js console when this happens:
Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIScrollBoxObject.getScrolledSize]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://global/content/bindings/scrollbox.xml :: _updateScrollButtonsDisabledState :: line 144"  data: no]
Comment 7 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-08-15 03:44:08 PDT
Created attachment 233762 [details] [diff] [review]
ugly fix1

This fixes it, by using try..catch, which is ugly, but I don't know of a way to fix this otherwise.
Comment 8 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-08-15 03:45:42 PDT
Created attachment 233763 [details] [diff] [review]
ugly fix2

Another ugly fix option.

Forgot to mention, I can reproduce this 100% when following comment 4 and comment 5.
Comment 9 Boris Zbarsky [:bz] 2006-08-15 08:26:59 PDT
So how come getScrolledSize is throwing?  At what point is this code being called, compared to nsCSSFrameConstructor::AttributeChanged?  For example, what's the callstack to getScrolledSize (on the C++ side, before we end up in the chrome JS)?
Comment 10 Mike Beltzner [:beltzner, not reading bugmail] 2006-08-15 10:44:40 PDT
Drivers don't feel that this is a serious enough issue (it's visual only, not a functionality regression) to warrant blocking the release. However, we'd take a well baked, low-risk patch.
Comment 11 Daniel Veditz [:dveditz] 2006-08-15 14:36:18 PDT
Ditto 1.8.1 drivers, especially as there's no owner on the bug. Please nominate the patch you want for 1.8.0 once it's landed on the trunk.
Comment 12 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-08-16 03:59:30 PDT
Created attachment 233984 [details]
backtrace from branch debug build

I added an assertion in my debug build when nsIFrame* scrolledBox = GetScrolledBox(this); returns null in nsScrollBoxObject::GetScrolledSize
This is the backtrace I get with the steps to reproduce.
Note that I get this assertion after I clicked on the warning dialog, which is odd, because the context menu has disappeared by long since then.
Comment 13 Boris Zbarsky [:bz] 2006-08-16 09:12:18 PDT
So basically we're calling _updateScrollButtonsDisabledState off an overflow or underflow event handler, right?  Which is handled right after reflow completes?

And sometime in there we destroyed the frame, after posting the event, right?  So the box object can't find the frame, etc...

Maybe the try/catch is indeed the right approach here.  Or canceling the DOM event when the frame is destroyed, perhaps?  It really doesn't make sense to fire overflow/underflow when the frame is dead.

Of course it's not clear to me why frame is dying somewhere in the middle of reflow... unless it's dying after we've dispatched the DOM event while the JS is running or something?  That can certainly happen on trunk, since nsBoxObject::GetFrame will flush frames.  In fact, that seems like the most likely scenario....

Perhaps HandlePostedDOMEvents should flush frames on that presshell before processing the events?  And we should have a way to "unpost" an event that scrollframes would use?
Comment 14 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-08-17 04:12:51 PDT
(In reply to comment #13)
> Maybe the try/catch is indeed the right approach here.  Or canceling the DOM
> event when the frame is destroyed, perhaps?  It really doesn't make sense to
> fire overflow/underflow when the frame is dead.

Should I ask review on the patch? I don't know a better way, but I would prefer if we didn't regress this bug on the 1.8.0.x branch also.

Comment 15 Boris Zbarsky [:bz] 2006-08-17 09:47:35 PDT
> Should I ask review on the patch? 

Yes.  Probably from the xpfe/toolkit owners.  I thought about it some more, and there is no way we can guarantee that the frame is still alive by the time the overflow/underflow handler is called (e.g. it could have been killed by a capturing handler higher up; in this case we can't prevent the child handler being called either, really).  So the UI code needs to deal with that eventuality somehow.
Comment 16 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-08-17 09:57:30 PDT
Comment on attachment 233763 [details] [diff] [review]
ugly fix2

Ok, asking review then.
Seamonkey doesn't need a fix because it doesn't seem to have code for disabled state for the scrollbox, currently.
Maybe a comment in the code why try..catch is needed here should be added?
Comment 17 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-08-17 10:23:17 PDT
Boris, could we somehow face the scenario you've described in comment 13 on underflow?
Comment 18 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-08-17 10:32:50 PDT
Comment on attachment 233763 [details] [diff] [review]
ugly fix2

See comment 17. Also:

>Index: toolkit/content/widgets/scrollbox.xml
>===================================================================

>       <handler event="overflow"><![CDATA[
>         // filter underflow events which were dispatched on nested scrollboxes
>         if (event.target != this)
>           return;
> 
>-        this._scrollButtonUp.collapsed = false;
>-        this._scrollButtonDown.collapsed = false;
>-        this._updateScrollButtonsDisabledState();
>+        try {
>+          this._scrollButtonUp.collapsed = false;
>+          this._scrollButtonDown.collapsed = false;

nit: put these outside of the try block.

>+          this._updateScrollButtonsDisabledState();
>+        } 
>+        catch(e) {
>+          this._scrollButtonUp.collapsed = true;
>+          this._scrollButtonDown.collapsed = true;

Please add a comment here explaining the reason for hiding the buttons (and also when would _updateScrollButtonsDisabledState throw).
Comment 19 Boris Zbarsky [:bz] 2006-08-17 13:27:04 PDT
I think we could hit this for underflow, yes.  How did we _use_ to handle this?  Before the fix for bug 336999 we probably didn't fire the underflow or overflow event at all.  What happened then?
Comment 20 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-08-17 15:30:04 PDT
Created attachment 234308 [details] [diff] [review]
ugly fix2b

Like this?
Comment 21 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-08-17 15:31:51 PDT
Martijn, see comment 19.
Comment 22 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-08-17 15:34:56 PDT
(In reply to comment #21)
> Martijn, see comment 19.

Yes, I know, but I don't know of a practical situation in which a user is able to hit this and sees scrollbars in his scrollbox afterwards.
Anyway, if you want try..catch blocks around that code too, I can do that.
Comment 23 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-08-17 15:48:13 PDT
Created attachment 234311 [details] [diff] [review]
ugly fix3

So like this. I haven't tested this, but I don't know of a case where underflow is fired, while this.ensureElementIsVisible is throwing.
I could create such a testcase, I guess, but that would be an artificial situation.
Comment 24 Stephen Donner [:stephend] - PTO; back on 5/28 2006-08-19 22:22:28 PDT
*** Bug 349334 has been marked as a duplicate of this bug. ***
Comment 25 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-08-19 22:37:44 PDT
Comment on attachment 234311 [details] [diff] [review]
ugly fix3

>Index: toolkit/content/widgets/scrollbox.xml
>===================================================================

>-        var childNodes = document.getAnonymousNodes(this._scrollbox);
>-        if (childNodes && childNodes.length) {
>-          this.ensureElementIsVisible(childNodes[0]);
>-          if (childNodes.length > 1)
>-            this.ensureElementIsVisible(childNodes[childNodes.length-1]);
>+        try {
>+          // See bug 341047 and see comments in overflow handler of why try..catch is needed here

nit: s/of why/as to why/. Also, 80 chaacterrs per line.

>+          var childNodes = document.getAnonymousNodes(this._scrollbox);
>+          if (childNodes && childNodes.length) {
>+            this.ensureElementIsVisible(childNodes[0]);
>+            if (childNodes.length > 1)
>+              this.ensureElementIsVisible(childNodes[childNodes.length-1]);

while you're here, please change this to

if (childNodes.length > 0)
  this.ensureElementIsVisible(childNodes[0]);

the last-child thing isn't need here.


...
>+        try {
>+          // See bug 341047, the overflow event is firing when the scrollbox already is mostly destroyed.
>+          // this causes some code in _updateScrollButtonsDisabledState() to throw an error.
>+          // It also means that the scrollbarbuttons were uncollapsed when that should not be happening,
>+          // because the whole overflow event should not be happening in that case.

"dispatched". Again, 80 characters per line please.


r=mano otherwise.
Comment 26 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-08-19 22:58:24 PDT
This is also a problem in Seamonkey it seems, see bug 349334. It doesn't have the _updateScrollButtonsDisabledState code which throws the exception, but the overflow event is still fired when the popup is destroyed which causes the scrollbars.
Comment 27 Stephen Donner [:stephend] - PTO; back on 5/28 2006-08-19 23:31:57 PDT
(In reply to comment #26)
> This is also a problem in Seamonkey it seems, see bug 349334. It doesn't have
> the _updateScrollButtonsDisabledState code which throws the exception, but the
> overflow event is still fired when the popup is destroyed which causes the
> scrollbars.

Er, also?  SeaMonkey was noted in comment 0 ;-)
Comment 28 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-08-21 04:39:31 PDT
Comment on attachment 234311 [details] [diff] [review]
ugly fix3

Matijn, please land this on trunk asap, thanks.
Comment 29 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-08-21 07:05:53 PDT
Created attachment 234775 [details] [diff] [review]
for check-in (checked in on trunk)

(In reply to comment #27)
> Er, also?  SeaMonkey was noted in comment 0 ;-)

Oops! I probably should read the bug comments ;)
So Seamonkey needs a similar (or even worse) hack.
Comment 30 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-08-21 07:18:17 PDT
Checking in scrollbox.xml;
/cvsroot/mozilla/toolkit/content/widgets/scrollbox.xml,v  <--  scrollbox.xml
new revision: 1.20; previous revision: 1.19
done

Checked in on trunk. Leaving bug open for Seamonkey.
Comment 31 kitchin 2006-08-21 08:48:15 PDT
The checkin on trunk seems to have unbalanced {}'s.
Comment 32 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-08-21 09:13:40 PDT
(In reply to comment #31)
> The checkin on trunk seems to have unbalanced {}'s.

Sorry, it is caused here:
+          if (childNodes && childNodes.length) {
+            if (childNodes.length > 0)
+              this.ensureElementIsVisible(childNodes[0]);
+        }
It needs an extra } before the final }

I'm afraid I can fix this only after 5/6 hours, sorry about that.
Comment 33 Mike Beltzner [:beltzner, not reading bugmail] 2006-08-21 10:14:29 PDT
Driver's decision is the same as in comment 10; once the patch is on trunk and well baked, please nominate for approval1.8.1?
Comment 34 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-08-21 15:27:38 PDT
Fix checked in for the stupid error mentioned in comment 31.
Comment 35 Mike Connor [:mconnor] 2006-08-24 10:55:29 PDT
Comment on attachment 234775 [details] [diff] [review]
for check-in (checked in on trunk)

a=mconnor on behalf of drivers for checkin to 1.8 branch.
Comment 36 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-08-24 15:37:12 PDT
*** Bug 349897 has been marked as a duplicate of this bug. ***
Comment 37 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-08-24 16:16:05 PDT
Created attachment 235319 [details] [diff] [review]
patch for 1.8 branch
[Checkin: Comment 38]

This is the 1.8.1 branch patch, which I'll check in.
Comment 38 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-08-24 16:24:32 PDT
Checked in on the 1.8.1 branch. Is the patch something that should be worthy of the 1.5.0.8 branch?
Comment 39 Boris Zbarsky [:bz] 2006-08-24 16:38:48 PDT
I think so, yes.
Comment 40 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-08-24 16:54:17 PDT
Comment on attachment 235319 [details] [diff] [review]
patch for 1.8 branch
[Checkin: Comment 38]

It's really a shame that the patch for bug 336999, that's causing this bug is on the 1.8.0.7 branch.
Now, this will be broken on the 1.8.0.7 branch and could possibly be fixed again on the 1.8.0.8 branch, if this patch is approved.
Comment 41 Boris Zbarsky [:bz] 2006-08-24 17:12:30 PDT
I think you should request approval for 1.8.0.7... is there a reason not to?
Comment 42 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-08-24 17:18:13 PDT
Comment on attachment 235319 [details] [diff] [review]
patch for 1.8 branch
[Checkin: Comment 38]

I thought the 1.8.0.7 tree was already closed, but I can always try, I guess.
Comment 43 Daniel Veditz [:dveditz] 2006-08-25 11:30:15 PDT
Comment on attachment 235319 [details] [diff] [review]
patch for 1.8 branch
[Checkin: Comment 38]

approved for 1.8.0 branch, a=dveditz for drivers

Code freeze is today (+weekend if necessary) for a candidate build Monday. Please check in ASAP.
Comment 44 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-08-25 12:59:40 PDT
Oh, stuff on the 1.8.0.7 branch is more or less the same as Seamonkey code, so it needs a different patch. Not sure yet how to fix that in the best way.
Comment 45 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-08-26 10:55:09 PDT
Created attachment 235577 [details] [diff] [review]
patch for 1.8.0.x branch
[Checkin: Comment 52]

So something like this?
boxObject.width returns 0 when this is happening, so a check for that, prevents this from happening.
Comment 46 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-08-27 03:32:32 PDT
Comment on attachment 235577 [details] [diff] [review]
patch for 1.8.0.x branch
[Checkin: Comment 52]

r=mano
Comment 47 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-08-27 04:08:34 PDT
Comment on attachment 235577 [details] [diff] [review]
patch for 1.8.0.x branch
[Checkin: Comment 52]

This needs approval1.8.0.7 because this is a different patch.
Comment 48 Serge Gautherie (:sgautherie) 2006-08-27 20:17:30 PDT
Created attachment 235695 [details] [diff] [review]
(Gv1-XPFE) <scrollbox.xml>

Ports TK-180 patch to SeaMonkey.


[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1b2) Gecko/20060827 SeaMonkey/1.1b] (nightly) (W98SE)

In my local install,
I added an |else { alert(...); }| to each "function" to check whether this case was happening,
and, shortly after, I saw my first case in the |overflow| function:
immediately "after" closing a tab.
Then, it seems this fix is appropriate for SeaMonkey too :-)
Comment 49 jag (Peter Annema) 2006-08-27 20:40:23 PDT
Comment on attachment 235695 [details] [diff] [review]
(Gv1-XPFE) <scrollbox.xml>

+        // In that code, the below code should not be executed.

I wonder if "In that case, the ..." was intended.
Comment 50 Daniel Veditz [:dveditz] 2006-08-27 23:01:14 PDT
Comment on attachment 235577 [details] [diff] [review]
patch for 1.8.0.x branch
[Checkin: Comment 52]

a=dveditz on this 1.8.0 version, then, but may not make it (about 12 hours to go).
Comment 51 Serge Gautherie (:sgautherie) 2006-08-27 23:28:11 PDT
(In reply to comment #49)
> (From update of attachment 235695 [details] [diff] [review] [edit])
> +        // In that code, the below code should not be executed.
> 
> I wonder if "In that case, the ..." was intended.

I did too, but I decided to simply copy it as it was.
Martijn, what do you think ?
Comment 52 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-08-28 02:37:15 PDT
(In reply to comment #51)
> > I wonder if "In that case, the ..." was intended.
> 
> I did too, but I decided to simply copy it as it was.
> Martijn, what do you think ?

Argh! No, that should be: "In that case". I've fixed that comment before I checked in the "patch for 1.8.0.x branch" patch in the 1.8.0.7 tree.
Sorry for not providing a Seamonkey patch, I was more or less busy with getting the Firefox patches in. I was planning on doing that afterwards.
Comment 53 neil@parkwaycc.co.uk 2006-08-28 06:00:57 PDT
Comment on attachment 235695 [details] [diff] [review]
(Gv1-XPFE) <scrollbox.xml>

>+        // See bug 341047 and comments in overflow handler why this check is
>+        // needed.
Nit: See bug 341047 and comments in overflow handler as to why this check is needed.

>+        // See bug 341047, the overflow event is dispatched when the scrollbox
>+        // already is mostly destroyed, that should not be happening.
Nit: See bug 341047, the overflow event is dispatched when the scrollbox is already mostly destroyed, which should not be happening.

>+        // That is what this boxObject check is doing.
Nit: That is what this boxObject check does.
Comment 54 neil@parkwaycc.co.uk 2006-08-28 06:02:05 PDT
Comment on attachment 235695 [details] [diff] [review]
(Gv1-XPFE) <scrollbox.xml>

Alternative wording:
         // XXX Workaround for unexpected events dispatched
         // during scrollbox destruction (bug 341047).
Comment 55 Serge Gautherie (:sgautherie) 2006-09-02 03:17:28 PDT
Created attachment 236524 [details] [diff] [review]
(Gv2-XPFE) <scrollbox.xml>

Gv1-XPFE, with comment 54 suggestion,
and a reversed test.

'approval1.8.1=?': (XPFE only)
Simple UI regression fix, no risk. (see comment 40)

'approval1.8.0.7=?' / 'approval1.8.0.8=?': (XPFE only)
I believe it's too late for 1.8.0.7, but may be not for a "SeaMonkey" only patch !?
Comment 56 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-09-02 03:29:57 PDT
Comment on attachment 236524 [details] [diff] [review]
(Gv2-XPFE) <scrollbox.xml>

a1.8.0.x are Fx/Tb only. There's an a1.7.14 flag (i'm not sure whether or not it's still used.)
Comment 57 Serge Gautherie (:sgautherie) 2006-09-02 04:24:39 PDT
(In reply to comment #56)
> (From update of attachment 236524 [details] [diff] [review] [edit])
> a1.8.0.x are Fx/Tb only. There's an a1.7.14 flag (i'm not sure whether or not
> it's still used.)

Then, there must be missing flags,
as a1.7.14 is for ("retired") Mozilla Application Suite.

What are the flags to use for Core bugs (like this one) ?

NB: There must also be some news that I missed, as I used to do like that, and it went well, until now...
Comment 58 Robert Kaiser (not working on stability any more) 2006-09-02 05:09:52 PDT
(In reply to comment #56)
> (From update of attachment 236524 [details] [diff] [review] [edit])
> a1.8.0.x are Fx/Tb only.

Bullshit. Sorry for the strong language, but you should get informed before posting nonsense.

a1.8.[0.]x are applicable for all core changes (or better, all drivers-governed parts of mozilla.org code).
For SeaMonkey-spcific patches in relevant products on bugzilla, there are appropriate seamonkey-council-governed approval-seamonkey* flags, but core code is governed by drivers and therefore a1.8.* flags are correct to use.
Comment 59 Sander 2006-09-02 05:11:33 PDT
Comment on attachment 236524 [details] [diff] [review]
(Gv2-XPFE) <scrollbox.xml>

Resetting approval? flags
Comment 60 neil@parkwaycc.co.uk 2006-09-02 14:57:25 PDT
Comment on attachment 236524 [details] [diff] [review]
(Gv2-XPFE) <scrollbox.xml>

Not my preferred style, but jag can r+sr if he likes it
Comment 61 jag (Peter Annema) 2006-09-03 00:27:01 PDT
Comment on attachment 236524 [details] [diff] [review]
(Gv2-XPFE) <scrollbox.xml>

If these events are unexpected, does that mean there's a bug somewhere on making them not fire?

If so, then I like this patch as it'll make it easier to remove these lines when that bug gets fixed, and it makes it clear this code isn't part of the main purpose of that handler.

If on the other hand those events aren't unexpected I like the previous patch better.
Comment 62 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-09-03 07:02:57 PDT
(In reply to comment #61)
> If these events are unexpected, does that mean there's a bug somewhere on
> making them not fire?

These events are unexpected, I haven't filed a bug about it yet. I was trying to get a testcase that reproduces the bug, but failed.
Comment 63 Mike Schroepfer 2006-09-05 11:37:32 PDT
Comment on attachment 236524 [details] [diff] [review]
(Gv2-XPFE) <scrollbox.xml>

clearing the nom flag on the 1.8.1 branch.  Please make sure you have all reviews and patches have baked on trunk before asking for approval.
Comment 64 jag (Peter Annema) 2006-09-06 12:09:00 PDT
Comment on attachment 236524 [details] [diff] [review]
(Gv2-XPFE) <scrollbox.xml>

r+sr if you drop the {}s.
Comment 65 Serge Gautherie (:sgautherie) 2006-09-06 14:01:26 PDT
Created attachment 236999 [details] [diff] [review]
(Gv2b-XPFE) <scrollbox.xml>
[Checkin: Comment 70 & 71]

Gv2, with comment 64 suggestion(s).

For checkin.
Comment 66 Daniel Veditz [:dveditz] 2006-09-06 16:30:13 PDT
Comment on attachment 236999 [details] [diff] [review]
(Gv2b-XPFE) <scrollbox.xml>
[Checkin: Comment 70 & 71]

seamonkey-only, approved for the 1.8.0 branch in consultation with the seamonkey-council, a=dveditz

Do you need this on the 1.8 branch also?
Comment 67 Serge Gautherie (:sgautherie) 2006-09-06 19:23:06 PDT
Comment on attachment 236999 [details] [diff] [review]
(Gv2b-XPFE) <scrollbox.xml>
[Checkin: Comment 70 & 71]

Yes, we do.
Comment 68 Mike Beltzner [:beltzner, not reading bugmail] 2006-09-07 10:26:24 PDT
Comment on attachment 236999 [details] [diff] [review]
(Gv2b-XPFE) <scrollbox.xml>
[Checkin: Comment 70 & 71]

a=beltzner on behalf of 181drivers
Comment 69 Robert Kaiser (not working on stability any more) 2006-09-07 16:03:11 PDT
If the xpfe patch gets landed until about tomorrow noon PDT, we can ship it in SeaMonkey 1.0.5, as we need to respin builds for that release anyways.
Comment 70 Andrew Schultz 2006-09-07 22:47:09 PDT
Comment on attachment 236999 [details] [diff] [review]
(Gv2b-XPFE) <scrollbox.xml>
[Checkin: Comment 70 & 71]

I landed this on the 1.8.0 branch although I had to merge it (branch still has preventBubble)
Comment 71 Andrew Schultz 2006-09-08 22:50:36 PDT
Comment on attachment 236999 [details] [diff] [review]
(Gv2b-XPFE) <scrollbox.xml>
[Checkin: Comment 70 & 71]

I landed this on trunk and 1.8 branch
Comment 72 Serge Gautherie (:sgautherie) 2006-09-09 05:44:08 PDT
*PC/WXP -> All/All, per bug 349897.

(In reply to comment #62)
> These events are unexpected, I haven't filed a bug about it yet. I was trying
> to get a testcase that reproduces the bug, but failed.

I believe you can file that bug yet:
even if we don't have a testcase, we know the bug is there.
Comment 73 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-09-09 05:56:06 PDT
(In reply to comment #72)
> I believe you can file that bug yet:
> even if we don't have a testcase, we know the bug is there.

Ok, I filed bug 351938 for it.

Comment 74 Stephen Donner [:stephend] - PTO; back on 5/28 2006-09-10 14:30:12 PDT
Verified FIXED using SeaMonkey trunk build 2006-09-10-08 under Windows XP; I was no longer able to reproduce this.  If anybody sees this again on trunk or branch, please do reopen this bug.

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