| Summary: | [Chromium]Add clipping for listBox in popup window to fix wrong location display when the autofill item is really long. | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Naoki Takano <honten> | ||||||||||||
| Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | abarth, commit-queue, honten, isherman, tkent | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Naoki Takano
2011-05-22 12:44:18 PDT
Created attachment 94353 [details]
Patch
Created attachment 94354 [details]
Patch
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, Comment on attachment 94354 [details]
Patch
This looks reasonable, but I'm not sure I'm the right person to review this patch.
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. 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? 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? 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. Created attachment 94561 [details]
Patch
Comment on attachment 94561 [details]
Patch
Please review.
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.""" Created attachment 94563 [details]
Patch
Comment on attachment 94563 [details]
Patch
Change some nits according to Ilya's review.
Thanks,
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. Created attachment 94565 [details]
Patch
Comment on attachment 94565 [details]
Patch
Please review again.
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. 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. Comment on attachment 94565 [details] Patch Clearing flags on attachment: 94565 Committed r87137: <http://trac.webkit.org/changeset/87137> All reviewed patches have been landed. Closing bug. |