[Chromium]Add clear_autofill flag to hide autofill popup without clear field.
Created attachment 92047 [details] Patch
This doesn't seem like the right approach to me, because we never want to have suggested values in the form fields when the popup is closed. A drag action should accept the suggested values. Once the suggested values are accepted, closing the popup should work normally, without any further changes.
Ilya, Thank you for your comment. I want to make sure what "Accept" status. As you know, when user changes the selection, didAcceptAutoFillSuggestion() is called. I guess this is the already "Accept" status for the field, right? But after the "Accept" status, user select all string in the field while showing popup window, then starts dragging. In that situation, even if the field is after "Accept", the field string is gone. That is a problem. Or do you mean different "Accept"? Thanks, (In reply to comment #2) > This doesn't seem like the right approach to me, because we never want to have suggested values in the form fields when the popup is closed. A drag action should accept the suggested values. Once the suggested values are accepted, closing the popup should work normally, without any further changes.
If you say "Accept" means choosing item in autofill popup with keyboard, for example, the following function is called. 1061 void PopupListBox::acceptIndex(int index) 1062 { 1063 // Clear m_acceptedIndexOnAbandon once user accepts the selected index. 1064 if (m_acceptedIndexOnAbandon >= 0) 1065 m_acceptedIndexOnAbandon = -1; 1066 1067 if (index >= numItems()) 1068 return; 1069 1070 if (index < 0) { 1071 if (m_popupClient) { 1072 // Enter pressed with no selection, just close the popup. 1073 hidePopup(); 1074 } 1075 return; 1076 } 1077 1078 if (isSelectableItem(index)) { 1079 RefPtr<PopupListBox> keepAlive(this); 1080 1081 // Hide ourselves first since valueChanged may have numerous side-effects. 1082 hidePopup(); 1083 1084 // Tell the <select> PopupMenuClient what index was selected. 1085 m_popupClient->valueChanged(index); 1086 } 1087 } But, as you know, hidePopup() is the same function which I try to call from startDragging and then the function calls m_popupClient->valueChanged(index); here. Actually valueChange() calls didAcceptAutoFillSuggestion() internally, if we follow the same way as this, we have to call didAcceptAutoFillSuggestion(). But didAcceptAutoFillSuggestion() needs webnode information which is difficult to access outside of m_popupClient. Maybe we can make a special function to accept current index, but it it very long path to access from WebView... What do you think? Thanks, (In reply to comment #3) > Ilya, > > Thank you for your comment. > > I want to make sure what "Accept" status. > > As you know, when user changes the selection, didAcceptAutoFillSuggestion() is called. I guess this is the already "Accept" status for the field, right? > > But after the "Accept" status, user select all string in the field while showing popup window, then starts dragging. > > In that situation, even if the field is after "Accept", the field string is gone. > > That is a problem. > > Or do you mean different "Accept"? > > Thanks, > > > (In reply to comment #2) > > This doesn't seem like the right approach to me, because we never want to have suggested values in the form fields when the popup is closed. A drag action should accept the suggested values. Once the suggested values are accepted, closing the popup should work normally, without any further changes.
(In reply to comment #3) > Ilya, > > Thank you for your comment. > > I want to make sure what "Accept" status. > > As you know, when user changes the selection, didAcceptAutoFillSuggestion() is called. I guess this is the already "Accept" status for the field, right? This is the right method to call into. If this is called prior to the drag, then there shouldn't be a need to do anything extra to make sure that the text remains. When I tested locally, I did not see this method being called. However, I did notice that clicking&dragging within the field while the popup is open causes us to switch from Autofill suggestions to single-field Autocomplete suggestions. The form remains in the previewed state -- with all of the autofillable fields showing a preview of what would be filled into them -- but there is no way to accept this Autofill suggestion. This is a bug, and something we should probably fix before tackling http://code.google.com/p/chromium/issues/detail?id=80817
(In reply to comment #4) > If you say "Accept" means choosing item in autofill popup with keyboard, for example, the following function is called. > > 1061 void PopupListBox::acceptIndex(int index) > 1062 { > 1063 // Clear m_acceptedIndexOnAbandon once user accepts the selected index. > 1064 if (m_acceptedIndexOnAbandon >= 0) > 1065 m_acceptedIndexOnAbandon = -1; > 1066 > 1067 if (index >= numItems()) > 1068 return; > 1069 > 1070 if (index < 0) { > 1071 if (m_popupClient) { > 1072 // Enter pressed with no selection, just close the popup. > 1073 hidePopup(); > 1074 } > 1075 return; > 1076 } > 1077 > 1078 if (isSelectableItem(index)) { > 1079 RefPtr<PopupListBox> keepAlive(this); > 1080 > 1081 // Hide ourselves first since valueChanged may have numerous side-effects. > 1082 hidePopup(); > 1083 > 1084 // Tell the <select> PopupMenuClient what index was selected. > 1085 m_popupClient->valueChanged(index); > 1086 } > 1087 } > > But, as you know, hidePopup() is the same function which I try to call from startDragging and then the function calls m_popupClient->valueChanged(index); here. > > Actually valueChange() calls didAcceptAutoFillSuggestion() internally, if we follow the same way as this, we have to call didAcceptAutoFillSuggestion(). > > But didAcceptAutoFillSuggestion() needs webnode information which is difficult to access outside of m_popupClient. > > Maybe we can make a special function to accept current index, but it it very long path to access from WebView... This (accepting the current index when a drag is initiated) is probably the right solution -- at least in terms of the functionality it implies. I'm not sure if there is perhaps an easier way to write the code.
(In reply to comment #5) > When I tested locally, I did not see this method being called. However, I did notice that clicking&dragging within the field while the popup is open causes us to switch from Autofill suggestions to single-field Autocomplete suggestions. The form remains in the previewed state -- with all of the autofillable fields showing a preview of what would be filled into them -- but there is no way to accept this Autofill suggestion. This is a bug, and something we should probably fix before tackling http://code.google.com/p/chromium/issues/detail?id=80817 So you fixed it at http://codereview.chromium.org/6915003/ did you?
(In reply to comment #6) > This (accepting the current index when a drag is initiated) is probably the right solution -- at least in terms of the functionality it implies. I'm not sure if there is perhaps an easier way to write the code. Hmmm... As you know the function is not exposed to WebView, and the autofill popup window is abstracted as just a popup window like HTML select element popup. Of course, I can add an extra function like acceptCurrectAutoFilled(), but it is too specific to expose the method as a popup menu interface, right? The fundamental problem is autofill popup is mixed up with other popup windows. We should split it apart from the implementation;-( Anyway I'm looking around the source code again if there is a much easier solution or not...
(In reply to comment #7) > (In reply to comment #5) > > > When I tested locally, I did not see this method being called. However, I did notice that clicking&dragging within the field while the popup is open causes us to switch from Autofill suggestions to single-field Autocomplete suggestions. The form remains in the previewed state -- with all of the autofillable fields showing a preview of what would be filled into them -- but there is no way to accept this Autofill suggestion. This is a bug, and something we should probably fix before tackling http://code.google.com/p/chromium/issues/detail?id=80817 > > So you fixed it at > http://codereview.chromium.org/6915003/ > did you? Yes indeed. It turned out to be a rather straightforward fix. (In reply to comment #8) > (In reply to comment #6) > > This (accepting the current index when a drag is initiated) is probably the right solution -- at least in terms of the functionality it implies. I'm not sure if there is perhaps an easier way to write the code. > Hmmm... > > As you know the function is not exposed to WebView, and the autofill popup window is abstracted as just a popup window like HTML select element popup. > > Of course, I can add an extra function like acceptCurrectAutoFilled(), but it is too specific to expose the method as a popup menu interface, right? Would something go wrong if we were to add a more general method: acceptSelectedPopupItem() ?
> (In reply to comment #8) > > (In reply to comment #6) > > > This (accepting the current index when a drag is initiated) is probably the right solution -- at least in terms of the functionality it implies. I'm not sure if there is perhaps an easier way to write the code. > > Hmmm... > > > > As you know the function is not exposed to WebView, and the autofill popup window is abstracted as just a popup window like HTML select element popup. > > > > Of course, I can add an extra function like acceptCurrectAutoFilled(), but it is too specific to expose the method as a popup menu interface, right? > > Would something go wrong if we were to add a more general method: acceptSelectedPopupItem() ? I see. Still I don't know how I should do for <select> popup window though, I'll look into that. Thanks,
Created attachment 92677 [details] Patch
Comment on attachment 92677 [details] Patch According to Ilya's suggestion, I added the function to accept autofilled field. Please review.
Comment on attachment 92677 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=92677&action=review This looks ok to me, but should definitely get looked at by one of the WebKit API guardians. > Source/WebCore/ChangeLog:5 > + [Chromium]Add clear_autofill flag to hide autofill popup without clear field. nit: This description is out of sync with the code changes. > Source/WebKit/chromium/public/WebView.h:301 > + virtual void acceptCurrectAutoFilled() = 0; nit: Is there any reason not to name this more generally, e.g. """ acceptSelectedPopupItem() """ ?
Ilya, Thank you for your review. But, as I wrote in Chromium LC, there might have some problem. So the current flow is 1, Autofill overwrap the field and show the completion. Here the node color is yellow. 2, The user selects the range in the yellow field. 3, The user start dragging. 4, Accept the completion and the yellow field is gone. 5, The user drops the text to another field. 6, But the original field stays the accepted text. (Is this Ok???) The reason why I ask you about 6 is this dragged text should be gone from the original filed. But still it stays as is. Maybe when staring dragging, we have to replace the dragged node from original yellow field to white accepted field, don't we? If so, we have to do more tweaking when starting dragging... Thanks, (In reply to comment #13) > (From update of attachment 92677 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=92677&action=review > > This looks ok to me, but should definitely get looked at by one of the WebKit API guardians. > > > Source/WebCore/ChangeLog:5 > > + [Chromium]Add clear_autofill flag to hide autofill popup without clear field. > > nit: This description is out of sync with the code changes. > > > Source/WebKit/chromium/public/WebView.h:301 > > + virtual void acceptCurrectAutoFilled() = 0; > > nit: Is there any reason not to name this more generally, e.g. """ acceptSelectedPopupItem() """ ?
(In reply to comment #14) > So the current flow is > 1, Autofill overwrap the field and show the completion. Here the node color is yellow. > 2, The user selects the range in the yellow field. > 3, The user start dragging. > 4, Accept the completion and the yellow field is gone. > 5, The user drops the text to another field. > 6, But the original field stays the accepted text. (Is this Ok???) > > The reason why I ask you about 6 is this dragged text should be gone from the original filed. But still it stays as is. > > Maybe when staring dragging, we have to replace the dragged node from original yellow field to white accepted field, don't we? We should try to make this work exactly the same way it would if the user were to first autofill the form, then drag the filled text. Could you please clarify why this code coupled with the Chromium change does not result in this desired behavior?
Maciej, the code revision history tells me you might be familiar with the drag&drop code. Could you weigh in on this patch and the related Chromium one, http://codereview.chromium.org/6902196/ ? Here's a quick summary of the problem, as I understand it: (1) We initiate a drag event for "suggested" (uncommitted) Autofill text in a text field. (2) This drag should cause the text to be committed. (3) When the text is committed, the "suggested" text is cleared; but it is still referenced in the drag state. (4) When the corresponding drop event occurs, we hit an assertion because the referenced selection is orphaned. Do you have any suggestions on how to handle this somewhat unusual situation?
Maciej, Could you give us your comment or thought? Thanks, (In reply to comment #16) > Maciej, the code revision history tells me you might be familiar with the drag&drop code. Could you weigh in on this patch and the related Chromium one, http://codereview.chromium.org/6902196/ ? > > Here's a quick summary of the problem, as I understand it: > (1) We initiate a drag event for "suggested" (uncommitted) Autofill text in a text field. > (2) This drag should cause the text to be committed. > (3) When the text is committed, the "suggested" text is cleared; but it is still referenced in the drag state. > (4) When the corresponding drop event occurs, we hit an assertion because the referenced selection is orphaned. > > Do you have any suggestions on how to handle this somewhat unusual situation?
Is there anybody who can comment this problem? (In reply to comment #17) > Maciej, > > Could you give us your comment or thought? > > Thanks, > > (In reply to comment #16) > > Maciej, the code revision history tells me you might be familiar with the drag&drop code. Could you weigh in on this patch and the related Chromium one, http://codereview.chromium.org/6902196/ ? > > > > Here's a quick summary of the problem, as I understand it: > > (1) We initiate a drag event for "suggested" (uncommitted) Autofill text in a text field. > > (2) This drag should cause the text to be committed. > > (3) When the text is committed, the "suggested" text is cleared; but it is still referenced in the drag state. > > (4) When the corresponding drop event occurs, we hit an assertion because the referenced selection is orphaned. > > > > Do you have any suggestions on how to handle this somewhat unusual situation?
Maciej is probably the completely wrong guy for this question. Daniel perhaps?
Sorry, this is out of my area of knowledge.
Hmmm.... Is there any other person in mind? (In reply to comment #20) > Sorry, this is out of my area of knowledge.
(In reply to comment #16) > Maciej, the code revision history tells me you might be familiar with the drag&drop code. Could you weigh in on this patch and the related Chromium one, http://codereview.chromium.org/6902196/ ? > > Here's a quick summary of the problem, as I understand it: > (1) We initiate a drag event for "suggested" (uncommitted) Autofill text in a text field. > (2) This drag should cause the text to be committed. > (3) When the text is committed, the "suggested" text is cleared; but it is still referenced in the drag state. > (4) When the corresponding drop event occurs, we hit an assertion because the referenced selection is orphaned. > > Do you have any suggestions on how to handle this somewhat unusual situation? Is this problem specific to Autofill? For example, if I select and drag text and a dom event fires (e.g., ondragenter) and removes the text I'm dragging, do we hit the same assertion? If so, this might be a bug that needs to be fixed separately.
Ok, I'll make sure with some test code... (In reply to comment #22) > (In reply to comment #16) > > Maciej, the code revision history tells me you might be familiar with the drag&drop code. Could you weigh in on this patch and the related Chromium one, http://codereview.chromium.org/6902196/ ? > > > > Here's a quick summary of the problem, as I understand it: > > (1) We initiate a drag event for "suggested" (uncommitted) Autofill text in a text field. > > (2) This drag should cause the text to be committed. > > (3) When the text is committed, the "suggested" text is cleared; but it is still referenced in the drag state. > > (4) When the corresponding drop event occurs, we hit an assertion because the referenced selection is orphaned. > > > > Do you have any suggestions on how to handle this somewhat unusual situation? > > Is this problem specific to Autofill? For example, if I select and drag text and a dom event fires (e.g., ondragenter) and removes the text I'm dragging, do we hit the same assertion? > > If so, this might be a bug that needs to be fixed separately.
Tony, this is not autofill specific problem. It happens with your suggested test pattern. Should I make another bug entry? Thanks, (In reply to comment #23) > Ok, I'll make sure with some test code... > > (In reply to comment #22) > > (In reply to comment #16) > > > Maciej, the code revision history tells me you might be familiar with the drag&drop code. Could you weigh in on this patch and the related Chromium one, http://codereview.chromium.org/6902196/ ? > > > > > > Here's a quick summary of the problem, as I understand it: > > > (1) We initiate a drag event for "suggested" (uncommitted) Autofill text in a text field. > > > (2) This drag should cause the text to be committed. > > > (3) When the text is committed, the "suggested" text is cleared; but it is still referenced in the drag state. > > > (4) When the corresponding drop event occurs, we hit an assertion because the referenced selection is orphaned. > > > > > > Do you have any suggestions on how to handle this somewhat unusual situation? > > > > Is this problem specific to Autofill? For example, if I select and drag text and a dom event fires (e.g., ondragenter) and removes the text I'm dragging, do we hit the same assertion? > > > > If so, this might be a bug that needs to be fixed separately.
(In reply to comment #24) > Tony, this is not autofill specific problem. It happens with your suggested test pattern. > > Should I make another bug entry? Yes, a new bug for the drag assert would be good. Do we crash in release builds? I think it's fine to get this patch reviewed and landed since the bug you describe is not specific to this patch.
I filed the issue as https://bugs.webkit.org/show_bug.cgi?id=61008 (In reply to comment #25) > (In reply to comment #24) > > Tony, this is not autofill specific problem. It happens with your suggested test pattern. > > > > Should I make another bug entry? > > Yes, a new bug for the drag assert would be good. Do we crash in release builds? > > I think it's fine to get this patch reviewed and landed since the bug you describe is not specific to this patch.
It's been 8 months since this was posted. Do we really need this fix?
(In reply to comment #27) > It's been 8 months since this was posted. Do we really need this fix? We're currently working to move this UI out of WebKit, so this change should not be necessary within WebKit.
Comment on attachment 92677 [details] Patch Cleared review? from attachment 92677 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).
Comment on attachment 92677 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=92677&action=review >> Source/WebKit/chromium/public/WebView.h:301 >> + virtual void acceptCurrectAutoFilled() = 0; > > nit: Is there any reason not to name this more generally, e.g. """ acceptSelectedPopupItem() """ ? Agreed, the name is wrong. At the very least, it seems like you probably meant to write "Current" instead of "Currect", but I like Ilya's suggestion.