Page popups can show up at wrong locations

[Keishi Hattori]

ColorChooserUIController::contentSize is returning a size that is too big.

[Keishi Hattori]

Created attachment 157360 [details]
Patch

[Kent Tamura]

I'm not sure why this problem happens.  We have a code to adjust position on resizing in PagePopupChromeClient::setWindowRect().

[Keishi Hattori]

Created attachment 157405 [details]
Screenshot

It seems to be because there isn't enough room above the element.

[Kent Tamura]

(In reply to comment #3)
> Created an attachment (id=157405) [details]
> Screenshot
> 
> It seems to be because there isn't enough room above the element.

ok, I understand. We have no enough room both below the element and above the element, and the current algorithm in setWindowRect() can't correct the bottom position.

IMO, reposition() should be called in PagePopupChromeClient::setWindowRect(), not in WebPagePopupImpl::resize(), and we can remove the current code of PagePopupChromeClient::setWindowRect().

[Kent Tamura]

Comment on attachment 157360 [details]
Patch

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

> Source/WebCore/Resources/colorSuggestionPicker.js:94
>      var errorString = validateArguments(args);
> -    if (errorString)
> +    if (errorString) {
>          main.textContent = "Internal error: " + errorString;
> -    else
> +        updateWindowSize();
> +    } else {
>          new ColorPicker(main, args);
> +    }
>  }

It seems this change is not related to this bug. Do you need to change WebCore to fix this bug?

[Keishi Hattori]

(In reply to comment #4)
> (In reply to comment #3)
> > Created an attachment (id=157405) [details] [details]
> > Screenshot
> > 
> > It seems to be because there isn't enough room above the element.
> 
> ok, I understand. We have no enough room both below the element and above the element, and the current algorithm in setWindowRect() can't correct the bottom position.
> 
> IMO, reposition() should be called in PagePopupChromeClient::setWindowRect(), not in WebPagePopupImpl::resize(), and we can remove the current code of PagePopupChromeClient::setWindowRect().

It would be too late to call reposition in setWindowRect, because setWindowRect takes the screen rect and we can only know the screen rect by calling reposition.

[Keishi Hattori]

(In reply to comment #5)
> (From update of attachment 157360 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157360&action=review
> 
> > Source/WebCore/Resources/colorSuggestionPicker.js:94
> >      var errorString = validateArguments(args);
> > -    if (errorString)
> > +    if (errorString) {
> >          main.textContent = "Internal error: " + errorString;
> > -    else
> > +        updateWindowSize();
> > +    } else {
> >          new ColorPicker(main, args);
> > +    }
> >  }
> 
> It seems this change is not related to this bug. Do you need to change WebCore to fix this bug?

If we return 0x0 for contentSize we won't be able to see the error message.

[Keishi Hattori]

Created attachment 157411 [details]
Patch

[Kent Tamura]

(In reply to comment #7)
> (In reply to comment #5)
> > (From update of attachment 157360 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=157360&action=review
> > 
> > > Source/WebCore/Resources/colorSuggestionPicker.js:94
> > >      var errorString = validateArguments(args);
> > > -    if (errorString)
> > > +    if (errorString) {
> > >          main.textContent = "Internal error: " + errorString;
> > > -    else
> > > +        updateWindowSize();
> > > +    } else {
> > >          new ColorPicker(main, args);
> > > +    }
> > >  }
> > 
> > It seems this change is not related to this bug. Do you need to change WebCore to fix this bug?
> 
> If we return 0x0 for contentSize we won't be able to see the error message.

We never show the error message to end users as far as we know, right?  This change should be done in a separated patch.

[Kent Tamura]

Comment on attachment 157411 [details]
Patch

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

> Source/WebKit/chromium/src/ColorChooserUIController.cpp:95
> -    return WebCore::IntSize(300, 300);
> +    return WebCore::IntSize(0, 0);

Would you update CalendarPickerElement::contentSize too please?

> Source/WebKit/chromium/src/WebPagePopupImpl.cpp:241
> +        m_isPutAboveOrigin = true;

We can remove WebPagePopupImpl::m_isPutAboveOrigin.

[Keishi Hattori]

Created attachment 157420 [details]
Patch

[Keishi Hattori]

> We never show the error message to end users as far as we know, right?  This change should be done in a separated patch.
Done.
(In reply to comment #10)
> (From update of attachment 157411 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157411&action=review
> 
> > Source/WebKit/chromium/src/ColorChooserUIController.cpp:95
> > -    return WebCore::IntSize(300, 300);
> > +    return WebCore::IntSize(0, 0);
> 
> Would you update CalendarPickerElement::contentSize too please?
Done.
> > Source/WebKit/chromium/src/WebPagePopupImpl.cpp:241
> > +        m_isPutAboveOrigin = true;
> 
> We can remove WebPagePopupImpl::m_isPutAboveOrigin.
Done.

[Kent Tamura]

Comment on attachment 157420 [details]
Patch

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

> Source/WebKit/chromium/ChangeLog:3
> +        Color suggestion popup shows up in wrong locations

Should be updated.

> Source/WebKit/chromium/ChangeLog:8
> +        We need to reposition the page popup every time we resize it.

We had better explain what was wrong.

[Keishi Hattori]

Created attachment 157425 [details]
Patch

[WebKit Review Bot]

Comment on attachment 157425 [details]
Patch

Clearing flags on attachment: 157425

Committed r125169: <http://trac.webkit.org/changeset/125169>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.