Bug 54795 – [Chromium] Autocomplete suggestion extends out of window (and onto second monitor)

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Naoki Takano]

What steps will reproduce the problem?
1. Enter a very long entry into a box which has suggestions
2. Hit Enter
3. Go back and start to enter that entry again

What is the expected result?
The result is truncated to the window or desktop

What happens instead?
It extends outside the window and onto a second monitor

See
http://code.google.com/p/chromium/issues/detail?id=71371

[Naoki Takano]

Created attachment 84189 [details]
Patch

[Naoki Takano]

Ilya,

Could you review the patch?

As I wrote in ChangeLog, this fix is not enough for Linux.

Actually, the problem is related to
http://code.google.com/p/chromium/issues/detail?id=28560

I guess we need to call Xinerama or other special APIs.

According to my survey, please refer to
http://ubuntuforums.org/showthread.php?t=221174

Actually, FireFox only consider Xinerama.
If Chrome does the same way, I try.

But, first, I want to land this patch.

Thanks,

[Ilya Sherman]

Comment on attachment 84189 [details]
Patch

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

I am not a WebKit reviewer, but here are my thoughts:

> Source/WebCore/ChangeLog:8
> +        Implement width clip logic according to browser screen width and popup window width. This fix is enough for Win and Mac, but there is a problem in Linux. Because WebScreenInfoFactory::screenInfo() can get only merged screen size, not the screen size where the browser exits.

nit: exits => exists

> Source/WebCore/ChangeLog:10
> +        Test: fast/canvas/2d.composite.globalAlpha.fillPath.html and canvas/philip/tests/2d.composite.globalAlpha.fill.html with --accelerated-2d-canvas. With multiple screens and submit long input string first. And check the popup window size after that.

I don't understand why tests and fast/canvas would be relevant here.  I would just leave out this line, and instead include a link above to the Chromium bug.

> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:364
> +            if (widgetRect.maxX() > screen.maxX())
> +                widgetRect.setWidth(screen.maxX() - widgetRect.x());

nit: Should this be an "else if"?

> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:386
> +                int w = widgetRect.width();

nit: Please name this variable "width"

[Ilya Sherman]

(In reply to comment #2)
> As I wrote in ChangeLog, this fix is not enough for Linux.
> 
> Actually, the problem is related to
> http://code.google.com/p/chromium/issues/detail?id=28560
> 
> I guess we need to call Xinerama or other special APIs.
> 
> According to my survey, please refer to
> http://ubuntuforums.org/showthread.php?t=221174
> 
> Actually, FireFox only consider Xinerama.
> If Chrome does the same way, I try.

I'm not sure what the right solution on Linux is.  You could try asking [e-mail], to whom the Chromium bug is currently assigned.

[Naoki Takano]

Ilya,

Thank you for your review.

(In reply to comment #3)
> (From update of attachment 84189 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84189&action=review
> 
> I am not a WebKit reviewer, but here are my thoughts:
> 
> > Source/WebCore/ChangeLog:8
> > +        Implement width clip logic according to browser screen width and popup window width. This fix is enough for Win and Mac, but there is a problem in Linux. Because WebScreenInfoFactory::screenInfo() can get only merged screen size, not the screen size where the browser exits.
> 
> nit: exits => exists
Yes, this is typo.

> > Source/WebCore/ChangeLog:10
> > +        Test: fast/canvas/2d.composite.globalAlpha.fillPath.html and canvas/philip/tests/2d.composite.globalAlpha.fill.html with --accelerated-2d-canvas. With multiple screens and submit long input string first. And check the popup window size after that.
> 
> I don't understand why tests and fast/canvas would be relevant here.  I would just leave out this line, and instead include a link above to the Chromium bug.
I completely copy&paste wrong text. I'll fix it.

> > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:364
> > +            if (widgetRect.maxX() > screen.maxX())
> > +                widgetRect.setWidth(screen.maxX() - widgetRect.x());
> 
> nit: Should this be an "else if"?
Yes, you are right.

> > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:386
> > +                int w = widgetRect.width();
> 
> nit: Please name this variable "width"
Ok,

[Kent Tamura]

Comment on attachment 84189 [details]
Patch

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

>> Source/WebCore/ChangeLog:10
>> +        Test: fast/canvas/2d.composite.globalAlpha.fillPath.html and canvas/philip/tests/2d.composite.globalAlpha.fill.html with --accelerated-2d-canvas. With multiple screens and submit long input string first. And check the popup window size after that.

ChangeLog is not a place to write a test procedure of the change.  I think we need to introduce new manual test, and should write the test procedure in it.

> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:389
> +                int x = widgetRect.x();
> +                int w = widgetRect.width();
>                  widgetRect = chromeClient->windowToScreen(frameRect());
> +                widgetRect.setX(x);
> +                widgetRect.setWidth(w);

This code doesn't look straightforward.
How about:

IntRect frameInScreen = chromeClient->windowToScreen(frameRect());
widgetRect.setY(frameInScreen.y());
widgetRect.setHeight(frameInScreen.height());

[Naoki Takano]

Created attachment 84381 [details]
Patch

[Naoki Takano]

Kent-san,

Thank you for your support every time.

Please review.

I added a new manual test.

Thanks,

(In reply to comment #7)
> Created an attachment (id=84381) [details]
> Patch

[Kent Tamura]

Comment on attachment 84381 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Test: manual-tests/autofill-popup-width-restrction-within-screen.html

This bug is reproducible for <select>, right?  If so, we had better remove the word "autofill" from the file name.
If this bug is only for Chromium's autofill feature, the test should be in WebCore/manual-tests/chromium/.

> Source/WebCore/manual-tests/autofill-popup-width-restrction-within-screen.html:26
> +   1, Launch Chrome and maximize the browser window size in the right screen.<br>
> +   2, Enter a very long string into "Right to Left" text input box.<br>
> +   3, Hit enter.<br>
> +   5, Go back and start to enter the same string again.<br>
> +   6, Make sure the popup list box's width which suggests the result doesn't exceed the right screen width.<br>

There is no step 4? :-)
Your HTML looks not good.
- Please use either of lowercase tag names or uppercase tag names consistently.
- Use <ol><li> for ordered lists.

[Naoki Takano]

Created attachment 84522 [details]
Patch

[Naoki Takano]

Thank you for your review.

(In reply to comment #9)
> (From update of attachment 84381 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84381&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        Test: manual-tests/autofill-popup-width-restrction-within-screen.html
> 
> This bug is reproducible for <select>, right?  If so, we had better remove the word "autofill" from the file name.
> If this bug is only for Chromium's autofill feature, the test should be in WebCore/manual-tests/chromium/.
Yes, this bug is also reproducible for <select>. So I remove the word "autofill" from the file name.

> > Source/WebCore/manual-tests/autofill-popup-width-restrction-within-screen.html:26
> > +   1, Launch Chrome and maximize the browser window size in the right screen.<br>
> > +   2, Enter a very long string into "Right to Left" text input box.<br>
> > +   3, Hit enter.<br>
> > +   5, Go back and start to enter the same string again.<br>
> > +   6, Make sure the popup list box's width which suggests the result doesn't exceed the right screen width.<br>
> 
> There is no step 4? :-)
> Your HTML looks not good.
> - Please use either of lowercase tag names or uppercase tag names consistently.
> - Use <ol><li> for ordered lists.
I fixed them.

Thanks,

[Kent Tamura]

Comment on attachment 84522 [details]
Patch

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

> Source/WebCore/manual-tests/popup-width-restriction-within-screen.html:13
> +   <li> Launch Chrome and maximize the browser window size in the left screen.</li>

Do not use the specific browser name "Chrome".
This test is helpful for any browsers.

Also, can we provide <select> with very long <option> in advance instead of depending on input history suggestion feature?  Though it might be not enough for really large display.

> Source/WebCore/manual-tests/popup-width-restriction-within-screen.html:23
> +   <li> Launch Chrome and maximize the browser window size in the right screen.</li>

ditto. Do not use "Chrome"

[Naoki Takano]

Created attachment 84875 [details]
Patch

[Naoki Takano]

Kent-san,

I added <select> example, please review it.

Thanks,

(In reply to comment #12)
> (From update of attachment 84522 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84522&action=review
> 
> > Source/WebCore/manual-tests/popup-width-restriction-within-screen.html:13
> > +   <li> Launch Chrome and maximize the browser window size in the left screen.</li>
> 
> Do not use the specific browser name "Chrome".
> This test is helpful for any browsers.
> 
> Also, can we provide <select> with very long <option> in advance instead of depending on input history suggestion feature?  Though it might be not enough for really large display.
> 
> > Source/WebCore/manual-tests/popup-width-restriction-within-screen.html:23
> > +   <li> Launch Chrome and maximize the browser window size in the right screen.</li>
> 
> ditto. Do not use "Chrome"

[Kent Tamura]

Comment on attachment 84875 [details]
Patch

ok.
Thank you for fixing this!

[WebKit Commit Bot]

Comment on attachment 84875 [details]
Patch

Clearing flags on attachment: 84875

Committed r80448: <http://trac.webkit.org/changeset/80448>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.