Bug 13467 - REGRESSION (r20972): Google Spreadsheets: spreadsheet controls not displayed
Summary: REGRESSION (r20972): Google Spreadsheets: spreadsheet controls not displayed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 523.x (Safari 3)
Hardware: Macintosh Intel OS X 10.4
: P1 Normal
Assignee: Nobody
URL:
Keywords: GoogleBug, InRadar, Regression
Depends on:
Blocks:
 
Reported: 2007-04-23 23:01 PDT by Tommi Komulainen
Modified: 2007-06-11 22:21 PDT (History)
6 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Tommi Komulainen 2007-04-23 23:01:27 PDT
I know Google says Spreadsheets does not support Safari yet, but it used to work just fine with r20961.

With r21003 when loading a spreadsheet, the controls (toolbars etc.) disappear and only the spreadsheet cells (iframe?) are shown in the window. The cells fill the whole window and keyboard navigation no longer works between cells, but instead scrolls the whole window.
Comment 1 mitz 2007-05-01 13:18:59 PDT
Regressed in <http://trac.webkit.org/projects/webkit/changeset/20972> (fix for <rdar://problem/4671964>). Rolling out that patch fixes this bug.
Comment 2 Antti Koivisto 2007-05-02 06:16:51 PDT
This was broken by r20972 which removed document property from iframe element. This was done to fix a bug with SAP application. The new behavior should match Firefox.

The problem is that after this change the spreadsheet grid gets loaded to main view instead of the iframe it should go to.

Looking around the obfuscated code in http://spreadsheets.google.com/client/js/2100283068-trix_main.js I saw the following function:

  ff[_P].hj = function ()
  {
    if ($b)
      {
        if (this.Ui[hb] == null)
          {
            return null;
          }
        return this.Ui[hb][lg];
      }
    else
      {
        return this.Ui[jg];
      }
  };

Here $b is a Safari test result, hb="document", lg="defaultView" and jg="contentWindow" and this.Ui is an iframe element (I think). Basically for Safari it does "iframe.document.defaultView" whereas for all other browsers it does "iframe.contentWindow". With iframe.document gone, this would not work anymore. I can't verify that this exact function causes the problem (there are other Safari tests and other places where iframe content is accessed in various ways) but it certainly looks like something which would fail after the change in r20972.
Comment 3 W. Andy Carrel 2007-05-02 09:18:29 PDT
Nice detective work, thanks! I've made contact with an appropriate engineer.
Comment 4 Antti Koivisto 2007-05-02 13:45:02 PDT
I tested that at least in simple cases iframe.contentWindow works fine in ToT, so the special code path for Safari should no be needed. Please tell if you find there is some more subtle brokeness that still needs special handling so we can fix it.
Comment 5 Darin Adler 2007-05-04 22:18:23 PDT
<rdar://problem/5183687>
Comment 6 Sam Weinig 2007-05-24 23:14:17 PDT
I can no longer test this as I can no longer get a spreadsheet up at all.  I am getting this assert while loading:

ASSERTION FAILED: startPosition.isNotNull() && endPosition.isNotNull()
(rendering/RenderTextControl.cpp:433 void WebCore::RenderTextControl::setSelectionRange(int, int))

Comment 7 mitz 2007-05-25 00:15:47 PDT
(In reply to comment #6)
> I can no longer test this as I can no longer get a spreadsheet up at all.  I am
> getting this assert while loading:
> 
> ASSERTION FAILED: startPosition.isNotNull() && endPosition.isNotNull()
> (rendering/RenderTextControl.cpp:433 void
> WebCore::RenderTextControl::setSelectionRange(int, int))
> 

Apply the patch from bug 13413 :)
Comment 8 Justin Garcia 2007-06-11 22:21:17 PDT
Looks OK with Safari 3 Beta.