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.
Created attachment 167307 [details] Patch
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.
Created attachment 167677 [details] Patch
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.
Created attachment 167703 [details] Patch
(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.
(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.
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.
Created attachment 167704 [details] Patch
Comment on attachment 167704 [details] Patch Clearing flags on attachment: 167704 Committed r130734: <http://trac.webkit.org/changeset/130734>
All reviewed patches have been landed. Closing bug.