REGRESSION: Google Calendar cell highlight misplaced

[Antti Koivisto]

1. login to Google Calendar
2. click somewhere on main calendar area to select a time

The grey transparent highlight is drawn shifted several pixels right/down from cell area where it should be. Works fine in shipping Safari.

Could be a result of changing offsetTop/offsetLeft behavior to match Firefox recently.

(In reply to comment #1)
> Could be a result of changing offsetTop/offsetLeft behavior to match Firefox
> recently.
> 

Wrong. Based on nightly builds, the regression happened sometime between 2005-12-01 and 2005-12-16.

Perhaps this: <http://trac.webkit.org/projects/webkit/changeset/11484>.

[Antti Koivisto]

No, wasn't that one. Something else...(In reply to comment #3)
> Perhaps this: <http://trac.webkit.org/projects/webkit/changeset/11484>.
> 

No, doesn't seem to be that one. 

WebKit-CVS-2005-12-07 10-27-14 GMT.dmg works
WebKit-CVS-2005-12-08 23-04-18 GMT.dmg broken

[Antti Koivisto]

If you revert both Bug 11109 and http://trac.webkit.org/projects/webkit/changeset/11484 then you get correct positioning. This is because Calendar works around offsetLeft/offsetTop/offsetParent bug in tiger webkit. Specifically tiger webkit would not include body margins to offsets of relative positioned elements.

This code in function qC(a, b, c, d) in the obfuscated calendar script

      if (T)
        i.x += 3;
      else
        if ($)
          {
            i.x += fb() ? 6 : 8;
            i.y += 6;
          }

adds some magic offsets to fix positioning issues. For safari $=1, for MSIE T=1.

Safari offsetLeft behavior is still not exactly the same as Firefox. For some reason, like MSIE, ToT Safari needs that i.x += 3; to get pixel-correct horizontal position. Is the idea here to try to replicate MSIE or Firefox behavior for offset functions? If target is MSIE then I don't think there is any bug here.

(In reply to comment #6)
> Safari offsetLeft behavior is still not exactly the same as Firefox.

> Is the idea here to try to replicate MSIE or Firefox
> behavior for offset functions?

My intention with the patch for bug 11109 was to replicate Firefox (WinIE behavior is described in one of my comments on that bug). Do you know what the remaining discrepancy is?

[Antti Koivisto]

Created attachment 13086 [details]
test case showing different offset behavior compared to ie/ff

Both FF and IE agree that offsetLeft/Top should be measured from inner border edge of the offsetParent. WebKit measures it from outer border edge of the parent. 

In this case first offsetLeft is zero in FF/IE and non-zero in Safari

[Antti Koivisto]

Here is a nice test suite for offset properties by Anne van Kesteren: 

http://dump.testsuite.org/2006/dom/style/offset/

We get quite a few wrong and more importantly we get quite a few different from FF. Especially offsetParent seems to be often wrong.

[David Kilzer (:ddkilzer)]

Per Comment #6, it sounds like both Safari and Google Calendar will need fixes to resolve this bug.

[David Kilzer (:ddkilzer)]

(In reply to comment #9)
> Here is a nice test suite for offset properties by Anne van Kesteren: 
> 
> http://dump.testsuite.org/2006/dom/style/offset/
> 
> We get quite a few wrong and more importantly we get quite a few different from
> FF. Especially offsetParent seems to be often wrong.

Filed Bug 12713 to import this test suite.

[Antti Koivisto]

Created attachment 13091 [details]
proposed patch

Ok, so here is proposed patch. It does three things
1) calculate offsets from padding edge (inner edge of border), not from border edge
2) revert this 

!(!style()->htmlHacks() && skipTables ? curr->isRoot() : curr->isBody())

in http://trac.webkit.org/projects/webkit/changeset/11484 back to this

!curr->isBody()

which seems to match FF behavior in both strict and compat modes. Why was this change done? Hard to say since there was no test case nor explanation.

3) Check for tableness from dom tree instead of rendering tree. That's just what other browser seem to do based on Anne's tests.

These changes make all Anne's tests pass except for 3,8,12,29,43,45,49,50. In all those cases we match FF behavior (except 12, where we fail a bit less). Failures are of course all debatable since there is no authorative spec about offset behaviors. No layout test failures either.

(some of Anne's test cases may randomly produce wrong offsetTop results when reloaded. that is some layout timing problem not directly related to offset calculations)

None of this actually fixes Google Calendar, it needs ToT fixes in the other end. Best we can do is to make this stuff consistent.

[Bob Austin]

This problem has been written to the Google bug tracking system.
631734

[Maciej Stachowiak]

Comment on attachment 13091 [details]
proposed patch

Fix looks good (Mitz agrees) but needs ChangeLog entry and test case. Feel free to convert this to an r+ if you add a ChangeLog and land the attached test case w/ this.

Also, this won't really make the bug on Google go away, but being more like FF will allow us to turn this into strictly an evangelism bug.

[Maciej Stachowiak]

Comment on attachment 13091 [details]
proposed patch

r- per above comments

[Antti Koivisto]

Commited as r19567 with attached layout test based on comment  #14 above.

[Antti Koivisto]

Comment on attachment 13091 [details]
proposed patch

see comment 14