Bug 5656 - REGRESSION: Buttons on Yahoo! Mail misplaced in ToT
Summary: REGRESSION: Buttons on Yahoo! Mail misplaced in ToT
Status: VERIFIED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P1 Critical
Assignee: David Kilzer (:ddkilzer)
URL: http://mail.yahoo.com
Keywords:
Depends on:
Blocks:
 
Reported: 2005-11-07 15:51 PST by Samuel Allen
Modified: 2006-01-31 21:20 PST (History)
2 users (show)

See Also:


Attachments
Screenshot of top of Yahoo! Mail (57.70 KB, image/tiff)
2005-11-07 15:52 PST, Samuel Allen
no flags Details
Firefox screenshot (correct rendering) (75.12 KB, image/tiff)
2005-11-07 15:59 PST, Samuel Allen
no flags Details
A reduced testcase for Yahoo! Mail bug (421 bytes, text/html)
2005-11-17 15:26 PST, Samuel Allen
no flags Details
Minimized testcase (167 bytes, text/html)
2005-11-17 22:46 PST, Samuel Allen
no flags Details
Patch v1 (6.10 KB, patch)
2006-01-19 22:46 PST, David Kilzer (:ddkilzer)
mjs: review-
Details | Formatted Diff | Diff
Image v1 (10.46 KB, image/png)
2006-01-19 22:48 PST, David Kilzer (:ddkilzer)
no flags Details
Patch v2 (7.90 KB, patch)
2006-01-22 00:39 PST, David Kilzer (:ddkilzer)
darin: review-
Details | Formatted Diff | Diff
Test Image Expected v2 (29.43 KB, image/png)
2006-01-22 00:47 PST, David Kilzer (:ddkilzer)
no flags Details
Proposed layout test (584 bytes, text/html)
2006-01-22 12:49 PST, David Kilzer (:ddkilzer)
no flags Details
Patch v3 (9.02 KB, patch)
2006-01-22 15:47 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Test Image Expected v3 (43.93 KB, image/png)
2006-01-22 15:48 PST, David Kilzer (:ddkilzer)
no flags Details
Patch v4 (8.73 KB, patch)
2006-01-22 16:12 PST, David Kilzer (:ddkilzer)
mjs: review+
Details | Formatted Diff | Diff
Test Image Expected v4 (36.45 KB, image/png)
2006-01-22 16:13 PST, David Kilzer (:ddkilzer)
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Samuel Allen 2005-11-07 15:51:58 PST
In the latest builds the new button code is enabled. This works great on most sites, but the buttons at the 
top of Yahoo! Mail are misaligned. They appear too low. I think the problem is obvious when you look at 
the screenshot.
Comment 1 Samuel Allen 2005-11-07 15:52:31 PST
Created attachment 4623 [details]
Screenshot of top of Yahoo! Mail
Comment 2 Dave Hyatt 2005-11-07 15:54:55 PST
What do they look like in Firefox?
Comment 3 Samuel Allen 2005-11-07 15:59:54 PST
Created attachment 4624 [details]
Firefox screenshot (correct rendering)
Comment 4 Dave Hyatt 2005-11-08 13:42:27 PST
I'm not sure if this is a button problem or a white-space problem.  The buttons in the rightmost cell are 
wrapping now (despite the cell having a nowrap specified).  This needs a reduction.
Comment 5 Samuel Allen 2005-11-17 15:23:44 PST
Ok I did some digging into this problem and created a reduced testcase. To my surprise, I don't think 
buttons are to blame. Basically there are two buttons in a table cell with nowrap specified. The ID of one of 
them is "searchmail" and the ID of the other is "searchtheweb". However, the stylesheet associated with the 
page only contains styles for "searchmail1" and "searchtheweb". When the 1 is taken off, all three browsers 
work. When the 1 remains, Safari 2.0.2, Firefox 1.5 succeed but WebKit ToT fails. Has something changed 
in selector parsing or matching in the recently? Should "searchmail1" match "searchmail"? I don't know the 
answers to these questions. I will, however attach the reduced testcase. It consists of two buttons, which 
in all browsers other than ToT line up on one line.
Comment 6 Samuel Allen 2005-11-17 15:26:21 PST
Created attachment 4717 [details]
A reduced testcase for Yahoo! Mail bug

This works on all browsers except ToT.
Comment 7 Samuel Allen 2005-11-17 15:30:45 PST
Actually, the "searchmail1" rule is not applied in any of the testcases. So I don't have a clue what the 
problem is. As I said, the reduced testcase works on every browser except ToT.
Comment 8 Samuel Allen 2005-11-17 22:44:37 PST
The problem is with "nowrap". The test case features a td cell with nowrap specified, and then two 
buttons, the second of which has style="white-space:nowrap". When either nowrap is removed, the 
testcase lays out correctly.
Comment 9 Samuel Allen 2005-11-17 22:46:40 PST
Created attachment 4720 [details]
Minimized testcase

Works correctly in Firefox and Safari, not on ToT.
Comment 10 Eric Seidel (no email) 2005-12-28 01:55:14 PST
The test case looks like ot works correclty.  Or?
Comment 11 Joost de Valk (AlthA) 2005-12-28 07:12:27 PST
No it doesn't, buttons should be next to each other. Reopening.
Comment 12 Eric Seidel (no email) 2005-12-28 13:23:22 PST
They appear next to each other to me [search mail] [search the web] ?
Comment 13 Joost de Valk (AlthA) 2005-12-29 00:16:11 PST
They don't to me, and this is latest ToT...
Comment 14 David Kilzer (:ddkilzer) 2006-01-08 12:48:32 PST
The space between the last </button> tag and the </td> tag is what causes the button to wrap.  If you 
remove the space between these two tags, the HTML renders correctly:

</button></td>

ANY whitespace (spaces, tabs, newlines) will cause it to render incorrectly.  Hopefully this will narrow 
down the cause!
Comment 15 Alice Liu 2006-01-10 10:58:53 PST
<rdar://problem/4404335>
Comment 16 David Kilzer (:ddkilzer) 2006-01-19 19:28:41 PST
I did a binary search through the nightly builds (thanks Mark Rowe!) and found where the bug was 
introduced:

Testcase passes: WebKit-CVS-2005-10-22 04-42-12 GMT.dmg
Testcase fails:  WebKit-CVS-2005-10-23 02-27-01 GMT.dmg

I will begin looking at commits during that time to see if I can determine which change caused this bug, 
unless someone beats me to it!
Comment 17 David Kilzer (:ddkilzer) 2006-01-19 19:55:57 PST
Using "svn log -r{YYYY-MM-DD}", I determined that these CVS builds match the following Subversion 
revisions:

Passed: WebKit-CVS-2005-10-22 04-42-12 GMT.dmg: r10904
Failed: WebKit-CVS-2005-10-23 02-27-01 GMT.dmg: r10916
Comment 18 David Kilzer (:ddkilzer) 2006-01-19 22:46:23 PST
Created attachment 5792 [details]
Patch v1

The regression occurred in Subversion revision r10909.

The fix is to change the default white-space attribute for the <button> tag in
html4.css from 'normal' to 'pre' to match the other button-like items in the
stylesheet.

Included in the patch is a fix to a misleading comment (which doesn't match the
code).

One layout test also changed slightly: fast/forms/button-in-forms-collection.

Taking this bug.
Comment 19 David Kilzer (:ddkilzer) 2006-01-19 22:48:53 PST
Created attachment 5793 [details]
Image v1
Comment 20 David Kilzer (:ddkilzer) 2006-01-20 04:41:59 PST
Hrm...should probably mention the Radar bug number in the log message as well.
Comment 21 Maciej Stachowiak 2006-01-21 12:35:15 PST
I don't think this fix is correct. Buttons do not appear to have white-space: pre in other browsers. I tried this test case and both buttons looked the same in all browsers:

<button>test button</button><br>
<button>  test  button  </button>

However, buttons don't appear to force white-space: normal, instead they inherit the containing white-space mode of their container. For instance, in this case, the extra spaces do show up in Firefox, but not in ToT Safari:

<pre>
<button>test button</button><br>
<button>  test  button  </button>
</pre>

Therefore I think the right fix is to remove the white-space property for button entirely.
Comment 22 David Kilzer (:ddkilzer) 2006-01-22 00:22:26 PST
(In reply to comment #21)
> However, buttons don't appear to force white-space: normal, instead they
> inherit the containing white-space mode of their container. For instance, in
> this case, the extra spaces do show up in Firefox, but not in ToT Safari:
> 
> <pre>
> <button>test button</button><br>
> <button>  test  button  </button>
> </pre>

Which version of Firefox are you using?  I'm using 1.5 (Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051111 Firefox/1.5), but the above two buttons look identical (no extra whitespace).
Comment 23 David Kilzer (:ddkilzer) 2006-01-22 00:39:51 PST
Created attachment 5827 [details]
Patch v2

Updated patch per Comment #21, but see also Comment #22.

Also added more HTML to test case per Comment #21.
Comment 24 David Kilzer (:ddkilzer) 2006-01-22 00:47:27 PST
Created attachment 5828 [details]
Test Image Expected v2
Comment 25 David Kilzer (:ddkilzer) 2006-01-22 12:49:42 PST
Created attachment 5846 [details]
Proposed layout test
Comment 26 Maciej Stachowiak 2006-01-22 14:34:58 PST
Comment on attachment 5827 [details]
Patch v2

r=me

Please add some more explanation of what the test case is testing however (for instance mention the bug number and that it is checking white-space mode for the <button> tag).
Comment 27 Darin Adler 2006-01-22 15:17:05 PST
Comment on attachment 5827 [details]
Patch v2

Dave will do another round of revision on it the layout test and get reviewed again.
Comment 28 David Kilzer (:ddkilzer) 2006-01-22 15:47:48 PST
Created attachment 5852 [details]
Patch v3

Updated layout test with more information per Comment #26.  Updated ChangeLog entries.  Revised *-expected.* files.  New expected PNG soon.
Comment 29 David Kilzer (:ddkilzer) 2006-01-22 15:48:57 PST
Created attachment 5853 [details]
Test Image Expected v3
Comment 30 David Kilzer (:ddkilzer) 2006-01-22 16:12:09 PST
Created attachment 5855 [details]
Patch v4

Fixed HTML of layout test.  Made sure expected image is correct, too.
Comment 31 David Kilzer (:ddkilzer) 2006-01-22 16:13:13 PST
Created attachment 5856 [details]
Test Image Expected v4

Fixed expected image.
Comment 32 David Kilzer (:ddkilzer) 2006-01-22 20:26:04 PST
Verified this issue has been fixed by nightly build r12295 (commit was made in r12294).
Comment 33 Eric Seidel (no email) 2006-01-31 21:20:50 PST
Removing Regression keyword from bugs already fixed.