Bug 60010 – [Chromium]Add clear_autofill flag to hide autofill popup without clear field.

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[Naoki Takano]

[Chromium]Add clear_autofill flag to hide autofill popup without clear field.

[Naoki Takano]

Created attachment 92047 [details]
Patch

[Ilya Sherman]

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.

[Naoki Takano]

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.

[Naoki Takano]

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.

[Ilya Sherman]

(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

[Ilya Sherman]

(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.

[Naoki Takano]

(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?

[Naoki Takano]

(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...

[Ilya Sherman]

(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() ?

[Naoki Takano]

> (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,

[Naoki Takano]

Created attachment 92677 [details]
Patch

[Naoki Takano]

Comment on attachment 92677 [details]
Patch

According to Ilya's suggestion, I added the function to accept autofilled field.

Please review.

[Ilya Sherman]

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() """ ?

[Naoki Takano]

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() """ ?

[Ilya Sherman]

(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?

[Ilya Sherman]

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?

[Naoki Takano]

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?

[Naoki Takano]

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?

[Dimitri Glazkov (Google)]

Maciej is probably the completely wrong guy for this question. Daniel perhaps?

[Daniel Cheng]

Sorry, this is out of my area of knowledge.

[Naoki Takano]

Hmmm....

Is there any other person in mind?

(In reply to comment #20)
> Sorry, this is out of my area of knowledge.

[Tony Chang]

(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.

[Naoki Takano]

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.

[Naoki Takano]

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.

[Tony Chang]

(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.

[Naoki Takano]

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.

[Eric Seidel (no email)]

It's been 8 months since this was posted.  Do we really need this fix?

[Ilya Sherman]

(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.

[Eric Seidel (no email)]

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).

[Darin Fisher (:fishd, Google)]

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.