Bug 55839

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 BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: honten, tkent
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Windows 7   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch tkent: review+, tkent: commit-queue-

Naoki Takano
Reported 2011-03-05 23:55:00 PST
[Chromium] Issue 73866: Input type=number spin buttons remain invisible but functional after div changed from hidden to visible.
Attachments
Patch (3.41 KB, patch)
2011-03-06 00:00 PST, Naoki Takano 		no flags
DetailsFormatted DiffDiff
Patch (12.37 KB, patch)
2011-03-06 18:41 PST, Naoki Takano 		no flags
DetailsFormatted DiffDiff
Patch (6.92 KB, patch)
2011-03-06 19:36 PST, Naoki Takano 		no flags
DetailsFormatted DiffDiff
Patch (7.04 KB, patch)
2011-03-06 19:55 PST, Naoki Takano 		tkent: review+
tkent: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Naoki Takano
Comment 1 2011-03-06 00:00:56 PST
Created attachment 84885 [details] Patch
Naoki Takano
Comment 2 2011-03-06 00:08:43 PST
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,
Kent Tamura
Comment 3 2011-03-06 18:23:07 PST
(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.
Kent Tamura
Comment 4 2011-03-06 18:31:38 PST
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 &quot;visibility:hidden&quot; 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?
Naoki Takano
Comment 5 2011-03-06 18:41:35 PST
Created attachment 84906 [details] Patch
Naoki Takano
Comment 6 2011-03-06 18:42:17 PST
Kent-san, I generated the "expected" files. Please review it. Thanks,
Kent Tamura
Comment 7 2011-03-06 18:58:11 PST
Comment on attachment 84906 [details] Patch The patch still have some issues I pointed out in the previous comment.
Naoki Takano
Comment 8 2011-03-06 19:19:14 PST
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.
Naoki Takano
Comment 9 2011-03-06 19:36:10 PST
Created attachment 84909 [details] Patch
Naoki Takano
Comment 10 2011-03-06 19:37:08 PST
Please review. Thanks,
Kent Tamura
Comment 11 2011-03-06 19:50:42 PST
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.
Naoki Takano
Comment 12 2011-03-06 19:55:26 PST
Created attachment 84912 [details] Patch
Naoki Takano
Comment 13 2011-03-06 20:00:59 PST
Could you review again?
Kent Tamura
Comment 14 2011-03-06 21:08:07 PST
Comment on attachment 84912 [details] Patch ok. I'll land this manually and take care of adding baseline files for some ports.
Naoki Takano
Comment 15 2011-03-06 21:09:05 PST
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.
Kent Tamura
Comment 16 2011-03-06 21:14:51 PST
Landed the patch: http://trac.webkit.org/changeset/80449 Landed Mac expectation: http://trac.webkit.org/changeset/80450
Note You need to log in before you can comment on or make changes to this bug.