Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6902196:
When start dragging event is detected, call hidePopups() to hide all popup windows. (Closed)
|
Created:
9 years, 7 months ago by honten Modified:
9 years, 2 months ago CC:
chromium-reviews, jam Visibility:
Public. |
DescriptionWhen start dragging event is detected, call hidePopups() to hide all popup windows.
When hide autofill popup window, we should not clear autofilled text. The patch also needs WebKit side change. Please refer to
https://bugs.webkit.org/show_bug.cgi?id=60010
BUG=80817
TEST= 1.open the page with multi fields (name,surname,email,confirm email, email pro...) 2.autofill the name fields 3.select (to move/copy) name field 4.start drag'n'drop the name field and autofill popup windows should be dismissed.
Patch Set 1 #Patch Set 2 : Fix assert problem. #Patch Set 3 : Change to use acceptCurrentAutoFilled(). #
Total comments: 3
Messages
Total messages: 19 (0 generated)
Please review it. Just I added two lines to dismiss autofill popup window once drag'n'drop starts. Thanks,
I'm seeing an assert fire and what appears to be a race in the drag-n-drop code:
ASSERT(endingSelection().isNonOrphanedRange());
#0 0x0225bddd in WebCore::MoveSelectionCommand::doApply at
MoveSelectionCommand.cpp:42
#1 0x0222c62b in WebCore::EditCommand::apply at EditCommand.cpp:92
#2 0x0222c6db in WebCore::applyCommand at EditCommand.cpp:229
#3 0x023ce0e3 in WebCore::DragController::concludeEditDrag at
DragController.cpp:467
#4 0x023cf053 in WebCore::DragController::performDrag at DragController.cpp:206
#5 0x01a6aa35 in WebKit::WebViewImpl::dragTargetDrop at WebViewImpl.cpp:1937
#6 0x02f63aad in RenderView::OnDragTargetDrop at render_view.cc:3327
It appears that the closing of the menu causes the selection range (drag source)
to become invalid.
On 2011/05/01 20:17:40, honten wrote:
> Please review it.
>
> Just I added two lines to dismiss autofill popup window once drag'n'drop
starts.
>
> Thanks,
I'll make sure with debug binary... On Mon, May 2, 2011 at 9:19 AM, <dhollowa@chromium.org> wrote: > I'm seeing an assert fire and what appears to be a race in the drag-n-drop > code: > > ASSERT(endingSelection().isNonOrphanedRange()); > #0 0x0225bddd in WebCore::MoveSelectionCommand::doApply at > MoveSelectionCommand.cpp:42 > #1 0x0222c62b in WebCore::EditCommand::apply at EditCommand.cpp:92 > #2 0x0222c6db in WebCore::applyCommand at EditCommand.cpp:229 > #3 0x023ce0e3 in WebCore::DragController::concludeEditDrag at > DragController.cpp:467 > #4 0x023cf053 in WebCore::DragController::performDrag at > DragController.cpp:206 > #5 0x01a6aa35 in WebKit::WebViewImpl::dragTargetDrop at > WebViewImpl.cpp:1937 > #6 0x02f63aad in RenderView::OnDragTargetDrop at render_view.cc:3327 > > It appears that the closing of the menu causes the selection range (drag > source) > to become invalid. > > > On 2011/05/01 20:17:40, honten wrote: >> >> Please review it. > >> Just I added two lines to dismiss autofill popup window once drag'n'drop > > starts. > >> Thanks, > > > > http://codereview.chromium.org/6902196/ >
The reason why the assertion failed was the autofilled field was cleared when dragging started. On if we start dragging, we should not clear autofiled field. Please review the pactch and also see webkit patch. Thanks,
On 2011/05/03 05:40:20, honten wrote: > The reason why the assertion failed was the autofilled field was cleared when > dragging started. > > On if we start dragging, we should not clear autofiled field. As noted in the WebKit patch, we should accept the suggested values before closing the popup. Then, everything should work fine.
Ilya, I also commented in bugzilla. Could you take a look? Thanks, On Mon, May 2, 2011 at 10:45 PM, <isherman@chromium.org> wrote: > On 2011/05/03 05:40:20, honten wrote: >> >> The reason why the assertion failed was the autofilled field was cleared >> when >> dragging started. > >> On if we start dragging, we should not clear autofiled field. > > As noted in the WebKit patch, we should accept the suggested values before > closing the popup. Then, everything should work fine. > > http://codereview.chromium.org/6902196/ >
Please review. Also I updated WebKit patch. Thanks, On 2011/05/03 06:37:22, takano.naoki_gmail.com wrote: > Ilya, > > I also commented in bugzilla. > > Could you take a look? > > Thanks, > > On Mon, May 2, 2011 at 10:45 PM, <mailto:isherman@chromium.org> wrote: > > On 2011/05/03 05:40:20, honten wrote: > >> > >> The reason why the assertion failed was the autofilled field was cleared > >> when > >> dragging started. > > > >> On if we start dragging, we should not clear autofiled field. > > > > As noted in the WebKit patch, we should accept the suggested values before > > closing the popup. Then, everything should work fine. > > > > http://codereview.chromium.org/6902196/ > >
Oops, still there is problem in debug version... I'll look into it... On 2011/05/07 02:17:58, honten wrote: > Please review. > > Also I updated WebKit patch. > > Thanks, > > On 2011/05/03 06:37:22, http://takano.naoki_gmail.com wrote: > > Ilya, > > > > I also commented in bugzilla. > > > > Could you take a look? > > > > Thanks, > > > > On Mon, May 2, 2011 at 10:45 PM, <mailto:isherman@chromium.org> wrote: > > > On 2011/05/03 05:40:20, honten wrote: > > >> > > >> The reason why the assertion failed was the autofilled field was cleared > > >> when > > >> dragging started. > > > > > >> On if we start dragging, we should not clear autofiled field. > > > > > > As noted in the WebKit patch, we should accept the suggested values before > > > closing the popup. Then, everything should work fine. > > > > > > http://codereview.chromium.org/6902196/ > > >
http://codereview.chromium.org/6902196/diff/9001/content/renderer/render_view.cc File content/renderer/render_view.cc (right): http://codereview.chromium.org/6902196/diff/9001/content/renderer/render_view... content/renderer/render_view.cc:1671: webview()->acceptCurrectAutoFilled(); You probably want to swap the order of these two lines.
http://codereview.chromium.org/6902196/diff/9001/content/renderer/render_view.cc File content/renderer/render_view.cc (right): http://codereview.chromium.org/6902196/diff/9001/content/renderer/render_view... content/renderer/render_view.cc:1671: webview()->acceptCurrectAutoFilled(); Hmmm... Even if I change the order of the calling, the debug assertion problem is not figured out. The problem is the following, ASSERT(endingSelection().isNonOrphanedRange()); As you know, once we hide popup, the selected node is gone even if it's accepted. Because the selection is the yellow color node displayed by Autofill and it's gone after the accept. So that is the problem. Actually, if we accept the node, we don't have to care orphaned. So only in this case, we should not check the assertion. But to be strict behavior and to check other cases, we have to pass the accepted info with flag not to check assertion, right? What do you think? On 2011/05/07 09:07:10, Ilya Sherman wrote: > You probably want to swap the order of these two lines.
http://codereview.chromium.org/6902196/diff/9001/content/renderer/render_view.cc File content/renderer/render_view.cc (right): http://codereview.chromium.org/6902196/diff/9001/content/renderer/render_view... content/renderer/render_view.cc:1671: webview()->acceptCurrectAutoFilled(); On 2011/05/09 02:30:32, honten wrote: > Hmmm... > > Even if I change the order of the calling, the debug assertion problem is not > figured out. > > The problem is the following, > ASSERT(endingSelection().isNonOrphanedRange()); > > As you know, once we hide popup, the selected node is gone even if it's > accepted. Because the selection is the yellow color node displayed by Autofill > and it's gone after the accept. So that is the problem. > > Actually, if we accept the node, we don't have to care orphaned. So only in this > case, we should not check the assertion. But to be strict behavior and to check > other cases, we have to pass the accepted info with flag not to check assertion, > right? > > What do you think? As I mentioned on the WebKit bug, I don't fully understand why this code does not behave the same as the code that handles the sequence of events (1) User clicks to fill the form, thereby dismissing the popup menu. (2) User drags text from a filled field to another field. Ideally, we should behave exactly as that sequence of events does -- and share code with it as much as possible. What additional changes do we need to make to achieve this matching behavior?
Now I know why we encounter assertion error. If hidePopup() is called, AutofillPopupWindow clears all autlfilled fields. So it means the range selection in the field is gone. That is the reason isOrphaned() is true. To prevent that, we can add a flag parameter in hidePopup() not to clear the fields and just close the popup window. But it is different behavior from the usual, and it might give some side effect. Right now, whenever the field is accepted, hidePopup() is called first, then valueChaneged() fills the field again. Do you have any idea, Ilya? Thanks On 2011/05/09 07:40:05, Ilya Sherman wrote: > http://codereview.chromium.org/6902196/diff/9001/content/renderer/render_view.cc > File content/renderer/render_view.cc (right): > > http://codereview.chromium.org/6902196/diff/9001/content/renderer/render_view... > content/renderer/render_view.cc:1671: webview()->acceptCurrectAutoFilled(); > On 2011/05/09 02:30:32, honten wrote: > > Hmmm... > > > > Even if I change the order of the calling, the debug assertion problem is not > > figured out. > > > > The problem is the following, > > ASSERT(endingSelection().isNonOrphanedRange()); > > > > As you know, once we hide popup, the selected node is gone even if it's > > accepted. Because the selection is the yellow color node displayed by Autofill > > and it's gone after the accept. So that is the problem. > > > > Actually, if we accept the node, we don't have to care orphaned. So only in > this > > case, we should not check the assertion. But to be strict behavior and to > check > > other cases, we have to pass the accepted info with flag not to check > assertion, > > right? > > > > What do you think? > > As I mentioned on the WebKit bug, I don't fully understand why this code does > not behave the same as the code that handles the sequence of events > > (1) User clicks to fill the form, thereby dismissing the popup menu. > (2) User drags text from a filled field to another field. > > Ideally, we should behave exactly as that sequence of events does -- and share > code with it as much as possible. What additional changes do we need to make to > achieve this matching behavior?
On 2011/05/12 19:53:06, honten wrote: > Now I know why we encounter assertion error. > > If hidePopup() is called, AutofillPopupWindow clears all autlfilled fields. So > it means the range selection in the field is gone. That is the reason > isOrphaned() is true. > > To prevent that, we can add a flag parameter in hidePopup() not to clear the > fields and just close the popup window. But it is different behavior from the > usual, and it might give some side effect. > > Right now, whenever the field is accepted, hidePopup() is called first, then > valueChaneged() fills the field again. > > Do you have any idea, Ilya? Ah, that makes sense, I think. Thanks for the explanation. I'm not sure how exactly to approach this, but I've added a comment to the WebKit bug looping in someone who hopefully does.
Thank you for your comment in Webkit bugzilla , Ilya. But still I wonder why we need to call clear whenever accept the suggested results. For example, select tag just leaves the result when hidePopup() is called. On Thu, May 12, 2011 at 5:22 PM, <isherman@chromium.org> wrote: > On 2011/05/12 19:53:06, honten wrote: >> >> Now I know why we encounter assertion error. > >> If hidePopup() is called, AutofillPopupWindow clears all autlfilled >> fields. So >> it means the range selection in the field is gone. That is the reason >> isOrphaned() is true. > >> To prevent that, we can add a flag parameter in hidePopup() not to clear >> the >> fields and just close the popup window. But it is different behavior from >> the >> usual, and it might give some side effect. > >> Right now, whenever the field is accepted, hidePopup() is called first, >> then >> valueChaneged() fills the field again. > >> Do you have any idea, Ilya? > > Ah, that makes sense, I think. Thanks for the explanation. I'm not sure > how > exactly to approach this, but I've added a comment to the WebKit bug looping > in > someone who hopefully does. > > http://codereview.chromium.org/6902196/ >
On 2011/05/13 00:33:16, takano.naoki_gmail.com wrote: > Thank you for your comment in Webkit bugzilla , Ilya. > > But still I wonder why we need to call clear whenever accept the > suggested results. > For example, select tag just leaves the result when hidePopup() is called. For Autofill, there are two types of values that text fields can have. When the popup menu is open and you hover over the suggestions, the fields have "suggested" values, which are displayed to the user, but not visible to JavaScript. When the user selects a suggestion, we want to commit these suggestions, so that they are also visible to JavaScript. Currently, we do this by first clearing all of the suggested values, and then subsequently setting all of the actual values. We don't have to do these two steps separately, but in the end we want each field to have an actual value set and no suggested value set.
Ah-ha!! I got it. Thank you for your explanation. On Thu, May 12, 2011 at 6:07 PM, <isherman@chromium.org> wrote: > On 2011/05/13 00:33:16, takano.naoki_gmail.com wrote: >> >> Thank you for your comment in Webkit bugzilla , Ilya. > >> But still I wonder why we need to call clear whenever accept the >> suggested results. >> For example, select tag just leaves the result when hidePopup() is called. > > For Autofill, there are two types of values that text fields can have. When > the > popup menu is open and you hover over the suggestions, the fields have > "suggested" values, which are displayed to the user, but not visible to > JavaScript. When the user selects a suggestion, we want to commit these > suggestions, so that they are also visible to JavaScript. Currently, we do > this > by first clearing all of the suggested values, and then subsequently setting > all > of the actual values. We don't have to do these two steps separately, but > in > the end we want each field to have an actual value set and no suggested > value > set. > > http://codereview.chromium.org/6902196/ >
After the discussion in WebKig bugzilla, the assertion bug is not only specific for autofill, but more general drag&drop bug in WebKit. Tony suggested we can review and land, but the behavior might be changed if we don't fix the drag&drop bug. So should I fix the drag&drop bug first? Or can I continue fixing this bug and land it? Thanks, On 2011/05/13 01:30:05, takano.naoki_gmail.com wrote: > Ah-ha!! > > I got it. Thank you for your explanation. > > On Thu, May 12, 2011 at 6:07 PM, <mailto:isherman@chromium.org> wrote: > > On 2011/05/13 00:33:16, http://takano.naoki_gmail.com wrote: > >> > >> Thank you for your comment in Webkit bugzilla , Ilya. > > > >> But still I wonder why we need to call clear whenever accept the > >> suggested results. > >> For example, select tag just leaves the result when hidePopup() is called. > > > > For Autofill, there are two types of values that text fields can have. When > > the > > popup menu is open and you hover over the suggestions, the fields have > > "suggested" values, which are displayed to the user, but not visible to > > JavaScript. When the user selects a suggestion, we want to commit these > > suggestions, so that they are also visible to JavaScript. Currently, we do > > this > > by first clearing all of the suggested values, and then subsequently setting > > all > > of the actual values. We don't have to do these two steps separately, but > > in > > the end we want each field to have an actual value set and no suggested > > value > > set. > > > > http://codereview.chromium.org/6902196/ > >
It sounds like the correct path is to fix the underlying drag&drop bug first. On 2011/05/17 17:04:29, honten wrote: > After the discussion in WebKig bugzilla, the assertion bug is not only specific > for autofill, but more general drag&drop bug in WebKit. > > Tony suggested we can review and land, but the behavior might be changed if we > don't fix the drag&drop bug. > > So should I fix the drag&drop bug first? > > Or can I continue fixing this bug and land it? > > Thanks, > > On 2011/05/13 01:30:05, http://takano.naoki_gmail.com wrote: > > Ah-ha!! > > > > I got it. Thank you for your explanation. > > > > On Thu, May 12, 2011 at 6:07 PM, <mailto:isherman@chromium.org> wrote: > > > On 2011/05/13 00:33:16, http://takano.naoki_gmail.com wrote: > > >> > > >> Thank you for your comment in Webkit bugzilla , Ilya. > > > > > >> But still I wonder why we need to call clear whenever accept the > > >> suggested results. > > >> For example, select tag just leaves the result when hidePopup() is called. > > > > > > For Autofill, there are two types of values that text fields can have. > When > > > the > > > popup menu is open and you hover over the suggestions, the fields have > > > "suggested" values, which are displayed to the user, but not visible to > > > JavaScript. When the user selects a suggestion, we want to commit > these > > > suggestions, so that they are also visible to JavaScript. Currently, > we do > > > this > > > by first clearing all of the suggested values, and then subsequently setting > > > all > > > of the actual values. We don't have to do these two steps separately, > but > > > in > > > the end we want each field to have an actual value set and no suggested > > > value > > > set. > > > > > > http://codereview.chromium.org/6902196/ > > >
I try it. But I've never fixed Drag&Drop related bug, so it might take a while. Thanks, On Tue, May 17, 2011 at 12:55 PM, <dhollowa@chromium.org> wrote: > > It sounds like the correct path is to fix the underlying drag&drop bug > first. > > On 2011/05/17 17:04:29, honten wrote: >> >> After the discussion in WebKig bugzilla, the assertion bug is not only > > specific >> >> for autofill, but more general drag&drop bug in WebKit. > >> Tony suggested we can review and land, but the behavior might be changed >> if we >> don't fix the drag&drop bug. > >> So should I fix the drag&drop bug first? > >> Or can I continue fixing this bug and land it? > >> Thanks, > >> On 2011/05/13 01:30:05, http://takano.naoki_gmail.com wrote: >> > Ah-ha!! >> > >> > I got it. Thank you for your explanation. >> > >> > On Thu, May 12, 2011 at 6:07 PM, <mailto:isherman@chromium.org> wrote: >> > > On 2011/05/13 00:33:16, http://takano.naoki_gmail.com wrote: >> > >> >> > >> Thank you for your comment in Webkit bugzilla , Ilya. >> > > >> > >> But still I wonder why we need to call clear whenever accept the >> > >> suggested results. >> > >> For example, select tag just leaves the result when hidePopup() is > > called. >> >> > > >> > > For Autofill, there are two types of values that text fields can have. >> When >> > > the >> > > popup menu is open and you hover over the suggestions, the fields have >> > > "suggested" values, which are displayed to the user, but not visible >> > > to >> > > JavaScript. When the user selects a suggestion, we want to >> > > commit >> these >> > > suggestions, so that they are also visible to JavaScript. >> > > Currently, >> we do >> > > this >> > > by first clearing all of the suggested values, and then subsequently > > setting >> >> > > all >> > > of the actual values. We don't have to do these two steps > > separately, >> >> but >> > > in >> > > the end we want each field to have an actual value set and no >> > > suggested >> > > value >> > > set. >> > > >> > > http://codereview.chromium.org/6902196/ >> > > > > > > http://codereview.chromium.org/6902196/ > |
