Bug 61252 - [Chromium]Add clipping for listBox in popup window to fix wrong location display when the autofill item is really long.
Summary: [Chromium]Add clipping for listBox in popup window to fix wrong location disp...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-22 12:44 PDT by Naoki Takano
Modified: 2011-05-24 02:03 PDT (History)
5 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Naoki Takano 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.
Comment 1 Naoki Takano 2011-05-22 12:49:48 PDT
Created attachment 94353 [details]
Patch
Comment 2 Naoki Takano 2011-05-22 12:50:47 PDT
Created attachment 94354 [details]
Patch
Comment 3 Naoki Takano 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,
Comment 4 Adam Barth 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.
Comment 5 Naoki Takano 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.
Comment 6 Ilya Sherman 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?
Comment 7 Naoki Takano 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?
Comment 8 Ilya Sherman 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.
Comment 9 Naoki Takano 2011-05-23 22:01:47 PDT
Created attachment 94561 [details]
Patch
Comment 10 Naoki Takano 2011-05-23 22:02:26 PDT
Comment on attachment 94561 [details]
Patch

Please review.
Comment 11 Ilya Sherman 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."""
Comment 12 Naoki Takano 2011-05-23 22:14:21 PDT
Created attachment 94563 [details]
Patch
Comment 13 Naoki Takano 2011-05-23 22:15:52 PDT
Comment on attachment 94563 [details]
Patch

Change some nits according to Ilya's review.

Thanks,
Comment 14 Kent Tamura 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.
Comment 15 Naoki Takano 2011-05-23 22:28:44 PDT
Created attachment 94565 [details]
Patch
Comment 16 Naoki Takano 2011-05-23 22:29:12 PDT
Comment on attachment 94565 [details]
Patch

Please review again.
Comment 17 Kent Tamura 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.
Comment 18 Naoki Takano 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.
Comment 19 WebKit Commit Bot 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>
Comment 20 WebKit Commit Bot 2011-05-24 02:03:06 PDT
All reviewed patches have been landed.  Closing bug.