Bug 65359 - Make form validation bubble fit with Chrome UI style
Summary: Make form validation bubble fit with Chrome UI style
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other All
: P2 Normal
Assignee: Kent Tamura
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-28 21:58 PDT by Kent Tamura
Modified: 2011-08-08 00:27 PDT (History)
3 users (show)

See Also:


Attachments
Patch (94.26 KB, patch)
2011-07-28 23:52 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 2 (96.01 KB, patch)
2011-07-29 04:33 PDT, Kent Tamura
morrita: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2011-07-28 21:58:12 PDT
http://code.google.com/p/chromium/issues/detail?id=88219

The appearance change was requested by Chrome UI team.
Comment 1 Kent Tamura 2011-07-28 23:52:01 PDT
Created attachment 102335 [details]
Patch
Comment 2 Hajime Morrita 2011-07-29 00:21:49 PDT
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.
Comment 3 Kent Tamura 2011-07-29 01:00:37 PDT
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.
Comment 4 Hajime Morrita 2011-07-29 01:03:22 PDT
> It should be a separated patch.
> I have filed https://bugs.webkit.org/show_bug.cgi?id=65363.

That's true. r+ed.
Comment 5 Kent Tamura 2011-07-29 04:33:26 PDT
Created attachment 102344 [details]
Patch 2

Omit ExceptionCode parameter of setShadowPseudoId()
Comment 6 Darin Fisher (:fishd, Google) 2011-07-29 15:27:48 PDT
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.
Comment 7 Kent Tamura 2011-08-02 18:41:05 PDT
(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.
Comment 8 Darin Fisher (:fishd, Google) 2011-08-03 09:39:51 PDT
(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.
Comment 9 Hajime Morrita 2011-08-07 23:26:59 PDT
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?
Comment 10 Kent Tamura 2011-08-08 00:25:49 PDT
Committed r92585: <http://trac.webkit.org/changeset/92585>
Comment 11 Kent Tamura 2011-08-08 00:27:31 PDT
(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.