Bug 48497 – Avoid overlapping label text in autofill popup with icon

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

[Ilya Sherman]

Avoid overlapping label text in autocomplete popup with icon

[Ilya Sherman]

Created attachment 72134 [details]
Patch

[Ilya Sherman]

paintRow() probably also needs to updated to take the icon width into account for the |restrictWidthOfListBox| case.  It's not entirely obvious to me how that should work -- should the label take precedence to the icon, or vice versa?  If there are multiple rows and only some of the rows have overflow, what alignment do we want across the rows?

This could probably also use a layout test, but I couldn't find any existing layout tests for these functions.  Could someone please point me in the right direction?

[Ilya Sherman]

I should have mentioned -- this corresponds to Chromium bug 60096: http://code.google.com/p/chromium/issues/detail?id=60096

[Jay Civelli]

View in context: https://bugs.webkit.org/attachment.cgi?id=72134&action=review

> WebCore/platform/chromium/PopupMenuChromium.cpp:1272
> +              if (width > 0)

4 space indent

> WebCore/platform/chromium/PopupMenuChromium.cpp:1277
> +

We should probably take into account the icon when computing the row height.

[Ilya Sherman]

Created attachment 72274 [details]
Patch, addressing Jay's comments

[Ilya Sherman]

Comment on attachment 72274 [details]
Patch, addressing Jay's comments

New patch up, addressing Jay's review comments

[WebKit Review Bot]

Attachment 72274 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4830078

[Jay Civelli]

I am not a reviewer but R+ for me.

[Kent Tamura]

Comment on attachment 72274 [details]
Patch, addressing Jay's comments

View in context: https://bugs.webkit.org/attachment.cgi?id=72274&action=review

> WebCore/ChangeLog:741
> [phone]  Ilya Sherman  <[e-mail]>

New ChangeLog entry should be at the beginning of the file.

> WebCore/platform/chromium/PopupMenuChromium.cpp:1278
> +                width += kLabelToIconPadding;

We don't use "k" prefix in WebKit style.
I understand this symbol is not introduced by this patch.  So you don't need to address this issue for now.

[Ilya Sherman]

Created attachment 72531 [details]
Patch

I think I uploaded an old version of the patch somehow last time; hoping things work better this time

[Ilya Sherman]

Created attachment 72532 [details]
Patch

Third time's the charm?

[Kent Tamura]

Comment on attachment 72532 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=72532&action=review

> WebCore/ChangeLog:9
> +        No new tests. (OOPS!)

Remove this line please.

[Ilya Sherman]

(In reply to comment #12)
> (From update of attachment 72532 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72532&action=review
> 
> > WebCore/ChangeLog:9
> > +        No new tests. (OOPS!)
> 
> Remove this line please.

Does that mean this patch doesn't need any tests?  I'd figured that we want tests, but wasn't sure where to add them and what to use as a starting point...

[Kent Tamura]

Comment on attachment 72532 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=72532&action=review

>>> WebCore/ChangeLog:9
>>> +        No new tests. (OOPS!)
>> 
>> Remove this line please.
> 
> Does that mean this patch doesn't need any tests?  I'd figured that we want tests, but wasn't sure where to add them and what to use as a starting point...

Of course we should have tests as possible.  I meant we should remove (OOPS!) except "Reviewed by" line.  Commit-queue rejects a patch with (OOPS!).

We already have WebKit/chromium/tests/PopupMenuTest.cpp, but I think it's difficult to add a test for this change in WebKit.  So I think it's ok to have no tests.

[Ilya Sherman]

Created attachment 72675 [details]
Patch

[Ilya Sherman]

(In reply to comment #14)
> (From update of attachment 72532 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72532&action=review
> 
> >>> WebCore/ChangeLog:9
> >>> +        No new tests. (OOPS!)
> >> 
> >> Remove this line please.
> > 
> > Does that mean this patch doesn't need any tests?  I'd figured that we want tests, but wasn't sure where to add them and what to use as a starting point...
> 
> Of course we should have tests as possible.  I meant we should remove (OOPS!) except "Reviewed by" line.  Commit-queue rejects a patch with (OOPS!).
> 
> We already have WebKit/chromium/tests/PopupMenuTest.cpp, but I think it's difficult to add a test for this change in WebKit.  So I think it's ok to have no tests.

Ok, removed.

[WebKit Commit Bot]

Comment on attachment 72675 [details]
Patch

Clearing flags on attachment: 72675

Committed r71191: <http://trac.webkit.org/changeset/71191>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.