Bug 65359 – Make form validation bubble fit with Chrome UI style

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

[Kent Tamura]

http://code.google.com/p/chromium/issues/detail?id=88219

The appearance change was requested by Chrome UI team.

[Kent Tamura]

Created attachment 102335 [details]
Patch

[Hajime Morrita]

Comment on attachment 102335 [details]
Patch

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

I think it would be better to use dump-as-markup + pixel test instead of using layout test
because dump-as-markup now supports shadows.

> Source/WebCore/html/ValidationMessage.cpp:171
>  

Could you make ASSERT_NO_EXCEPTION a default parameter for setShadowPseudId() ?
The macro is originally made for default parameters and This is a good opportunity to adopt it.
For much-used API like appendChild(), It's better to do it separately though.

[Kent Tamura]

Comment on attachment 102335 [details]
Patch

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

>> Source/WebCore/html/ValidationMessage.cpp:171
>>  
> 
> Could you make ASSERT_NO_EXCEPTION a default parameter for setShadowPseudId() ?
> The macro is originally made for default parameters and This is a good opportunity to adopt it.
> For much-used API like appendChild(), It's better to do it separately though.

It should be a separated patch.
I have filed https://bugs.webkit.org/show_bug.cgi?id=65363.

[Hajime Morrita]

> It should be a separated patch.
> I have filed https://bugs.webkit.org/show_bug.cgi?id=65363.

That's true. r+ed.

[Kent Tamura]

Created attachment 102344 [details]
Patch 2

Omit ExceptionCode parameter of setShadowPseudoId()

[Darin Fisher (:fishd, Google)]

It seems like there should be a WebKit API to ask the embedder to draw this bubble.  That way we can use exactly the same code to render this bubble that Chrome uses to render its other bubbles.

Also, we would then have a chance to solve the problem that this bubble is clipped by the viewport of the browser window.

[Kent Tamura]

(In reply to comment #6)
> It seems like there should be a WebKit API to ask the embedder to draw this bubble.  That way we can use exactly the same code to render this bubble that Chrome uses to render its other bubbles.
> 
> Also, we would then have a chance to solve the problem that this bubble is clipped by the viewport of the browser window.

Let's discuss it separately in http://crbug.com/90958 because it will make drawbacks. I'd like to fix only the style issue for now.

[Darin Fisher (:fishd, Google)]

(In reply to comment #7)
> (In reply to comment #6)
> > It seems like there should be a WebKit API to ask the embedder to draw this bubble.  That way we can use exactly the same code to render this bubble that Chrome uses to render its other bubbles.
> > 
> > Also, we would then have a chance to solve the problem that this bubble is clipped by the viewport of the browser window.
> 
> Let's discuss it separately in http://crbug.com/90958 because it will make drawbacks. I'd like to fix only the style issue for now.

Agreed.  This is a good improvement already.

[Hajime Morrita]

Comment on attachment 102344 [details]
Patch 2

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

Looks OK. It would be great to convert these test to reftests because both visual representation and DOM structures matter for shadow related code.

> Source/WebCore/ChangeLog:3
> +        Make form validation bubble fit with Chrome UI style

[Chromium] prefix?

[Kent Tamura]

Committed r92585: <http://trac.webkit.org/changeset/92585>

[Kent Tamura]

(In reply to comment #9)
> > Source/WebCore/ChangeLog:3
> > +        Make form validation bubble fit with Chrome UI style
> 
> [Chromium] prefix?

I added it and landed.
I wondered if I should have prepend it because this patch changes non-Chromium part.