Closed Bug 375390 Opened 18 years ago Closed 17 years ago

New Event Boxes appear too large in month/multiweek view

Categories

(Calendar :: Calendar Frontend, defect)

Product:
Calendar
Component:
Calendar Frontend
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: Fallen, Assigned: michael.buettner)

References

Details

(Keywords: regression)

Attachments

(2 files)

Screenshot
24.02 KB, image/png
Details
remove setting height
4.87 KB, patch
michael.buettner
: review+
Details | Diff | Splinter Review
Attached image Screenshot — 
The event boxes look fine on branch, but on trunk the event boxes are ~60px each in the month views.
Confirmed with own trunk build on Windows.
Status: UNCONFIRMED → NEW
Ever confirmed: true
According to bug 368982 comment 48 this also happens in multiweek view. Build ID: 
Mozilla/5.0 (Windows NT 5.1; en-US; rv:1.9a4pre) Gecko/20070410 Calendar/0.6a1
Severity: normal → major
Summary: New Event Boxes appear too large in month view on Trunk → New Event Boxes appear too large in month/multiweek view on Trunk
I can confirm  https://bugzilla.mozilla.org/show_bug.cgi?id=375390#c3 -- it also happens in multiweek view at least on WinXP.

Build ID:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a4pre) Gecko/20070411 Calendar/0.6a1

This happens for me when the view is changed, on first load they look great, but if I switch views or create a new event, anything that's basically not drawn on startup is too big.
First investigations results using DOMi: The height of the xul:image element containing the gradient image is set to 72px in the month view. For all day events in the week view the height is set to 68px. The latter matches the height of gradient-overlay.png (64px) + 2px margin-top + 2px margin-bottom set by the event-frame css rule. If I disable the event-frame css rule the height is set to 64px in both cases matching the height of gradient-overlay.png.

Removing the gradient image by using the following userChrome.css rule fixes the issue and can be used as a workaround:

    .calendar-event-box-gradient { 
        display: none !important; 
    }
I now see this issue in a non-Trunk build too. Using Thunderbird 2.0.0.7pre (20071001) + Lightning 0.7 (2007100203) on Windows 2000.

Requesting blocking-calendar0.7 to investigate this issue before release.
Flags: blocking-calendar0.7?
Seems to be caused by a recent MOZILLA_1_8_BRANCH checkin. Regression range:

WORKS in Thunderbird 2.0.0.7pre (2007093003) + Lightning 0.7 (2007100203)
FAILS in Thunderbird 2.0.0.7pre (2007100104) + Lightning 0.7 (2007100203)

Checkins during regression range: http://tinyurl.com/yuefen

Probably caused by the checkin "Fire XBL constructors asynchronously after binding attachment, unless we're in the middle of an update. In that case, fire them at the end of the update. Bugs 267833, 373756, 394676, 394014."

The error is not visible in Sunbird 0.7 because we created the SUNBIRD_0_7_BRANCH before the checkin from above.
I can confirm this with the same Lightning nightly and yesterday's TB nightly (20071001).

This definitely blocks 0.7
Flags: blocking-calendar0.7? → blocking-calendar0.7+
God, I wish someone had brought this to my attention in March.

Ok.  So now we're in firedrill mode for fixing this.  Can someone give me an approximate idea of what's going wrong?  Are people expecting a constructor to have run when it hasn't yet?  Or something else?

I can try debugging this later this week, but I'll need detailed instructions to reproduce (on trunk, preferably), starting with what to build and ending with the exact UI steps to perform (like where to click, etc).  Assume you're giving directions to someone who has never built or used calendar before (which is the case, so it's a good assumption).  I do have Thunderbird builds.
Blocks: 267833
(In reply to Bug 267833 Comment #71)
> I don't see bug 375390 in the dep list for bug 267833, nor was I cced on it.
> I can only assume no one took the trouble to hunt down the regression 
> range on trunk?

It was broken since the first day on Trunk since the calendar changes for the new event boxes landed. Therefore there was no indication that this could be a regression.
No longer blocks: 267833
Blocks: 267833
Keywords: regression
Steps To Reproduce:
1. Download Sunbird from <http://ftp.mozilla.org/pub/mozilla.org/calendar/sunbird/nightly/latest-trunk/> or 
build Sunbird using "mk_add_options MOZ_CO_PROJECT=calendar" and "ac_add_options --enable-application=calendar"
2. Create new profile
3. Switch to Month view via menu View -> Month View
4. Create new dummy event via menu File -> New Event and save it
5. Force a view refresh e.g. by selecting menu Go -> Today

Actual Results:
After step 4 the event box is displayed normal in the Month View.
After step 5 the event box is displayed too large in the Month View.
OK.  I'll try that, probably on Friday...  In the meantime, if someone familiar with this code has any insight into the questions in comment 15, or can produce a somewhat smaller testcase that still shows the problem, that would be much appreciated.
Summary: New Event Boxes appear too large in month/multiweek view on Trunk → New Event Boxes appear too large in month/multiweek view
Boris, the relevant code is in
http://lxr.mozilla.org/mozilla/source/calendar/base/content/calendar-month-view.xml
(large file, I know)

You'll probably mostly be interested in the 'refresh' function.

I'll also ask mickey (michael.buettner@sun.com) and daniel (daniel.boelzle@sun.com), the developers most familiar with this code, to answer your questions in comment 15.
more accurately, the interesting code is here:
http://mxr.mozilla.org/mozilla1.8/source/calendar/base/content/calendar-month-view.xml#99

The problem as I see it is that the constructor wants to know the height on one if it's children, and then adjust the height of another child.
To add more fun, another constructor wants to do exactly the same at the same time. (don't ask me why)
http://mxr.mozilla.org/mozilla1.8/source/calendar/base/content/calendar-view-core.xml#86

And for even more fun: it seems to not be needed at all! When i remove the code that sets the height attribute, but set height="1px" on the relevant element in the <content/> part itself, I still get a nice gradient. And at the same time, the bug seems to be gone. The box has the correct size. The gradient also still works in day view (which only uses the gradient defined in the -core file, afaik)

The change I was talking about. The question is: is this just a workaround, or was our original code just buggy? And even is this is a workaround, do we care?
(only tested on linux, and didn't change system font size or anything, so this needs a bit more testing)
So was the change that container.boxObject.height started returning an incorrect value?  Or that it started returning a correct value?
20070930 Thunderbird 2.0.0.7pre build:
    After step 4 (creating the event):
        calendar-view-core.xml  container.boxObject.height=0
        calendar-month-view.xml container.boxObject.height=0
    After step 5 (refresh):
        calendar-view-core.xml  container.boxObject.height=0
        calendar-month-view.xml container.boxObject.height=0

20071001 Thunderbird 2.0.0.7pre build:
    After step 4 (creating the event):
        calendar-view-core.xml  container.boxObject.height=4
        calendar-month-view.xml container.boxObject.height=8
    After step 5 (refresh):
        calendar-view-core.xml  container.boxObject.height=68
        calendar-month-view.xml container.boxObject.height=72

After the checkin container.boxObject.height started returning a value. But somehow the value is changed from one pass to the next pass. The values itself might be correct, see Comment #10.
OK, that makes sense.

If I read the code correctly, prior to bug 267833 being fixed, constructors sometimes (but not always!) ran at a time when reflow was not allowed to happen.  So asking for layout-dependent things (like boxObject.height) would return bogus values if the frame targeted had pending restyles or reflows against it.  In particular, in this case it would always return 0 because the frame hadn't been reflowed yet.

The patch to bug 267833 moved constructor firing to a safe time, so attempts to flush our layout during a constructor now succeed and you get back up-to-date layout information.

I could try to restore the old behavior (not allowing reflow while processing binding constructors) on branch.  I don't think we should be doing that on trunk.
OK.  So on the second pass the layout is definitely different.  Is there a way to get Calendar and DOM Inspector in the same app?
bz, I've put up a slightly hacked (had to adjust the maxversion in install.rdf) DOMI build for you on http://www.babylonsounds.com/calendar/dom_inspector-1.8.1.2.1-tb-sb.xpi

I hope it works for you.
I don't seem to have a way of selecting the Calendar window there....
You can select the Calendar window, but DOMI will not always show that it has been selected. Just inspect chrome://calendar/content/calendar.xul and then go to:

Window -> vbox -> hbox -> 2. vbox -> deck -> calendar-decorated-month-view and you're basically there.

You will have to dig a little bit deeper though, since our views do some crazy stuff with XUL (as I've been told).
This is completely off the radar for the 1.8.1.8 triage team. Could we move this bug to a product that has appropriate flags, or if a core change is required file a back-end bug on the specific core issue?
> You can select the Calendar window

There are no windows in the Inspect Window list....
My apologies for stepping up that late in the game, but I was offline during the last couple of days. I originally wrote this particular piece of code.

Just to explain what's the initial intend was, I tried to display a gradient inside of the event boxes. The problem is that the height of the box enclosing the gradient image should be defined by the content of the box *excluding* the image, i.e. the height should be determined by the label and all the rest of the content, the image should be scaled to fit inside the resulting frame (as the image is used in several other places as well where the height is variable). I'm using an image in order to being able to set an arbitrary background color while displaying a gradient on top of that.

The idea was to hide the image to avoid that it influences the layout. After the initial layout has been run, I tried to ask what's the resulting height of the enclosing box. This obviously doesn't work if it's unsure when the constructor will be called. If it's equally valid to set the height to '1px' and just show the image after the initial layout has been run, it should be perfectly valid, even it that's a workaround. I just would like to understand *why* it works. If I'm setting the height to '1px' why will the image be blown to full height later? Just to repeat myself, if this patch fixes the problem, I would just take it. If somebody could explain *why* it works, it would make my day.
(In reply to comment #24)
> I could try to restore the old behavior (not allowing reflow 
> while processing binding constructors) on branch.  I don't think
> we should be doing that on trunk.

Personally I would prefer to have the same behavior on trunk and branch. Judging from the comments here and the latest comments in Bug 267833 there were no other regressions reported. This makes me think that this is not a Core bug but something that should be fixed on calendar side.


(In reply to comment #14)
> I can confirm this with the same Lightning nightly and yesterday's TB nightly
> (20071001).
> 
> This definitely blocks 0.7
> 

Simon, why should this block 0.7? As Stefan Sitter pointed out (and I've verified that), SUNBIRD_0_7_BRANCH isn't affected by the recent branch change.

(In reply to comment #33)
But all Lightning 0.7 users will be affected if they upgrade to Thunderbird 2.0.0.7 once it's released.
(In reply to comment #32)
> (In reply to comment #24)
> > I could try to restore the old behavior (not allowing reflow 
> > while processing binding constructors) on branch.  I don't think
> > we should be doing that on trunk.
> 
> Personally I would prefer to have the same behavior on trunk and branch.
> Judging from the comments here and the latest comments in Bug 267833 there were
> no other regressions reported. This makes me think that this is not a Core bug
> but something that should be fixed on calendar side.

Unintended change in behaviour on branch is a bug, because it'll break other things as well.  We have to fix it or bump the 3rd digit.
> Simon, why should this block 0.7? As Stefan Sitter pointed out (and I've
> verified that), SUNBIRD_0_7_BRANCH isn't affected by the recent branch change.

Because this will affect our users once they upgrade to TB 2.0.0.7 or have upgraded to recent TB 2 nightlies.
Version: Trunk → unspecified
Depends on: 398404
> Unintended change in behaviour on branch is a bug

Precisely.  I've filed bug 267833 to track this issue.
Probably not 267833?
Er, bug 398404
(In reply to comment #31)
> The idea was to hide the image to avoid that it influences the layout. After
> the initial layout has been run, I tried to ask what's the resulting height of
> the enclosing box.

The relevant code being:

 108           gradient.removeAttribute("hidden");
 109           gradient.setAttribute("style",
 110                                 "height: " +
 111                                 container.boxObject.height+"px");

If this runs at a time when reflow and style flushes are allowed, the attempt to get .height will process the restyle from removeAttribute("hidden"), then will reflow.  At which point, the image will have its intrinsic size, since it doesn't have a height style.  The intrinsic size of this particular image (chrome://calendar/skin/gradient-overlay.png) is 64px by 64px.  Since the image is in a stack, the stack will grow to fit it, and will resize any kids that are smaller than this size to be the same size as the stack (see bug 281426).  Then you ask for the height of one of these other kids, and get the behavior observed here.

So just changing the order of setting the height and unhiding the image should work too.

> I just would like to understand *why* it works. If I'm setting the height to
> '1px' why will the image be blown to full height later?

See above about how stacks behave.
Ok, with the checkin of the fix for bug 398404 this *should* now be a trunk-only issue again.

Question: 
Do we want to take mvl's fix on the trunk (and the branch - to keep both in sync) or do you have something else in mind, mickey?

BTW, thanks Boris for helping us out here so quickly!
Comment on attachment 283215 [details] [diff] [review]
remove setting height

Thanks Boris for explaining why the relevant code actually fails. Furthermore, it clarifies why mvl's fix is not a workaround but the correct way to resolve the initial problem. It gives the image a height style in order to avoid that the image gets its intrinsic size. The stack will resize the image during reflow to match the size of the other children, which is exactly what I tried to achieve in the first place. That's also why it works for any height setting that is reasonably smaller than the default height of the event boxes, so 1px or 0px work just fine. I'm kicking myself for having caused all this trouble and not having found the solution by myself...

I'd like to take this patch for trunk, MOZILLA_1_8_BRANCH and SUNBIRD_0_7_BRANCH. Daniel, do you agree with that?
Attachment #283215 - Flags: review+
Assignee: nobody → michael.buettner
+1; I'm fine with that.
(In reply to comment #35)
> (In reply to comment #32)
> > (In reply to comment #24)
> > > I could try to restore the old behavior (not allowing reflow 
> > > while processing binding constructors) on branch.  I don't think
> > > we should be doing that on trunk.
> > 
> > Personally I would prefer to have the same behavior on trunk and branch.
> > Judging from the comments here and the latest comments in Bug 267833 there were
> > no other regressions reported. This makes me think that this is not a Core bug
> > but something that should be fixed on calendar side.
> 
> Unintended change in behaviour on branch is a bug, because it'll break other
> things as well.  We have to fix it or bump the 3rd digit.

Honestly, I wouldn't even expect such a change in 2.x.y.z. releases, but in TB 3 only. However, thanks to all resolving that issue!

patch checked in on trunk, MOZILLA_1_8_BRANCH and SUNBIRD_0_7_BRANCH

-> FIXED
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment on attachment 283215 [details] [diff] [review]
remove setting height

>+                         hidden="true" height="1px"/>
Nit: the height attribute is an integer, always in pixel units.

If you're overriding the preferred height of the image so that it doesn't stretch the stack, is there still any point hiding it?
(In reply to comment #46)
> If you're overriding the preferred height of the image so that it doesn't
> stretch the stack, is there still any point hiding it?
The idea is to make the image fill the area of the stack, but the intrinsic size of the image should not be taken into account (as the image is almost always larger than the resulting size of the stack). That's why the image is initially hidden while also overriding the intrinsic size with the height attribute. 

Later, after the initial layout has been run, the image will be shown. At this stage the image gets stretched over the whole area of the stack (which is the intended behavior in this case). As Boris pointed out all children get the size of the stack, which seems to override the height attribute in this case. 

Interestingly enough, this doesn't work when not hiding the image element of the stack initially.
Target Milestone: --- → 0.7
verified with
Mozilla/5.0 (Windows; U; Windows NT 5.1; pl; rv:1.8.1.8pre) Gecko/20071002 Sunbird/0.7
Status: RESOLVED → VERIFIED
(In reply to comment #48)
You verified the fix with a build that never showed the issue, see Comment #13

Verified correct behavior on Windows 2000 using:
  Thunderbird 2.0.0.6 (20070728) + Lightning 0.7 (2007100504)
  Thunderbird 2.0.0.7pre (20071005) + Lightning 0.7 (2007100504)
  Thunderbird 3.0a1pre (2007100503) + Lightning 0.6a1 (2007100602)
and Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a9pre) Gecko/2007100507 Calendar/0.6a1
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: