| Summary: | Input type=number spin buttons remain invisible but functional after div changed from hidden to visible. | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Naoki Takano <honten> | ||||||||||
| Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | honten, tkent | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Other | ||||||||||||
| OS: | Windows 7 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Naoki Takano
2011-03-05 23:55:00 PST
Created attachment 84885 [details]
Patch
I know this is not a complete patch, because we need baseline for "expected". But I'm not sure if the test is correct. Also I don't have the permission to run the builldbot to generate the baseline for all environment. Anyway, please review the patch and could you tell me how to deal with it if the test is fine? Thanks, (In reply to comment #2) > I know this is not a complete patch, because we need baseline for "expected". > But I'm not sure if the test is correct. Also I don't have the permission to run the builldbot to generate the baseline for all environment. You can make a baseline image on your local machine. Use Tools/Scripts/new-run-webkit-tests for Chromium port. You don't need to make expectations for all platforms. However I'd like to see at least one expectation for a platform in which this bug occurs. Comment on attachment 84885 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=84885&action=review Thank you for working on this. > LayoutTests/ChangeLog:6 > + [Chromium] Issue 73866: Input type=number spin buttons remain invisible but functional after div changed from hidden to visible. > + https://bugs.webkit.org/show_bug.cgi?id=55839 I guess this problem occurs on all of platforms with inner spin button, such as Apple Windows and Chromium-Linux. So we should remove "[Chromium] Issue 73866: " from the description. You may add http://crbug.com/73866 next to the bugs.webkit.org URL. > LayoutTests/ChangeLog:8 > + * fast/forms/input-appearance-spinbutton-visibility.html: Added. We should add expectation files. > LayoutTests/fast/forms/input-appearance-spinbutton-visibility.html:1 > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> nit: <!DOCTYPE html> is enough. > LayoutTests/fast/forms/input-appearance-spinbutton-visibility.html:4 > +<link rel="stylesheet" href="../../../../fast/js/resources/js-test-style.css"> The path looks wrong. It should be ../js/resource/js-test-style.css ? Anyway, I don't hink we need js-test-style.css for this test. > LayoutTests/fast/forms/input-appearance-spinbutton-visibility.html:8 > +input { > + font-size: 20px; > +} The test doesn't depend on font-size. We don't need this line. > LayoutTests/fast/forms/input-appearance-spinbutton-visibility.html:12 > +<p>Test visibility of spin buttons. The number input text area hidden by "visibility:hidden" becomes visible with Javascript visibility change.</p> Please make this paragraph an HTML comment like <!-- Test visibility ... --> We should avoid text in pixel tests as possible. > Source/WebCore/ChangeLog:5 > + [Chromium] Issue 73866: Input type=number spin buttons remain invisible but functional after div changed from hidden to visible. Remove "[Chromium] Issue 73866: " > Source/WebCore/ChangeLog:10 > + Test: fast/forms/input-appearance-spinbutton-visibility.html I guess this patch also fixes http://code.google.com/p/chromium/issues/detail?id=62527. If so, would you add another test for it please? Created attachment 84906 [details]
Patch
Kent-san, I generated the "expected" files. Please review it. Thanks, Comment on attachment 84906 [details]
Patch
The patch still have some issues I pointed out in the previous comment.
Sorry, I'm working on it... (In reply to comment #7) > (From update of attachment 84906 [details]) > The patch still have some issues I pointed out in the previous comment. Created attachment 84909 [details]
Patch
Please review. Thanks, Comment on attachment 84909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=84909&action=review Great. I have one more comment. > LayoutTests/fast/forms/input-appearance-spinbutton-visibility.html:7 > +<!-- Test visibility of spin buttons. The number input text area hidden by "visibility:hidden" becomes visible with Javascript visibility change. Also the test cheks the number input text with an inner spin box is correctly hidden by Javascript visibility change. --> Could you add a description of the expectation more concretely please? It would be "There should be just one number field at the top-left corner" or something like that. Created attachment 84912 [details]
Patch
Could you review again? Comment on attachment 84912 [details]
Patch
ok. I'll land this manually and take care of adding baseline files for some ports.
Great!! Thanks a lot!! (In reply to comment #14) > (From update of attachment 84912 [details]) > ok. I'll land this manually and take care of adding baseline files for some ports. Landed the patch: http://trac.webkit.org/changeset/80449 Landed Mac expectation: http://trac.webkit.org/changeset/80450 |