Bug 172703

Summary: REGRESSION(r215946): Can't reference a table cell in Google spreadsheet
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: WebKit Misc.Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, bfulgham, cdumez, commit-queue, jond, simon.fraser, webkit-bug-importer, zcorpan
Priority: P2 Keywords: InRadar, Regression
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
URL: https://drafts.csswg.org/cssom-view/#extensions-to-the-range-interface
Attachments:
Description Flags
WIP Patch
none
WIP Patch
none
WIP Patch
none
WIP Patch
none
Patch none

Description Ryosuke Niwa 2017-05-29 23:42:25 PDT
Reproduction steps:
1. Go to drive.google.com
2. Create a new Google spreadsheet or open an existing one
3. Click on one of the cells and type in = and then click another cell to reference it.
4. Press enter.

Result:
Google spreadsheet shows "Unable to load file" as an error.
Comment 1 Ryosuke Niwa 2017-05-29 23:42:37 PDT
Observed on STP30.
Comment 2 Ryosuke Niwa 2017-05-29 23:43:49 PDT
In the console, I see the error:

CustomError: Error in protected function: a.item is not a function. (In 'a.item(0)', 'a.item' is undefined)
Comment 3 Radar WebKit Bug Importer 2017-05-29 23:44:58 PDT
<rdar://problem/32458086>
Comment 4 Ryosuke Niwa 2017-05-30 10:29:01 PDT
Bisected to
http://trac.webkit.org/log/trunk/?mode=follow_copy&rev=215947&stop_rev=215945

It must be https://trac.webkit.org/changeset/215946/webkit given the error:

CustomError: Error in protected function: a.item is not a function. (In 'a.item(0)', 'a.item' is undefined)

This change was probably not compatible with the Web. I bet there is thousands of other web pages that rely on .item(~) call.
Comment 5 Chris Dumez 2017-05-30 10:32:37 PDT
Will look into this today.
Comment 6 Chris Dumez 2017-05-30 10:45:11 PDT
Yes, ClientRectList had an item() method, which is missing from the new DOMRect[] type that is returned by getClientRects() in the spec. I'll looks into the Google spreadsheet code a little but it looks to me that the specification is not WebCompatible and we may need to reintroduce a DOMRectList type :(
Comment 7 Chris Dumez 2017-05-30 10:50:16 PDT
For now, I am unable to reproduce with WebKit ToT. Will try some more.
Comment 8 Chris Dumez 2017-05-30 10:52:20 PDT
Ok, got it:
function Epb(a) {
    Dpb(a);
    a = xpb(a).Pq().getClientRects();
    return {
        first: a.item(0),
        nV: a.item(a.length - 1)
    }
}

in https://docs.google.com/static/spreadsheets2/client/js/2594367115-ritz_waffle_i18n_core.js
Comment 9 Chris Dumez 2017-05-30 11:00:02 PDT
(In reply to Chris Dumez from comment #8)
> Ok, got it:
> function Epb(a) {
>     Dpb(a);
>     a = xpb(a).Pq().getClientRects();
>     return {
>         first: a.item(0),
>         nV: a.item(a.length - 1)
>     }
> }
> 
> in
> https://docs.google.com/static/spreadsheets2/client/js/2594367115-
> ritz_waffle_i18n_core.js

Filed https://github.com/w3c/csswg-drafts/issues/1479 against the specification. Will now re-introduce a List type in WebKit to fix Google Speadsheet.
Comment 10 Chris Dumez 2017-05-30 12:20:25 PDT
Created attachment 311527 [details]
WIP Patch
Comment 11 Chris Dumez 2017-05-30 12:41:58 PDT
Created attachment 311529 [details]
WIP Patch
Comment 12 Chris Dumez 2017-05-30 13:03:47 PDT
Created attachment 311532 [details]
WIP Patch
Comment 13 Chris Dumez 2017-05-30 13:13:29 PDT
Created attachment 311533 [details]
WIP Patch
Comment 14 Chris Dumez 2017-05-30 13:34:20 PDT
Created attachment 311539 [details]
Patch
Comment 15 Ryosuke Niwa 2017-05-30 13:39:54 PDT
Comment on attachment 311539 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=311539&action=review

> Source/WebCore/dom/DOMRectList.idl:33
> +] interface DOMRectList {
> +    readonly attribute unsigned long length;
> +    getter DOMRect item(unsigned long index);
> +};

Huh, this doesn't have an indexed getter!? No wonder Google docs is using item to access each item.
Comment 16 Chris Dumez 2017-05-30 13:58:37 PDT
Comment on attachment 311539 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=311539&action=review

>> Source/WebCore/dom/DOMRectList.idl:33
>> +};
> 
> Huh, this doesn't have an indexed getter!? No wonder Google docs is using item to access each item.

This is an indexed getter:
getter DOMRect item(unsigned long index);
Comment 17 Chris Dumez 2017-05-30 14:19:34 PDT
Comment on attachment 311539 [details]
Patch

https://trac.webkit.org/r217576