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
Created attachment 84189 [details] Patch
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,
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"
(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 thestig@chromium.org, to whom the Chromium bug is currently assigned.
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,
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());
Created attachment 84381 [details] Patch
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
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.
Created attachment 84522 [details] Patch
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,
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"
Created attachment 84875 [details] Patch
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"
Comment on attachment 84875 [details] Patch ok. Thank you for fixing this!
Comment on attachment 84875 [details] Patch Clearing flags on attachment: 84875 Committed r80448: <http://trac.webkit.org/changeset/80448>
All reviewed patches have been landed. Closing bug.