Bug 63438 – [Chromium] SELECT or autofill popup is trimmed by screen edge on Windows

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Alexander Pavlov (apavlov)]

Currently, our SELECT/autofill popups are trimmed on the right(LTR)/left(RTL) by the corresponding screen edge if their content does not fit the screen with the browser window.

Considering the LTR case, if a popup is close to the right edge of the screen, much of the content can be trimmed off if the SELECT has a custom look (using -webkit-appearance: none). Please see the attached screenshot (for Web Inspector).
The suggested solution is to inverse the alignment of the popup (align the right edges for LTR and the left edges for RTL) if the trimmed-off content is shorter in the inverted case - see the fix in action in another screenshot.

I have not fixed the tests (if any) yet since it's too early to say if this approach is acceptable. What do you think?

[Alexander Pavlov (apavlov)]

Created attachment 98682 [details]
[IMAGE] Trimmed popup screenshot

[Alexander Pavlov (apavlov)]

Created attachment 98684 [details]
[IMAGE] Screenshot of a popup with the fixed alignment

[Alexander Pavlov (apavlov)]

Created attachment 98686 [details]
[PATCH] Suggested solution

[WebKit Review Bot]

Comment on attachment 98686 [details]
[PATCH] Suggested solution

Attachment 98686 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8948180

[Kent Tamura]

(In reply to comment #0)
> The suggested solution is to inverse the alignment of the popup (align the right edges for LTR and the left edges for RTL) if the trimmed-off content is shorter in the inverted case - see the fix in action in another screenshot.

It sounds a good idea.

[Kent Tamura]

Comment on attachment 98686 [details]
[PATCH] Suggested solution

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

> Source/WebCore/ChangeLog:13
> +        (WebCore::PopupContainer::layoutAndCalculateWidgetRect):
> +        (WebCore::PopupContainer::layoutAndGetRTLOffset):
> +        (WebCore::PopupContainer::showInRect):
> +        (WebCore::PopupContainer::refresh):
> +        (WebCore::PopupContainer::isRTL):

Need explanations.
- Why do you think layoutAndGetRTLOffset is better than layoutAndGetRightOffset?
- Why is it ok to remove some code of layoutAndGetRTLOffset()?
- Why do we need to keep m_originalFrameRect?

> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:350
> +    m_listBox->setMaxWidthAndLayout(std::numeric_limits<int>::max());
>  
>      // Lay everything out to figure out our preferred size, then tell the view's
>      // WidgetClient about it.  It should assign us a client.
> -    int rightOffset = layoutAndGetRightOffset();
> +    int rtlOffset = layoutAndGetRTLOffset();

Calling layout() twice is not good.

We usually declare "using namespace std;" and omit std:: in the code.

[Alexander Pavlov (apavlov)]

A fixed patch to be attached shortly.

(In reply to comment #6)
> (From update of attachment 98686 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98686&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +        (WebCore::PopupContainer::layoutAndCalculateWidgetRect):
> > +        (WebCore::PopupContainer::layoutAndGetRTLOffset):
> > +        (WebCore::PopupContainer::showInRect):
> > +        (WebCore::PopupContainer::refresh):
> > +        (WebCore::PopupContainer::isRTL):
> 
> Need explanations.
> - Why do you think layoutAndGetRTLOffset is better than layoutAndGetRightOffset?

The method semantics have changed, so I decided to reflect this change in the name. the "right offset" used to be the actual offset to be added to the left edge x-coordinate of the focused element (e.g. SELECT or INPUT). Now the method _always_ returns what used to be "right offset" but this offset is initially added to the x-coordinate only in the case of RTL (or later, if we decide to use the RTL-based popup layout for an LTR SELECT parent). If you think this semantic change is minor, I can quickly revert the method rename.

> - Why is it ok to remove some code of layoutAndGetRTLOffset()?

It's for the reason stated above: we always return the "right offset" of the popup, regardless of the RTL-ness, so the actual code checking if the control is RTL is not needed. Instead, we perform this check in the code invoking layoutAndGetRTLOffset().

> - Why do we need to keep m_originalFrameRect?

This is the trickiest part of the change. Consider the following case:
- you click an INPUT for which autofill entries exist
- a popup with the autofill suggestions pops up. At this point, the popup frameRect can get resized to be wider than the INPUT element (to host long suggestion items)
- you click the same INPUT once again. PopupContainer::refresh() is called for this same popup, which calls layoutAndCalculateWidgetRect(), but at this point the actual frameRect is wider than it used to be when the initial PopupContainer::showInRect() was called (frameRect gets updated in layoutAndGetRTLOffset(), the resize() call).
- the entire layout is performed with the new, widened frameRect used as the initial one, when the popup was initially shown. This causes various layout artifacts inside the popup itself (items are misaligned), and the popup itself can get misaligned, too (it's weird, but when you click the INPUT three times in a row, you get three different layouts of the popup).

That's what made me reset the frameRect to the initially requested rectangle at this point (and reset the max width elsewhere, since now you can get this trimmed popup, then move the browser window so that there is enough space on the right to accommodate the popup, and click the same SELECT element once again - you will see the popup trimmed on the right to the same size as it had when shown previously).

> > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:350
> > +    m_listBox->setMaxWidthAndLayout(std::numeric_limits<int>::max());
> >  
> >      // Lay everything out to figure out our preferred size, then tell the view's
> >      // WidgetClient about it.  It should assign us a client.
> > -    int rightOffset = layoutAndGetRightOffset();
> > +    int rtlOffset = layoutAndGetRTLOffset();
> 
> Calling layout() twice is not good.

Right, thanks. I've added a new method, setMaxWidth().

> We usually declare "using namespace std;" and omit std:: in the code.

Thanks for the hint, done.

[Alexander Pavlov (apavlov)]

Created attachment 98698 [details]
[PATCH] Comments addressed

[WebKit Review Bot]

Attachment 98698 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/chromium/PopupMenuChromium.cpp:66:  Use 'using namespace std;' instead of 'using std::numeric_limits;'.  [build/using_std] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexander Pavlov (apavlov)]

Created attachment 98699 [details]
[PATCH] Style fixed

[Kent Tamura]

(In reply to comment #7)

Please summarize these explanations and add them to ChangeLog.
The file list in a ChangeLog entry is a good place to explain reasons of code changes.


> > - Why do you think layoutAndGetRTLOffset is better than layoutAndGetRightOffset?
> 
> The method semantics have changed, so I decided to reflect this change in the name. the "right offset" used to be the actual offset to be added to the left edge x-coordinate of the focused element (e.g. SELECT or INPUT). Now the method _always_ returns what used to be "right offset" but this offset is initially added to the x-coordinate only in the case of RTL (or later, if we decide to use the RTL-based popup layout for an LTR SELECT parent). If you think this semantic change is minor, I can quickly revert the method rename.
> 
> > - Why is it ok to remove some code of layoutAndGetRTLOffset()?
> 
> It's for the reason stated above: we always return the "right offset" of the popup, regardless of the RTL-ness, so the actual code checking if the control is RTL is not needed. Instead, we perform this check in the code invoking layoutAndGetRTLOffset().
> 
> > - Why do we need to keep m_originalFrameRect?
> 
> This is the trickiest part of the change. Consider the following case:
> - you click an INPUT for which autofill entries exist
> - a popup with the autofill suggestions pops up. At this point, the popup frameRect can get resized to be wider than the INPUT element (to host long suggestion items)
> - you click the same INPUT once again. PopupContainer::refresh() is called for this same popup, which calls layoutAndCalculateWidgetRect(), but at this point the actual frameRect is wider than it used to be when the initial PopupContainer::showInRect() was called (frameRect gets updated in layoutAndGetRTLOffset(), the resize() call).
> - the entire layout is performed with the new, widened frameRect used as the initial one, when the popup was initially shown. This causes various layout artifacts inside the popup itself (items are misaligned), and the popup itself can get misaligned, too (it's weird, but when you click the INPUT three times in a row, you get three different layouts of the popup).
> 
> That's what made me reset the frameRect to the initially requested rectangle at this point (and reset the max width elsewhere, since now you can get this trimmed popup, then move the browser window so that there is enough space on the right to accommodate the popup, and click the same SELECT element once again - you will see the popup trimmed on the right to the same size as it had when shown previously).

[Alexander Pavlov (apavlov)]

(In reply to comment #11)
> (In reply to comment #7)
> 
> Please summarize these explanations and add them to ChangeLog.
> The file list in a ChangeLog entry is a good place to explain reasons of code changes.

Will do shortly. The original patch was a proof-of-concept to find out whether I was moving in the right direction (since I'm completely ignorant of the Chromium UI guidelines).

[Alexander Pavlov (apavlov)]

Created attachment 98711 [details]
[PATCH] ChangeLog message augmented

[Kent Tamura]

Comment on attachment 98711 [details]
[PATCH] ChangeLog message augmented

ok.  Please add a reason this change has no tests to ChangeLog.

[Alexander Pavlov (apavlov)]

Committed r89916: <http://trac.webkit.org/changeset/89916>