RESOLVED FIXED 61252
[Chromium]Add clipping for listBox in popup window to fix wrong location display when the autofill item is really long.
https://bugs.webkit.org/show_bug.cgi?id=61252
Summary [Chromium]Add clipping for listBox in popup window to fix wrong location disp...
Naoki Takano
Reported 2011-05-22 12:44:18 PDT
[Chromium]Add clipping for listBox in popup window to fix wrong location display when the autofill item is really long.
Attachments
Patch (6.56 KB, patch)
2011-05-22 12:49 PDT, Naoki Takano
no flags
Patch (6.63 KB, patch)
2011-05-22 12:50 PDT, Naoki Takano
no flags
Patch (6.62 KB, patch)
2011-05-23 22:01 PDT, Naoki Takano
no flags
Patch (6.63 KB, patch)
2011-05-23 22:14 PDT, Naoki Takano
no flags
Patch (6.75 KB, patch)
2011-05-23 22:28 PDT, Naoki Takano
no flags
Naoki Takano
Comment 1 2011-05-22 12:49:48 PDT
Naoki Takano
Comment 2 2011-05-22 12:50:47 PDT
Naoki Takano
Comment 3 2011-05-22 12:53:28 PDT
Comment on attachment 94354 [details] Patch Please review. This is a pretty corner case, but definitely it is a bug. In chromimu, I filed the bug at http://code.google.com/p/chromium/issues/detail?id=83539 Thanks,
Adam Barth
Comment 4 2011-05-22 21:22:28 PDT
Comment on attachment 94354 [details] Patch This looks reasonable, but I'm not sure I'm the right person to review this patch.
Naoki Takano
Comment 5 2011-05-23 12:57:20 PDT
Thanks, Adam. I hope tkent@ or iserman@ will review. (In reply to comment #4) > (From update of attachment 94354 [details]) > This looks reasonable, but I'm not sure I'm the right person to review this patch.
Ilya Sherman
Comment 6 2011-05-23 14:50:49 PDT
Comment on attachment 94354 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94354&action=review > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:370 > + listBox()->setMaxWidthAndLayout(max(widgetRect.width() - kBorderSize * 2, 0)); nit: This line can be moved out of the if/else, since it is the same in both places. > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:1381 > + // windowWidth exceeds m_maxWindowWidth, so we have to clip. nit: Please move this comment into the if-stmt > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:1385 > + baseWidth -= diff; nit: I think these three lines would be clearer as windowWidth = m_maxWindowWidth; baseWidth = windowWidth - scrollbarWidth - paddingWidth; If you don't find that clearer, though, feel free to leave this as is. > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:1386 > + m_baseWidth = baseWidth; nit: Is this line not redundant with the else of the if/else below?
Naoki Takano
Comment 7 2011-05-23 15:14:06 PDT
Ilya, Thank you for your review. > > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:370 > > + listBox()->setMaxWidthAndLayout(max(widgetRect.width() - kBorderSize * 2, 0)); > > nit: This line can be moved out of the if/else, since it is the same in both places. I don't think so, because only this is not "if/else" but "if/else if", and only if width is change, we want to call setMaxWidthAndLayout(). (In reply to comment #6) > (From update of attachment 94354 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=94354&action=review > > > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:370 > > + listBox()->setMaxWidthAndLayout(max(widgetRect.width() - kBorderSize * 2, 0)); > > nit: This line can be moved out of the if/else, since it is the same in both places. > > > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:1381 > > + // windowWidth exceeds m_maxWindowWidth, so we have to clip. > > nit: Please move this comment into the if-stmt > > > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:1385 > > + baseWidth -= diff; > > nit: I think these three lines would be clearer as > windowWidth = m_maxWindowWidth; > baseWidth = windowWidth - scrollbarWidth - paddingWidth; > > If you don't find that clearer, though, feel free to leave this as is. > > > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:1386 > > + m_baseWidth = baseWidth; > > nit: Is this line not redundant with the else of the if/else below?
Ilya Sherman
Comment 8 2011-05-23 15:19:39 PDT
Comment on attachment 94354 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94354&action=review >>>> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:370 >>> >>> nit: This line can be moved out of the if/else, since it is the same in both places. >> >> I don't think so, because only this is not "if/else" but "if/else if", and only if width is change, we want to call setMaxWidthAndLayout(). >> >> (In reply to comment #6) > > Ah, you're right -- my bad.
Naoki Takano
Comment 9 2011-05-23 22:01:47 PDT
Naoki Takano
Comment 10 2011-05-23 22:02:26 PDT
Comment on attachment 94561 [details] Patch Please review.
Ilya Sherman
Comment 11 2011-05-23 22:10:54 PDT
Comment on attachment 94561 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94561&action=review I am not a WebKit reviewer, but this looks good to me. > Source/WebCore/ChangeLog:11 > + * manual-tests/autofill-popup-location.html: With long ong text, test the shown location is correct. nit: Please remove "ong" > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:364 > + // When clipping, we have to set and notify listBox() the change. nit: This comment might be clearer as """When clipping, we also need to set a maximum width for the list box."""
Naoki Takano
Comment 12 2011-05-23 22:14:21 PDT
Naoki Takano
Comment 13 2011-05-23 22:15:52 PDT
Comment on attachment 94563 [details] Patch Change some nits according to Ilya's review. Thanks,
Kent Tamura
Comment 14 2011-05-23 22:18:29 PDT
Comment on attachment 94563 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94563&action=review > Source/WebCore/manual-tests/autofill-popup-location.html:21 > + <li> Enter 'This is really long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long text.' in the following text input area.</li> Does it mean type the text and then type ENTER to submit the form? > Source/WebCore/manual-tests/autofill-popup-location.html:23 > + <li> Enter 'This is short text.' in the following text input area.</li> ditto. > Source/WebCore/manual-tests/autofill-popup-location.html:29 > + <form><input name=p autofocus></form> This HTML already has an input with autofocus. Multiple autofocus inputs are confusing. > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:170 > + , m_maxWindowWidth(INT_MAX) Because this is a C++ code, we had better use numeric_limits<int>::max() instead of INT_MAX.
Naoki Takano
Comment 15 2011-05-23 22:28:44 PDT
Naoki Takano
Comment 16 2011-05-23 22:29:12 PDT
Comment on attachment 94565 [details] Patch Please review again.
Kent Tamura
Comment 17 2011-05-23 22:46:18 PDT
Comment on attachment 94565 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94565&action=review ok > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:170 > + , m_maxWindowWidth(std::numeric_limits<int>::max()) nit: We usually declare "using namespace std;" at the beginning of the file, and omit 'std::' in the code.
Naoki Takano
Comment 18 2011-05-23 22:56:18 PDT
Thanks, Kent-san. Once I fix select event bug, I will clean up code about this namespace std problem and const static k* values problem.
WebKit Commit Bot
Comment 19 2011-05-24 02:03:00 PDT
Comment on attachment 94565 [details] Patch Clearing flags on attachment: 94565 Committed r87137: <http://trac.webkit.org/changeset/87137>
WebKit Commit Bot
Comment 20 2011-05-24 02:03:06 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.