Bug 198928

Summary:

m_focusedElement != &element in WebPage::elementDidBlur() sometimes; nonsense blur event fired at frame

Product:

WebKit

Reporter:

Daniel Bates <dbates>

Component:

WebKit Misc.

Assignee:

Daniel Bates <dbates>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, bfulgham, ews-watchlist, rniwa, webkit-bot-watchers-bugzilla, webkit-bug-importer, wenson_hsieh

Priority:

Keywords:

InRadar

Version:

WebKit Local Build

Hardware:

iPhone / iPad

OS:

iOS 12

URL:

https://www-archive.mozilla.org/editor/midasdemo/

See Also:

https://bugs.webkit.org/show_bug.cgi?id=198922
https://bugs.webkit.org/show_bug.cgi?id=199039
https://bugs.webkit.org/show_bug.cgi?id=195749

Bug Depends on:

Bug Blocks:

190571

Attachments:

Description	Flags
Patch	none
Patch	none
Archive of layout-test-results from ews126 for ios-simulator-wk2	none
To land	none

Daniel Bates

Reported 2019-06-17 11:44:02 PDT

Same repro steps as in bug #198922. May need to revert the change for that bug before investigating this bug.... Unclear of the implications of this bug at the moment other than causing bug #198922.

Attachments
Patch (10.16 KB, patch) 2019-06-18 13:57 PDT, Daniel Bates	no flags	Details Formatted Diff Diff
Patch (10.16 KB, patch) 2019-06-18 14:52 PDT, Daniel Bates	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.62 MB, application/zip) 2019-06-18 16:51 PDT, EWS Watchlist	no flags	Details
To land (11.10 KB, patch) 2019-06-24 12:46 PDT, Daniel Bates	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2019-06-17 11:44:15 PDT

<rdar://problem/51814327>

Daniel Bates

Comment 2 2019-06-18 13:34:56 PDT

For some reason the was never explained in WebKit2 (and only ever in WebKit2) we would mutate the focused frame when computing positional information (say for a tap): <https://trac.webkit.org/browser/trunk/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm?rev=246325#L2591>. ^^^ This is the source of this bug.

Daniel Bates

Comment 3 2019-06-18 13:35:20 PDT

Simpler test case: <!DOCTYPE html> <html> <body> <p>Tap the text field. Then tap the main page's background. Then dismiss the keyboard.</p> <iframe src="data:text/html,<input>"></iframe> </body> </html>

Daniel Bates

Comment 4 2019-06-18 13:57:09 PDT

Created attachment 372379 [details] Patch This patch depends on the test added in bug #198922 and will fail to apply.

Daniel Bates

Comment 5 2019-06-18 14:52:26 PDT

Created attachment 372387 [details] Patch Re-upload to trigger EWS now that the patch for bug #198922 landed.

EWS Watchlist

Comment 6 2019-06-18 16:51:56 PDT

Comment on attachment 372387 [details] Patch Attachment 372387 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/12513239 New failing tests: editing/deleting/smart-delete-paragraph-003.html

EWS Watchlist

Comment 7 2019-06-18 16:51:58 PDT

Created attachment 372406 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.14.5

Brent Fulgham

Comment 8 2019-06-19 10:22:40 PDT

Comment on attachment 372387 [details] Patch Good riddance! r=me

Daniel Bates

Comment 9 2019-06-19 16:01:42 PDT

How do I interpret this? --- /Volumes/Scratch/WebKitBuild/Debug-iphonesimulator/layout-test-results/editing/deleting/smart-delete-paragraph-003-expected.txt +++ /Volumes/Scratch/WebKitBuild/Debug-iphonesimulator/layout-test-results/editing/deleting/smart-delete-paragraph-003-actual.txt @@ -1,8 +1,5 @@ EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 3 of BODY > HTML > #document to 3 of BODY > HTML > #document toDOMRange:range from 0 of DIV > #document-fragment to 0 of DIV > #document-fragment affinity:NSSelectionAffinityDownstream stillSelecting:FALSE -EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification -EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 0 of DIV > #document-fragment to 0 of DIV > #document-fragment toDOMRange:range from 0 of DIV > #document-fragment to 0 of DIV > #document-fragment affinity:NSSelectionAffinityDownstream stillSelecting:FALSE -EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 0 of DIV > #document-fragment to 0 of DIV > #document-fragment toDOMRange:range from 0 of #text > DIV > #document-fragment to 4 of #text > DIV > #document-fragment affinity:NSSelectionAffinityDownstream stillSelecting:FALSE EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification EDITING DELEGATE: shouldDeleteDOMRange:range from 0 of #text > DIV > #document-fragment to 15 of #text > DIV > #document-fragment

Daniel Bates

Comment 10 2019-06-20 16:09:29 PDT

(In reply to Daniel Bates from comment #9) > How do I interpret this? > > --- > /Volumes/Scratch/WebKitBuild/Debug-iphonesimulator/layout-test-results/ > editing/deleting/smart-delete-paragraph-003-expected.txt > +++ > /Volumes/Scratch/WebKitBuild/Debug-iphonesimulator/layout-test-results/ > editing/deleting/smart-delete-paragraph-003-actual.txt > @@ -1,8 +1,5 @@ > EDITING DELEGATE: > webViewDidChangeSelection:WebViewDidChangeSelectionNotification > EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 3 of BODY > HTML > > #document to 3 of BODY > HTML > #document toDOMRange:range from 0 of DIV > > #document-fragment to 0 of DIV > #document-fragment > affinity:NSSelectionAffinityDownstream stillSelecting:FALSE > -EDITING DELEGATE: > webViewDidChangeSelection:WebViewDidChangeSelectionNotification > -EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 0 of DIV > > #document-fragment to 0 of DIV > #document-fragment toDOMRange:range from 0 > of DIV > #document-fragment to 0 of DIV > #document-fragment > affinity:NSSelectionAffinityDownstream stillSelecting:FALSE > -EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 0 of DIV > > #document-fragment to 0 of DIV > #document-fragment toDOMRange:range from 0 > of #text > DIV > #document-fragment to 4 of #text > DIV > #document-fragment > affinity:NSSelectionAffinityDownstream stillSelecting:FALSE > EDITING DELEGATE: > webViewDidChangeSelection:WebViewDidChangeSelectionNotification > EDITING DELEGATE: > webViewDidChangeSelection:WebViewDidChangeSelectionNotification > EDITING DELEGATE: shouldDeleteDOMRange:range from 0 of #text > DIV > > #document-fragment to 15 of #text > DIV > #document-fragment I'm going to land updated expectations. Other than the delegate messages the actual substance of the test (deleting the middle paragraph) is not regressed.

Daniel Bates

Comment 11 2019-06-20 16:22:28 PDT

Megan gave her blessing to land skipping the test (since we thought smart delete in <textarea> was broken <-- or at least it didn't work when we tried yesterday - filed bug #199039). Kinda wavering though since the delegate callback differences could be some coal mine canary.

Ryosuke Niwa

Comment 12 2019-06-20 16:24:56 PDT

(In reply to Daniel Bates from comment #9) > How do I interpret this? > > --- > /Volumes/Scratch/WebKitBuild/Debug-iphonesimulator/layout-test-results/ > editing/deleting/smart-delete-paragraph-003-expected.txt > +++ > /Volumes/Scratch/WebKitBuild/Debug-iphonesimulator/layout-test-results/ > editing/deleting/smart-delete-paragraph-003-actual.txt > @@ -1,8 +1,5 @@ > EDITING DELEGATE: > webViewDidChangeSelection:WebViewDidChangeSelectionNotification > EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 3 of BODY > HTML > > #document to 3 of BODY > HTML > #document toDOMRange:range from 0 of DIV > > #document-fragment to 0 of DIV > #document-fragment > affinity:NSSelectionAffinityDownstream stillSelecting:FALSE > -EDITING DELEGATE: > webViewDidChangeSelection:WebViewDidChangeSelectionNotification > -EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 0 of DIV > > #document-fragment to 0 of DIV > #document-fragment toDOMRange:range from 0 > of DIV > #document-fragment to 0 of DIV > #document-fragment > affinity:NSSelectionAffinityDownstream stillSelecting:FALSE > -EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 0 of DIV > > #document-fragment to 0 of DIV > #document-fragment toDOMRange:range from 0 > of #text > DIV > #document-fragment to 4 of #text > DIV > #document-fragment > affinity:NSSelectionAffinityDownstream stillSelecting:FALSE > EDITING DELEGATE: > webViewDidChangeSelection:WebViewDidChangeSelectionNotification > EDITING DELEGATE: > webViewDidChangeSelection:WebViewDidChangeSelectionNotification > EDITING DELEGATE: shouldDeleteDOMRange:range from 0 of #text > DIV > > #document-fragment to 15 of #text > DIV > #document-fragment This is a progression. We shouldn't be setting selection to a node in a document fragment.

Daniel Bates

Comment 13 2019-06-20 16:27:21 PDT

(In reply to Ryosuke Niwa from comment #12) > (In reply to Daniel Bates from comment #9) > > How do I interpret this? > > > > --- > > /Volumes/Scratch/WebKitBuild/Debug-iphonesimulator/layout-test-results/ > > editing/deleting/smart-delete-paragraph-003-expected.txt > > +++ > > /Volumes/Scratch/WebKitBuild/Debug-iphonesimulator/layout-test-results/ > > editing/deleting/smart-delete-paragraph-003-actual.txt > > @@ -1,8 +1,5 @@ > > EDITING DELEGATE: > > webViewDidChangeSelection:WebViewDidChangeSelectionNotification > > EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 3 of BODY > HTML > > > #document to 3 of BODY > HTML > #document toDOMRange:range from 0 of DIV > > > #document-fragment to 0 of DIV > #document-fragment > > affinity:NSSelectionAffinityDownstream stillSelecting:FALSE > > -EDITING DELEGATE: > > webViewDidChangeSelection:WebViewDidChangeSelectionNotification > > -EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 0 of DIV > > > #document-fragment to 0 of DIV > #document-fragment toDOMRange:range from 0 > > of DIV > #document-fragment to 0 of DIV > #document-fragment > > affinity:NSSelectionAffinityDownstream stillSelecting:FALSE > > -EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 0 of DIV > > > #document-fragment to 0 of DIV > #document-fragment toDOMRange:range from 0 > > of #text > DIV > #document-fragment to 4 of #text > DIV > #document-fragment > > affinity:NSSelectionAffinityDownstream stillSelecting:FALSE > > EDITING DELEGATE: > > webViewDidChangeSelection:WebViewDidChangeSelectionNotification > > EDITING DELEGATE: > > webViewDidChangeSelection:WebViewDidChangeSelectionNotification > > EDITING DELEGATE: shouldDeleteDOMRange:range from 0 of #text > DIV > > > #document-fragment to 15 of #text > DIV > #document-fragment > > This is a progression. We shouldn't be setting selection to a node in a > document fragment. Hallelujah! 😀

Ryosuke Niwa

Comment 14 2019-06-20 16:28:08 PDT

Actually, wait a minute. This document fragment is probably a shadow root.

Ryosuke Niwa

Comment 15 2019-06-20 16:31:27 PDT

Okay, so we used to select "Test" then "Test paragraph" before this change. Now we're selecting "Test paragraph" outright without first selecting "Test".

Daniel Bates

Comment 16 2019-06-24 12:28:04 PDT

I'm done with editing/deleting/smart-delete-paragraph-003.html. So, the cause of the delegate difference is because I removed <https://trac.webkit.org/browser/trunk/Source/WebKit/Shared/ios/InteractionInformationRequest.cpp?rev=246682#L72> and <https://trac.webkit.org/browser/trunk/Source/WebKit/Shared/ios/InteractionInformationRequest.cpp?rev=246682#L86>. When these early returns are removed, then double-tapping here <https://trac.webkit.org/browser/trunk/LayoutTests/resources/ui-helper.js?rev=246682#L214> does nothing. Now here is why I'm giving up on this test for now, skipping it, and punting this investigate into bug #199039: 1. UIHelper.doubleTapAt() ***sometimes*** doesn't do anything. Double tap still **works** without those lines. I can run the test manually and double tap the word "Test" in the first sentence after focusing the textarea (just like the test is coded to do <--- more on what this test is suppose to do in a moment). Manually running UIHelper.doubleTapAt() and/or adjusting the coordinates passed a little eventually makes it select the first word. 2. The test doesn't emit the actual output! I got confused (see (3) for more confusion) because the English in the test shows an expected result, but the contents of the textarea aren't actually dumped! So, who knows what is happening: unless you can read the editing delegate dump you don't know if these test is passing or failing. This just makes this test a time sink to investigate (as I fell into today), but I'm not going to continue because of other inconsistencies. If I were to continue then I would have to learn all about and possibly re-implement the original feature of smart delete and that does not seem fair. 3. This test doesn't know what it wants to test or I can't make heads or tails what it wants. The test description is "Smart paste when pasting a paragraph between two paragraphs." <-- It doesn't do this! The second part of the description is the expected result: [[ Extra newlines should be removed to maintin spacing between paragraphs. It should like this: Test paragraph. Last test paragraph. ]] And the <textarea> (that we operate on initially contains): [[ Test paragraph. Test paragraph to remove. Last test paragraph. ]] But the code does THIS!!! (formatting and comments by me): [[ internals.settings.setEditingBehavior("ios"); // Enable some setting // This mega function does: // 1. Tap the <textarea> to focus and wait for the keyboard to appear // 2. Double tap (x, y) = (29, 29) on iPad 6th gen => the first word in the FIRST sentence. <-- This selects the first word. await UIHelper.selectWordByDoubleTapOrClick(document.getElementById('test')); extendSelectionForwardByLineBoundaryCommand(); // Extend the selection to the end of the line. (i.e. window.getSelection().modify("extend", "forward", "lineBoundary")) cutCommand(); // Perform a cut. So, delete the line (it's copied to Clipboard) ]] ^^^^^ notice NONE of this code selects the line "Test paragraph to remove." So, let's summarize: a. this test doesn't have a correct description of what it does. b. what it claims is the expected result is not possible given the code. c. (from (2) this test does not dump the actual <textarea> content) With all these problems I think it is best that we skip this test and investigation and fixes for it take place in bug 199039

Daniel Bates

Comment 17 2019-06-24 12:32:48 PDT

Dang it. I forgot....there's one more thing 😀 remember those lines: <https://trac.webkit.org/browser/trunk/Source/WebKit/Shared/ios/InteractionInformationRequest.cpp?rev=246682#L72> and <https://trac.webkit.org/browser/trunk/Source/WebKit/Shared/ios/InteractionInformationRequest.cpp?rev=246682#L86>. ^^^ well, I never got the impression they were to support whatever it is this test does. They were added for a completely different purpose as part of maintaining the very hack I am removing in this bug! These conditionals were added in bug #195749, which has nothing to do about double tapping <---- oh boy, so that means the fix in bug #195749 is also suspect of being wrong OR at least it has an UNDOCUMENTED side effect; who knows if this side effect was accounted for

Daniel Bates

Comment 18 2019-06-24 12:46:48 PDT

Created attachment 372787 [details] To land

Daniel Bates

Comment 19 2019-06-24 12:47:37 PDT

Committed r246757: <https://trac.webkit.org/changeset/246757>

Wenson Hsieh

Comment 20 2019-06-24 13:25:58 PDT

(In reply to Daniel Bates from comment #17) > Dang it. I forgot....there's one more thing 😀 remember those lines: > > <https://trac.webkit.org/browser/trunk/Source/WebKit/Shared/ios/ > InteractionInformationRequest.cpp?rev=246682#L72> and > <https://trac.webkit.org/browser/trunk/Source/WebKit/Shared/ios/ > InteractionInformationRequest.cpp?rev=246682#L86>. > > ^^^ well, I never got the impression they were to support whatever it is > this test does. They were added for a completely different purpose as part > of maintaining the very hack I am removing in this bug! These conditionals > were added in bug #195749, which has nothing to do about double tapping > <---- oh boy, so that means the fix in bug #195749 is also suspect of being > wrong OR at least it has an UNDOCUMENTED side effect; who knows if this side > effect was accounted for It’s unclear to me why you believe that removing these early returns shouldn’t have any effect on the test. Some brief sleuthing reveals that this is because nodeAtPositionIsFocusedElement is false in the cached _positionInformation in the UI process without the early returns, whereas it would’ve otherwise been true with the returns. This makes sense, since we get this sequence of events: • Read-only hit-test happens when touch starts. No node is focused at this time. • The node is focused in the web process. • A non-readonly hit-test happens when selection is about to begin. The early return makes it so that we actually recompute position information, since we’re going from a readonly to non-readonly hit-test.

Wenson Hsieh

Comment 21 2019-06-24 14:03:31 PDT

(In reply to Wenson Hsieh from comment #20) > (In reply to Daniel Bates from comment #17) > > Dang it. I forgot....there's one more thing 😀 remember those lines: > > > > <https://trac.webkit.org/browser/trunk/Source/WebKit/Shared/ios/ > > InteractionInformationRequest.cpp?rev=246682#L72> and > > <https://trac.webkit.org/browser/trunk/Source/WebKit/Shared/ios/ > > InteractionInformationRequest.cpp?rev=246682#L86>. > > > > ^^^ well, I never got the impression they were to support whatever it is > > this test does. They were added for a completely different purpose as part > > of maintaining the very hack I am removing in this bug! These conditionals > > were added in bug #195749, which has nothing to do about double tapping > > <---- oh boy, so that means the fix in bug #195749 is also suspect of being > > wrong OR at least it has an UNDOCUMENTED side effect; who knows if this side > > effect was accounted for > > It’s unclear to me why you believe that removing these early returns > shouldn’t have any effect on the test. > > Some brief sleuthing reveals that this is because > nodeAtPositionIsFocusedElement is false in the cached _positionInformation > in the UI process without the early returns, whereas it would’ve otherwise > been true with the returns. This makes sense, since we get this sequence of > events: > > • Read-only hit-test happens when touch starts. No node is focused at this > time. > • The node is focused in the web process. > • A non-readonly hit-test happens when selection is about to begin. The > early return makes it so that we actually recompute position information, > since we’re going from a readonly to non-readonly hit-test. It seems that InteractionInformationRequest::isValidForRequest doesn’t take into account the scenario where nodeAtPositionIsFocusedElement may have changed. Perhaps we could rectify this by making it so that if we have the following sequence of events: 1. Interaction info is requested at (x, y) 2. An element is focused (and _elementDidFocus: is invoked in the UI process) 3. Interaction info is again requested at (x, y) …then the interaction info at (3) will not use the cached position info from (1).

Daniel Bates

Comment 22 2019-06-24 14:28:47 PDT

(In reply to Wenson Hsieh from comment #20) > (In reply to Daniel Bates from comment #17) > > Dang it. I forgot....there's one more thing 😀 remember those lines: > > > > <https://trac.webkit.org/browser/trunk/Source/WebKit/Shared/ios/ > > InteractionInformationRequest.cpp?rev=246682#L72> and > > <https://trac.webkit.org/browser/trunk/Source/WebKit/Shared/ios/ > > InteractionInformationRequest.cpp?rev=246682#L86>. > > > > ^^^ well, I never got the impression they were to support whatever it is > > this test does. They were added for a completely different purpose as part > > of maintaining the very hack I am removing in this bug! These conditionals > > were added in bug #195749, which has nothing to do about double tapping > > <---- oh boy, so that means the fix in bug #195749 is also suspect of being > > wrong OR at least it has an UNDOCUMENTED side effect; who knows if this side > > effect was accounted for Re-reading my words quoted in your reply it sounds a bit harsh. I didn't mean to come across the way. I just wanted to point out that it seems that bug #195749 had an unexpected side effect that was revealed by the removal of the conditionals. > > It’s unclear to me why you believe that removing these early returns > shouldn’t have any effect on the test. > That's not what I am trying to say, but maybe it came across that way. Oops.... > Some brief sleuthing reveals that this is because > nodeAtPositionIsFocusedElement is false in the cached _positionInformation > in the UI process without the early returns, whereas it would’ve otherwise > been true with the returns. This makes sense, since we get this sequence of > events: > > • Read-only hit-test happens when touch starts. No node is focused at this > time. > • The node is focused in the web process. > • A non-readonly hit-test happens when selection is about to begin. The > early return makes it so that we actually recompute position information, > since we’re going from a readonly to non-readonly hit-test. ^^^ yeah, I think this explain the issue I saw in (1) in comment #16. We talked in person. I think the fix is either bring back the readonly flag (renamed for its new purpose) OR unconditionally always update nodeAtPositionIsFocusedElement. At least those are the options we talked in person. I think this work should be done in bug #199039 along with fixing the issues I talked about in (2) and (3) in comment #16.

Daniel Bates

Comment 23 2019-06-24 14:30:06 PDT

(In reply to Wenson Hsieh from comment #21) > (In reply to Wenson Hsieh from comment #20) > > (In reply to Daniel Bates from comment #17) > > > Dang it. I forgot....there's one more thing 😀 remember those lines: > > > > > > <https://trac.webkit.org/browser/trunk/Source/WebKit/Shared/ios/ > > > InteractionInformationRequest.cpp?rev=246682#L72> and > > > <https://trac.webkit.org/browser/trunk/Source/WebKit/Shared/ios/ > > > InteractionInformationRequest.cpp?rev=246682#L86>. > > > > > > ^^^ well, I never got the impression they were to support whatever it is > > > this test does. They were added for a completely different purpose as part > > > of maintaining the very hack I am removing in this bug! These conditionals > > > were added in bug #195749, which has nothing to do about double tapping > > > <---- oh boy, so that means the fix in bug #195749 is also suspect of being > > > wrong OR at least it has an UNDOCUMENTED side effect; who knows if this side > > > effect was accounted for > > > > It’s unclear to me why you believe that removing these early returns > > shouldn’t have any effect on the test. > > > > Some brief sleuthing reveals that this is because > > nodeAtPositionIsFocusedElement is false in the cached _positionInformation > > in the UI process without the early returns, whereas it would’ve otherwise > > been true with the returns. This makes sense, since we get this sequence of > > events: > > > > • Read-only hit-test happens when touch starts. No node is focused at this > > time. > > • The node is focused in the web process. > > • A non-readonly hit-test happens when selection is about to begin. The > > early return makes it so that we actually recompute position information, > > since we’re going from a readonly to non-readonly hit-test. > > It seems that InteractionInformationRequest::isValidForRequest doesn’t take > into account the scenario where nodeAtPositionIsFocusedElement may have > changed. Perhaps we could rectify this by making it so that if we have the > following sequence of events: > > 1. Interaction info is requested at (x, y) > 2. An element is focused (and _elementDidFocus: is invoked in the UI process) > 3. Interaction info is again requested at (x, y) > > …then the interaction info at (3) will not use the cached position info from > (1). Yep, yep, yep... Let's use bug #199039 to fix this.

Note You need to log in before you can comment on or make changes to this bug.