Bug 83255 - [chromium] REGRESSION: Refreshed autofill popup renders garbage
Summary: [chromium] REGRESSION: Refreshed autofill popup renders garbage
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexander Pavlov (apavlov)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-05 00:53 PDT by Alexander Pavlov (apavlov)
Modified: 2012-04-06 01:01 PDT (History)
3 users (show)

See Also:


Attachments
Patch (4.71 KB, patch)
2012-04-05 03:55 PDT, Alexander Pavlov (apavlov)
tkent: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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]
Patch
Comment 3 Ilya Sherman 2012-04-05 12:36:36 PDT
Comment on attachment 135798 [details]
Patch

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]
Patch

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.