| Summary: | Make form validation bubble fit with Chrome UI style | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Kent Tamura <tkent> | ||||||
| Component: | Forms | Assignee: | Kent Tamura <tkent> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | dglazkov, fishd, morrita | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Other | ||||||||
| OS: | All | ||||||||
| Attachments: |
|
||||||||
|
Description
Kent Tamura
2011-07-28 21:58:12 PDT
Created attachment 102335 [details]
Patch
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 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. > It should be a separated patch.
> I have filed https://bugs.webkit.org/show_bug.cgi?id=65363.
That's true. r+ed.
Created attachment 102344 [details]
Patch 2
Omit ExceptionCode parameter of setShadowPseudoId()
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. (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. (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 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? Committed r92585: <http://trac.webkit.org/changeset/92585> (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. |