Summary: | [Chromium] Autocomplete suggestion extends out of window (and onto second monitor) | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Naoki Takano <honten> | ||||||||||
Component: | Forms | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | commit-queue, honten, isherman, tkent | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Naoki Takano
2011-02-18 22:36:22 PST
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. |