334765 - Google calendar creates appointments for the wrong dates

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Eli Friedman]

Steps:
1. Scroll down in view of week
2. Click to create appointment

Expected results:
Appointment time is the time that was clicked

Actual results:
Appointment time offset by some amount which depends on where you are scrolled.

The bug is in nsGenericHTMLElement::GetOffsetRect; bug 328881 changed the way adding up frame offsets works.

Testcase coming up.

Comment 1

[Eli Friedman]

Attachment: Testcase

Comment 2

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

*** Bug 330619 has been marked as a duplicate of this bug. ***

Comment 3

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

Attachment: fix

1) Ignore scroll position while compute offset rects for boxobjects and HTML elements
2) Move code to union continuation rects into a shared function

Comment 4

[Johnny Stenback (:jst)]

FWIW, I can confirm that this does indeed fix the google calendar regression.

Comment 5

[timeless]

erm. why isn't this in cvs? or is it?

Comment 6

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #5)
> erm. why isn't this in cvs? or is it?

Because it's still waiting for r/sr? 

Comment 7

[Boris Zbarsky [:bzbarsky]]

roc, once this lands the fixed-pos code in nsHTMLReflowState::CalculateHypotheticalBox should also switch to nsLayoutUtils::GetOffsetIgnoringScrolling, right?

Comment 8

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

Yes.

Comment 9

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

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

Tossing the review to Boris.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

>Index: layout/base/nsLayoutUtils.cpp
>+nsLayoutUtils::GetUnionOfAllRects(nsIFrame* aFrame)

Should this guard against aFrame->GetParent() being null?  It'll crash at the moment if that happens.

>Index: layout/base/nsLayoutUtils.h
>+   * @return the offset of aFrame from its parent, as if it was scrolled to

s/was/were/

r+sr=bzbarsky with those nits; let's do the reflow state change in a followup.

Comment 11

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

(In reply to comment #10)
> (From update of attachment 220044 [details] [diff] [review] [edit])
> >Index: layout/base/nsLayoutUtils.cpp
> >+nsLayoutUtils::GetUnionOfAllRects(nsIFrame* aFrame)
> 
> Should this guard against aFrame->GetParent() being null?  It'll crash at the
> moment if that happens.

Yes. I'll just return aFrame->GetRect() in that case (there can't be continuations).

Comment 12

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

checked in

Comment 13

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

This may have caused a Tp regression on btek :-(

Comment 14

[Bill Gianopoulos [:WG9s]]

I wonder if this is also responsible for Thunderbird crazyhorse orange.

Comment 15

[Bill Gianopoulos [:WG9s]]

(In reply to comment #14)
> I wonder if this is also responsible for Thunderbird crazyhorse orange.
> 

Evidently not.

Comment 16

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

Backing out seemed to show that this is indeed the cause of the btek regression :-(.

But do these GetOffsetRect functions actually get called during Tp? Through XUL maybe? Tomorrow I'll try relanding just the HTML part of the fix.

Comment 17

[Boris Zbarsky [:bzbarsky]]

I wouldn't be surprised if the Tp pages call those functions... you could breakpoint and run Tp and see?

Comment 18

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

I added printfs to nsBoxObject::GetOffsetRect and nsGenericHTMLElement::GetOffsetRect. They don't get hit during Tp :-|.

Comment 19

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

Attachment: new patch

Alright, here's a new approach. It should be faster than the previous try, just in case that matters, and it also changes only nsBoxObject for now. Basically I've added a custom method to nsIFrame to avoid the QueryInterface call.

If this lands, and it doesn't perturb btek or anyone else, then I'll make another patch for nsGenericHTMLElement offsetX/Y.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 225540 [details] [diff] [review]
new patch

>Index: layout/generic/nsGfxScrollFrame.h
>+    if (aChild == mInner.GetScrolledFrame()) pt += GetScrollPosition();
...
>+    if (aChild == mInner.GetScrolledFrame()) pt -= GetScrollPosition();

Those can't both be right.  The += should be what you want, right?

r+sr=bzbarsky with that fixed.

Comment 21

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

Yes, += is what I want, and what I tested :-|

Comment 22

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

checked in. Waiting for performance results...

Comment 23

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

No btek regression this time. This regression is fixed.

Comment 24

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED for me using build 2006-06-19-10 of SeaMonkey trunk under Windows XP with the Google Calendar application at http://www.google.com/calendar/render?pli=1.  Dates can now be correctly chosen.