Bug 61252 – [Chromium]Add clipping for listBox in popup window to fix wrong location display when the autofill item is really long.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Naoki Takano]

[Chromium]Add clipping for listBox in popup window to fix wrong location display when the autofill item is really long.

[Naoki Takano]

Created attachment 94353 [details]
Patch

[Naoki Takano]

Created attachment 94354 [details]
Patch

[Naoki Takano]

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 on attachment 94354 [details]
Patch

This looks reasonable, but I'm not sure I'm the right person to review this patch.

[Naoki Takano]

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 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]

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 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]

Created attachment 94561 [details]
Patch

[Naoki Takano]

Comment on attachment 94561 [details]
Patch

Please review.

[Ilya Sherman]

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]

Created attachment 94563 [details]
Patch

[Naoki Takano]

Comment on attachment 94563 [details]
Patch

Change some nits according to Ilya's review.

Thanks,

[Kent Tamura]

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]

Created attachment 94565 [details]
Patch

[Naoki Takano]

Comment on attachment 94565 [details]
Patch

Please review again.

[Kent Tamura]

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]

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 on attachment 94565 [details]
Patch

Clearing flags on attachment: 94565

Committed r87137: <http://trac.webkit.org/changeset/87137>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.