Bug 83255

Summary: [chromium] REGRESSION: Refreshed autofill popup renders garbage
Product: WebKit Reporter: Alexander Pavlov (apavlov) <apavlov>
Component: FormsAssignee: Alexander Pavlov (apavlov) <apavlov>
Severity: Normal CC: inferno, isherman, tkent
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
Patch tkent: review+

Description Alexander Pavlov (apavlov) 2012-04-05 00:53:33 PDT
What steps will reproduce the problem?
1. Enter a very long string into autofillable form field and submit form
2. Repeat process and choose very long value from drop down
3. You may need to focus on new field or effect may become immediately visible (see attached image)

What is the expected result?
- Drop down with value (truncated?)
- Form field filled with value, autofill drop down disappears

What happens instead?
- Drop down stretches across page
- When selected dropdown moves to below the address bar followed by a badly rendered portion of the page.

Upstreaming http://code.google.com/p/chromium/issues/detail?id=118374
Comment 1 Alexander Pavlov (apavlov) 2012-04-05 00:59:47 PDT
The issue is due to the incorrect layout/painting of a refreshed popup that contains a PopupListBox that does not fit into the screen width. In this case, the WebWidget - PopupContainer - PopupListBox hierarchy is painted incorrectly: the PopupContainer is given a position relative to the containing window, not the WebWidget (through chromeClient->setWindowRect()), thereby making it paint iself outside of the WebWidget (which results in the garbage), and chromeClient is never notified of the popup relocation.
Comment 2 Alexander Pavlov (apavlov) 2012-04-05 03:55:03 PDT
Created attachment 135798 [details]
Comment 3 Ilya Sherman 2012-04-05 12:36:36 PDT
Comment on attachment 135798 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=135798&action=review

I am not a reviewer, but this looks good to me.  Thanks for the fix!

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=83255

nit: Please add a link to the Chromium bug here as well.

> Source/WebCore/ChangeLog:16
> +        (PopupContainer):

nit: Please briefly describe the changes to each file.

> Source/WebCore/platform/chromium/PopupContainer.cpp:414
> +

nit: I would omit this blank line; but if you prefer to keep it, that's fine too.
Comment 4 Kent Tamura 2012-04-05 20:14:23 PDT
Comment on attachment 135798 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=135798&action=review

> Source/WebCore/ChangeLog:8
> +
> +        No new tests, as the popup appearance is not testable in WebKit.

It would be nice if you explain what was wrong and how do you fix it in ChangeLog.

> Source/WebCore/platform/chromium/PopupContainer.cpp:202
> +    // In screen coordinates.
>      return widgetRect;

Renaming widgetRect to widgetRectInScreen might be better.

> Source/WebCore/platform/chromium/PopupContainer.cpp:418
> +    // In window coordinates.
>      IntPoint location = m_frameView->contentsToWindow(targetControlRect.location());

ditto.  locationInWindow

> Source/WebCore/platform/chromium/PopupContainer.cpp:424
> +    // In screen coordinates.
>      IntRect widgetRect = layoutAndCalculateWidgetRect(targetControlRect.height(), location);

ditto. widgetRectInScreen
Comment 5 Alexander Pavlov (apavlov) 2012-04-06 00:58:38 PDT
Committed r113418: <http://trac.webkit.org/changeset/113418>
Comment 6 Alexander Pavlov (apavlov) 2012-04-06 01:01:26 PDT
Landed with all the comments addressed.