Bug 209493

Summary:

Element context character rects may be in wrong coordinate system

Product:

WebKit

Reporter:

Daniel Bates <dbates>

Component:

WebKit Misc.

Assignee:

Daniel Bates <dbates>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

webkit-bug-importer, wenson_hsieh

Priority:

Keywords:

InRadar

Version:

WebKit Local Build

Hardware:

iPhone / iPad

OS:

iOS 13

See Also:

https://bugs.webkit.org/show_bug.cgi?id=209506

Attachments:

Description	Flags
Patch and tests	none
To Land	none

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	Details Formatted Diff Diff
To Land (6.78 KB, patch) 2020-03-25 14:54 PDT, Daniel Bates	no flags	Details Formatted Diff Diff
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.