RESOLVED FIXED209493
Element context character rects may be in wrong coordinate system 
https://bugs.webkit.org/show_bug.cgi?id=209493
Summary Element context character rects may be in wrong coordinate system
Daniel Bates
Reported 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.
Attachments
Patch and tests (6.73 KB, patch)
2020-03-24 13:57 PDT, Daniel Bates 		no flags
DetailsFormatted DiffDiff
To Land (6.78 KB, patch)
2020-03-25 14:54 PDT, Daniel Bates 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2020-03-24 13:55:27 PDT
<rdar://problem/60840261>
Daniel Bates
Comment 2 2020-03-24 13:57:48 PDT
Created attachment 394407 [details] Patch and tests
Wenson Hsieh
Comment 3 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?
Daniel Bates
Comment 4 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.
Daniel Bates
Comment 5 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.
Daniel Bates
Comment 6 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>
Daniel Bates
Comment 7 2020-03-24 15:09:04 PDT
All reviewed patches have been landed. Closing bug.
Daniel Bates
Comment 8 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....
Daniel Bates
Comment 9 2020-03-24 22:53:28 PDT
I'm just going to revert this change for the moment.
Daniel Bates
Comment 10 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>
Daniel Bates
Comment 11 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');
Daniel Bates
Comment 12 2020-03-25 14:54:31 PDT
Created attachment 394546 [details] To Land
Daniel Bates
Comment 13 2020-03-25 14:56:12 PDT
Committed r259013: <https://trac.webkit.org/changeset/259013>
Note You need to log in before you can comment on or make changes to this bug.