Bug 98499 – Page popup should be smarter about its layout

• Component: Forms
• Status: RESOLVED FIXED

[Keishi Hattori]

1. Page popup should reposition itself so it won't get clipped by screen(Win/Linux) or rootview(Mac) bounds.
2. Page popup should resize itself when it doesn't fit.

[Keishi Hattori]

Created attachment 167307 [details]
Patch

[Kent Tamura]

Comment on attachment 167307 [details]
Patch

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

> Source/WebCore/Resources/pagepopups/pickerCommon.js:102
> +    // Make vertical adjustments

Such "what we do" comment indicates that we had better split the code in this function into multiple functions.

> Source/WebCore/Resources/pagepopups/pickerCommon.js:122
> +    // Make horizontal adjustments

ditto.

> Source/WebCore/page/PagePopupClient.cpp:95
> +void PagePopupClient::addProperty(const char* name, IntRect rect, DocumentWriter& writer)

IntRect -> const IntRect&

> Source/WebCore/page/PagePopupClient.cpp:102
> +    addProperty("x", static_cast<unsigned>(rect.x()), writer);
> +    addProperty("y", static_cast<unsigned>(rect.y()), writer);
> +    addProperty("width", static_cast<unsigned>(rect.width()), writer);
> +    addProperty("height", static_cast<unsigned>(rect.height()), writer);

We should have addProperty for int.

> Source/WebKit/chromium/src/ColorChooserUIController.cpp:111
> +    WebCore::IntRect anchorRectInScreen = m_chromeClient->rootViewToScreen(m_client->elementRectRelativeToRootView());
> +    WebCore::FrameView* view = static_cast<WebViewImpl*>(m_chromeClient->webView())->page()->mainFrame()->view();
> +    WebCore::IntRect rootViewVisibleContentRect = view->visibleContentRect(true /* include scrollbars */);
> +    WebCore::IntRect rootViewRectInScreen = m_chromeClient->rootViewToScreen(rootViewVisibleContentRect);

You may add "using namespace WebCore"

> LayoutTests/ChangeLog:9
> +        * platform/chromium/fast/forms/date/page-popup-adjust-rect-expected.txt: Added.
> +        * platform/chromium/fast/forms/date/page-popup-adjust-rect.html: Added.

Putting this test to fast/forms/date/ looks strange. We should make fast/forms/calendar-picker/ or fast/forms/page-popup/ in order that Android port can skip it easily.

[Keishi Hattori]

Created attachment 167677 [details]
Patch

[Kent Tamura]

Comment on attachment 167677 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        Test: platform/chromium/fast/forms/date/page-popup-adjust-rect.html

Need to udpate

> Source/WebCore/Resources/pagepopups/pickerCommon.js:74
> +Rect.intersection = function(rect1, rect2) {
> +    var x = Math.min(rect1.x, rect2.x);
> +    var maxX = Math.max(rect1.maxX, rect2.maxX);
> +    var y = Math.min(rect1.y, rect2.y);
> +    var maxY = Math.max(rect1.maxY, rect2.maxY);
> +    var width = maxX - x;
> +    var height = maxY - y;
> +    return new Rect(x, y, width, height);

This implementation is not "intersection", but "union".

> Source/WebKit/chromium/src/DateTimeChooserImpl.cpp:114
> +#endif

nit: ColorChooserUIControler..cpp and DateTimeChooserImpl.cpp should share some code.

> LayoutTests/ChangeLog:9
> +        * platform/chromium/fast/forms/page-popup/page-popup-adjust-rect-expected.txt: Added.
> +        * platform/chromium/fast/forms/page-popup/page-popup-adjust-rect.html: Added.

should skip it in platform/chromium-android/TestExpectation

> LayoutTests/platform/chromium/fast/forms/page-popup/page-popup-adjust-rect.html:45
> +    shouldBe('popupWindow.adjustWindowRect(30, 10, 30, 10).toString()', '"Rect(0,0,30,10)"');

You may use shouldBeEqualToString() to avoid nested quotes.

[Keishi Hattori]

Created attachment 167703 [details]
Patch

[Keishi Hattori]

(In reply to comment #4)
> > Source/WebKit/chromium/src/DateTimeChooserImpl.cpp:114
> > +#endif
> 
> nit: ColorChooserUIControler..cpp and DateTimeChooserImpl.cpp should share some code.

It wouldn't belong in PagePopupClient so I wasn't sure where to put it.

[Kent Tamura]

(In reply to comment #6)
> > nit: ColorChooserUIControler..cpp and DateTimeChooserImpl.cpp should share some code.
> 
> It wouldn't belong in PagePopupClient so I wasn't sure where to put it.

We may have WebKit/chromium/src/PagePopupClientImpl.cpp, and they may inehrit PagePopupClientImpl.  Anyway, you don't need to do it in this bug.

[Kent Tamura]

Comment on attachment 167703 [details]
Patch

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

> Source/WebCore/Resources/pagepopups/pickerCommon.js:114
> +        availRect = Rect.intersection(availRect, rootViewRect);

Null dereference would happen if Rect.intersection returns null.
Probably it never happens, but we had better set something non-null to availRect just in case.

[Keishi Hattori]

Created attachment 167704 [details]
Patch

[WebKit Review Bot]

Comment on attachment 167704 [details]
Patch

Clearing flags on attachment: 167704

Committed r130734: <http://trac.webkit.org/changeset/130734>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.