Closed Bug 346318 Opened 18 years ago Closed 17 years ago

Datepicker appears only briefly when clicking on date dropdown in event dialog

Categories

(Calendar :: General, defect)

Product:
Calendar
Component:
General
Platform:
PowerPC
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: mattwillis, Assigned: mattwillis)

References

Details

(Keywords: fixed1.8.1.4, regression)

Attachments

(2 files, 3 obsolete files)

doesn't work
(deleted), patch
Details | Diff | Splinter Review
rev2 - brute force hack
(deleted), patch
jaas
: first-review+
mark
: second-review+
dveditz
: approval1.8.1.4+
Details | Diff | Splinter Review 
Nominating for blocking0.3
Flags: blocking0.3?
Flags: blocking0.3? → blocking0.3+
Whiteboard: [swag: 1d]
Attached patch work in progress (obsolete) (deleted) — — Splinter Review 
The date/timepicker code alternatively makes me cry and tear my hair out.  This is a first step at getting things semi-sane in this area.  We listen for the event that we really care about, which is onpopuphiding.

(The actual bug is that .showPopup() doesn't work in reshowPopup, but I'd rather make our code here rational, and either (a) use that to find a minimal testcase for the regression range lilmatt gave, or (b) use that to remove whatever weird quirk is causing this bug.)

This patch makes everything about the datepicker work except for using the month/year dropdowns.
Assignee: nobody → jminta
Status: NEW → ASSIGNED
Attached patch real fix,let widgets finish (obsolete) (deleted) — — Splinter Review 
Turns out the reason that showPopup() didn't work was that the lower-level code still thought the popup was open.  Therefore, it ignored the call.  Moving things to a timeout, to let the current bits finish, ensures that the lower-level code will listen to us.
Attachment #231852 - Attachment is obsolete: true
Attachment #231856 - Flags: first-review?(dmose)
Comment on attachment 231856 [details] [diff] [review]
real fix,let widgets finish

See whether or not a bug is filed on the need for this workaround.  Include a comment with that bug number in the relevant bits here.

+              this.mReallyClose = false;
Add a comment here explaining what's going on.

r=dmose with those
Attachment #231856 - Flags: first-review?(dmose) → first-review+
Patch checked in.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Datepicker is broken in win32 Sunbird/0.3a2+ (2006080923) build now.

Open the datepicker, click on month or year dropdown and select a different month/year --> After that datepicker does not react on any mouse click anymore.
Comment on attachment 231856 [details] [diff] [review]
real fix,let widgets finish

I totally don't understand this patch. It seems to me that it does a lot of stuff that it should not do.

>-                         onpopupshowing="this.parentNode.kDatePicker.onPopup()">
>+                         anonid="datepopup"
>+                         onpopupshowing="onPopup()"
>+                         onpopuphiding="reshowPopup()">

Huh? reshowing the popup when it goes away?

>           this.kMinimonth.addEventListener("select", this.clickDate, false);
>-          this.kMinimonth.addEventListener("monthchange", this.reshowPopup, false);
>-          this.kMinimonth.addEventListener("popuplisthidden", this.reshowPopup, false);
>-          this.mIsReshowing = false;

Why remove those event listeners? They are there for a reason...

>       <method name="reshowPopup">
>         <parameter name="aEvent"/>
>         <body>
>           <![CDATA[
>-            var datepicker = aEvent.target.parentNode.parentNode.kDatePicker;
>-            datepicker.mIsReshowing = true;
>-            try { 
>-              aEvent.target.parentNode.hidePopup();
>-              aEvent.target.parentNode.showPopup();
>-            } finally {
>-              datepicker.mIsReshowing = false;

Why remove the trick to make the month and year popups not freeze?

>+            this.mIsReshowing = true;
>+            var popup = document.getAnonymousElementByAttribute(this, "anonid", "datepopup");
>+            // This must be in a timeout in order to give the popup time to 
>+            // finish closing.  Otherwise, our widget will think the popup is
>+            // still open, and hence it will ignore this call, and then close
>+            // a split-second later.
>+            setTimeout(popup.showPopup, 0);

Why will the widget think the popup is still open? Is that a xul issue, or something in this datepicker code? Can't you just fix that, instead of a workaround like this?
(In reply to comment #7)
> (From update of attachment 231856 [details] [diff] [review] [edit])
> I totally don't understand this patch. It seems to me that it does a lot of
> stuff that it should not do.
> 
> >-                         onpopupshowing="this.parentNode.kDatePicker.onPopup()">
> >+                         anonid="datepopup"
> >+                         onpopupshowing="onPopup()"
> >+                         onpopuphiding="reshowPopup()">
> 
> Huh? reshowing the popup when it goes away?
Yes, when the popup disappears (ie. from clicking the arrows) but we didn't want it to disappear, we should re-show it.
> 
> >           this.kMinimonth.addEventListener("select", this.clickDate, false);
> >-          this.kMinimonth.addEventListener("monthchange", this.reshowPopup, false);
> >-          this.kMinimonth.addEventListener("popuplisthidden", this.reshowPopup, false);
> >-          this.mIsReshowing = false;
> 
> Why remove those event listeners? They are there for a reason...
The monthchange and select events were covered by the onpopuphidden listener.  Removing the popuplisthidden here was likely wrong.
> 
> >       <method name="reshowPopup">
> >         <parameter name="aEvent"/>
> >         <body>
> >           <![CDATA[
> >-            var datepicker = aEvent.target.parentNode.parentNode.kDatePicker;
> >-            datepicker.mIsReshowing = true;
> >-            try { 
> >-              aEvent.target.parentNode.hidePopup();
> >-              aEvent.target.parentNode.showPopup();
> >-            } finally {
> >-              datepicker.mIsReshowing = false;
> 
> Why remove the trick to make the month and year popups not freeze?
Because the 'trick' was what broke Mac.  I don't like introducing workarounds to keep workarounds working.  We should fix the real bug.
> 
> >+            this.mIsReshowing = true;
> >+            var popup = document.getAnonymousElementByAttribute(this, "anonid", "datepopup");
> >+            // This must be in a timeout in order to give the popup time to 
> >+            // finish closing.  Otherwise, our widget will think the popup is
> >+            // still open, and hence it will ignore this call, and then close
> >+            // a split-second later.
> >+            setTimeout(popup.showPopup, 0);
> 
> Why will the widget think the popup is still open? Is that a xul issue, or
> something in this datepicker code? Can't you just fix that, instead of a
> workaround like this?
> 
It's a XUL/Layout issue, so that's what I can't fix it/answer this question.

> Because the 'trick' was what broke Mac.  I don't like introducing workarounds
> to keep workarounds working.  We should fix the real bug.

I'm lost. What exactly broke mac? Let's try to describe the code paths here.

On linux, the datepicker freezes after you select something from the month or year popup. That was fixed by closing and reshowing the datepicker after the year of month list went away.

What broke mac, and how was it fixed?
(In reply to comment #9)
> On linux, the datepicker freezes after you select something from the month or
> year popup. That was fixed by closing and reshowing the datepicker after the
> year of month list went away.
> 
> What broke mac, and how was it fixed?
> 
On Mac the reshowPopup function a) was entered too aggressively (called when it wasn't needed, even to fix the above freeze) and b) didn't result in an open popup.  Thus, the patch reduced the number of calls to that function (through different event listeners) and made sure that it actually does show the popup (through the setTimeout).

I can see why the patch makes the popup not close.

It removes the call to actually close the popup:
-              aEvent.target.parentNode.hidePopup();

And it removes the listener that should try to make the popup reshow:
-          this.kMinimonth.addEventListener("popuplisthidden", this.reshowPopup, false);

So, what happens when we add back that listener and add back the call to hidePopup, maybe in a setTimeout? Does that still work on mac?
Attached patch doesn't work (deleted) — — Splinter Review 
This, I believe, is the patch proposed in comment #11.  It does not fix Mac.  The datepicker simply closes whenever one tries to change months (even with the arrows, since reshowPopup is called then too.)
Now I remember: changing the month using the arrows _must_ reshow the popup, because the width changes (different month, different name, different width)

What if you set the timeout to something like 100msec? Sure, it will flicker, but it will be interesting to know.
(In reply to comment #13) 
> What if you set the timeout to something like 100msec? Sure, it will flicker,
> but it will be interesting to know.
> 
On its own, no matter how large I set the timeout, it never brings the popup back.  (This may be because the mIsReshowing variable has already become false?)  If however, I wrap the showPopup() call in another function, and set a timeout on that, I end up with an infinite loop of flicker immediately.  This happens whether or not I tweak the mIsReshowing variable.

1133         // Bug 345388: hiding windows using the async
1134         // TransitionWindowWithOptions causes memory corruption on 10.3, even

It seems that hiding the popup on mac is async, and takes a small bit of time. This mean that the popup.hide(); popup.show(); trick doesn't work as it should.
I think the solution would be to just not run that code on mac. It doesn't seem to be needed there anyway.
reopening, because fixing bug 348245 will break this again. We need to get this on the radar.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Mark, given the regression range here, do you have any thoughts on how to fix this  in Gecko instead of working around it?
Whiteboard: [swag: 1d] → [needs patch]
Assignee: jminta → nobody
Status: REOPENED → NEW
Whiteboard: [needs patch] → [needs patch][needs input from mento/josh]
After more investigation I suspect this checkin caused the regression on Mac...
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=mozilla&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-07-14+12%3A50%3A00&maxdate=2006-07-14+13%3A00%3A00&cvsroot=%2Fcvsroot

specifically the animated menu fade ins and outs in nsMacWindow.cpp

bug 348146 tracks the actual bug in toolkit.

In the meantime, we need a workaround for 0.3
Depends on: 348146
Assignee: nobody → lilmatt
Status: NEW → ASSIGNED

    
  
Whiteboard: [needs patch][needs input from mento/josh] → [nasty patch in hand][needs input dmose]

    
  
Whiteboard: [nasty patch in hand][needs input dmose] → [nasty patch in hand][needs input from mento/josh]

    
    

    
  
mento and/or josh:

We'd like to back the window-fade-out-on-Tiger code from nsMacWindow.cpp on trunk.
It is what is causing us all the trouble in this bug, and in bug 348146.

Since you're planning to move trunk to Cocoa widgets real soon, would you be okay with us backing this out? See attachment 239744 [details] [diff] [review] for what code we're talking about.

    
    

    
  
I am fine with backing it out.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        rev2 - brute force hack (deleted)
        — 
          — Splinter Review
      

    


  
Using ifndef to make this less lame

        Attachment #231856 -
        Attachment is obsolete: true

        Attachment #239744 -
        Attachment is obsolete: true

        Attachment #240022 -
        Flags: second-review?

        Attachment #240022 -
        Flags: first-review?(joshmoz)

    
  

        Attachment #240022 -
        Flags: second-review? → second-review?(mark)

    
  
Whiteboard: [nasty patch in hand][needs input from mento/josh] → [patch in hand][needs review joshaas mento]

    
    

    
  
Comment on attachment 240022 [details] [diff] [review]
rev2 - brute force hack

This is fine with me, but if this code causes problems for sunbird it is going to cause problems for others. Better to just knock it out totally, or leave it but "#if 0" it.

        Attachment #240022 -
        Flags: first-review?(joshmoz) → first-review+

    
  
Whiteboard: [patch in hand][needs review joshaas mento] → [patch in hand][needs review mento]

    
    

    
  
OK, wait, so the problem is that we're hiding a window and then showing the same window before the async hide has completed?

    
    

    
  
(In reply to comment #24)
> OK, wait, so the problem is that we're hiding a window and then showing the
> same window before the async hide has completed?

Yes.



    
    

    
  
Comment on attachment 240022 [details] [diff] [review]
rev2 - brute force hack

I talked to Matt, and given the time crunch for Sunbird's release, I'll allow this in spite of the no-app-specific-code-in-widget prohibition, under the condition that a comment referencing bug 348146 is included, and that we fix bug 348146 soon.

We do care about fixing this, because we have the same async hide on the 1.8 branch that we've got on the trunk, and we want to fix it there, even though we're moving the trunk to cocoa.  That's why I don't want to #if 0 this.

        Attachment #240022 -
        Flags: second-review?(mark) → second-review+

    
    

    
  
Comment on attachment 240022 [details] [diff] [review]
rev2 - brute force hack

Patch checked in (with comment) on trunk.

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 18 years ago → 18 years ago
Resolution: --- → FIXED

    
  
Whiteboard: [patch in hand][needs review mento]

    
    

    
  
Verified.
Sunbird 2006100207/windows
Lighning 2006100206/windows

    
  
Status: RESOLVED → VERIFIED

    
    

    
  
Now that we're releasing Sunbird 0.5 from branch, we're running into this bug again.

mento:
If guessing we can't just axe this stuff on MOZILLA_1_8_BRANCH like we did on trunk, right? ;)

blocking-calendar0.5+
Status: VERIFIED → REOPENED
Resolution: FIXED → ---

    
    

    
  
Now that we're releasing Sunbird 0.5 from branch, we're running into this bug again.

mento:
If guessing we can't just axe this stuff on MOZILLA_1_8_BRANCH like we did on trunk, right? ;)

blocking-calendar0.5+
Flags: blocking-calendar0.5+

    
    

    
  
I dunno, I don't see the problem with #ifdefs.  They did the trick for you on the trunk, right?

    
    

    
  
Comment on attachment 240022 [details] [diff] [review]
rev2 - brute force hack

mento:
Yes, #ifdefs worked on trunk, and in testing work on branch.  I just didn't know if we'd make an exception to allow an app-specific #ifdef on branch.

requesting a1813

        Attachment #240022 -
        Flags: approval1.8.1.3?

    
  
Whiteboard: [patch in hand] [needs a1813]

    
    

    
  
Comment on attachment 240022 [details] [diff] [review]
rev2 - brute force hack

requesting a1814

        Attachment #240022 -
        Flags: approval1.8.1.3? → approval1.8.1.4?

    
  
Whiteboard: [patch in hand] [needs a1813] → [patch in hand] [needs a1814]

    
    

    
  
Comment on attachment 240022 [details] [diff] [review]
rev2 - brute force hack

approved for 1.8.1.4, a=dveditz for release-drivers

        Attachment #240022 -
        Flags: approval1.8.1.4? → approval1.8.1.4+

    
    

    
  
Patch checked in on MOZILLA_1_8_BRANCH

-> FIXED
Status: REOPENED → RESOLVED
Closed: 18 years ago → 17 years ago
Resolution: --- → FIXED

    
  
Whiteboard: [patch in hand] [needs a1814]

    
  
Keywords: fixed1.8.1.4
Status: RESOLVED → VERIFIED
Flags: blocking-calendar0.5+

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: