Bug 209493 - Element context character rects may be in wrong coordinate system
Summary: Element context character rects may be in wrong coordinate system
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Local Build
Hardware: iPhone / iPad iOS 13
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-03-24 13:55 PDT by Daniel Bates
Modified: 2020-03-25 14:56 PDT (History)
2 users (show)

See Also:


Attachments
Patch and tests (6.73 KB, patch)
2020-03-24 13:57 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
To Land (6.78 KB, patch)
2020-03-25 14:54 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2020-03-24 13:55:17 PDT
Consider a page with the following markup:

<!DOCTYPE html>
<html>
<body>
<input>
<iframe width="500" height="500" srcdoc="<textarea>Hello</textarea>"></iframe>
</body>
</html>

If you set the set the insertion point to be before the 'H' in the textarea and -requestDocumentContext for character rects then the returned rects are in the coordinate system of the framed document, but they should be in root view coordinates.
Comment 1 Radar WebKit Bug Importer 2020-03-24 13:55:27 PDT
<rdar://problem/60840261>
Comment 2 Daniel Bates 2020-03-24 13:57:48 PDT
Created attachment 394407 [details]
Patch and tests
Comment 3 Wenson Hsieh 2020-03-24 14:03:22 PDT
Comment on attachment 394407 [details]
Patch and tests

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

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:4254
> +                rects.append({ currentRange->ownerDocument().view()->contentsToRootView(absoluteBoundingBox), { offsetSoFar++, stride } });

What guarantees that view() is non-null here?
Comment 4 Daniel Bates 2020-03-24 14:54:28 PDT
Comment on attachment 394407 [details]
Patch and tests

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

Thanks for the review, Wenson.

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:4254
>> +                rects.append({ currentRange->ownerDocument().view()->contentsToRootView(absoluteBoundingBox), { offsetSoFar++, stride } });
> 
> What guarantees that view() is non-null here?

How can it be null here? The owner document for these ranges has to be the document in the focused frame since the range is constructed with respect to the frame's selection. If focusedOrMainFrame() returns a frame without a view then it must the main frame because detaching a focused frame, which is what had to happen for its view to be nullified, defocuses it. And the main frame is the focused frame of last resort. So, that means it only the main frame can be returned and possibly not have a view. But that cannot happen either because there is nothing that this function does that could cause that... oh, I misspoke, layout. That's an existing bug in this function not related to this patch...Anyway, I'll take out a ref for the frame view of the focused frame at the top of this function.
Comment 5 Daniel Bates 2020-03-24 15:08:04 PDT
Comment on attachment 394407 [details]
Patch and tests

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

>>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:4254
>>> +                rects.append({ currentRange->ownerDocument().view()->contentsToRootView(absoluteBoundingBox), { offsetSoFar++, stride } });
>> 
>> What guarantees that view() is non-null here?
> 
> How can it be null here? The owner document for these ranges has to be the document in the focused frame since the range is constructed with respect to the frame's selection. If focusedOrMainFrame() returns a frame without a view then it must the main frame because detaching a focused frame, which is what had to happen for its view to be nullified, defocuses it. And the main frame is the focused frame of last resort. So, that means it only the main frame can be returned and possibly not have a view. But that cannot happen either because there is nothing that this function does that could cause that... oh, I misspoke, layout. That's an existing bug in this function not related to this patch...Anyway, I'll take out a ref for the frame view of the focused frame at the top of this function.

Will fix this in bug #209506.
Comment 6 Daniel Bates 2020-03-24 15:09:02 PDT
Comment on attachment 394407 [details]
Patch and tests

Clearing flags on attachment: 394407

Committed r258945: <https://trac.webkit.org/changeset/258945>
Comment 7 Daniel Bates 2020-03-24 15:09:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Daniel Bates 2020-03-24 22:51:35 PDT
Dang it, I forgot this patch depends on another bug fix I didn't post yet to make selecting position at point work in <iframes>. That's why the test fails. I'm going to disable the test temporarily for tonight. Tomorrow I will have the patch to fix it....
Comment 9 Daniel Bates 2020-03-24 22:53:28 PDT
I'm just going to revert this change for the moment.
Comment 10 Daniel Bates 2020-03-24 23:00:28 PDT
Reverted r258945 for reason:

Revert change that broke API tests while I investigate offline.

Committed r258974: <https://trac.webkit.org/changeset/258974>
Comment 11 Daniel Bates 2020-03-25 14:52:42 PDT
Comment on attachment 394407 [details]
Patch and tests

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/DocumentEditingContext.mm:520
> +    [webView synchronouslyLoadHTMLString:applyAhemStyle([NSString stringWithFormat:@"<iframe srcdoc=\"%@\" style='position: absolute; left: 1em; top: 1em; border: none'></iframe>", applyAhemStyle(@"<textarea id='test' style='padding: 0'>The quick brown fox jumps over the lazy dog.</textarea><script>let textarea = document.querySelector('iframe').contentDocument.getElementById('test'); textarea.focus(); textarea.setSelectionRange(0, 0); /* Place caret at the beginning of the field. */</script>")])];

Here's the bug that led to the test failure....

let textarea = document.querySelector('iframe').contentDocument.getElementById('test');

should be:

let textarea = document.getElementById('test');
Comment 12 Daniel Bates 2020-03-25 14:54:31 PDT
Created attachment 394546 [details]
To Land
Comment 13 Daniel Bates 2020-03-25 14:56:12 PDT
Committed r259013: <https://trac.webkit.org/changeset/259013>