WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(6.63 KB, patch)
2011-05-22 12:50 PDT
,
Naoki Takano
no flags
Details
Formatted Diff
Diff
Patch
(6.62 KB, patch)
2011-05-23 22:01 PDT
,
Naoki Takano
no flags
Details
Formatted Diff
Diff
Patch
(6.63 KB, patch)
2011-05-23 22:14 PDT
,
Naoki Takano
no flags
Details
Formatted Diff
Diff
Patch
(6.75 KB, patch)
2011-05-23 22:28 PDT
,
Naoki Takano
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Naoki Takano
Comment 1
2011-05-22 12:49:48 PDT
Created
attachment 94353
[details]
Patch
Naoki Takano
Comment 2
2011-05-22 12:50:47 PDT
Created
attachment 94354
[details]
Patch
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
Created
attachment 94561
[details]
Patch
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
Created
attachment 94563
[details]
Patch
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
Created
attachment 94565
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug