Bug 5656 – REGRESSION: Buttons on Yahoo! Mail misplaced in ToT

• Component: Layout and Rendering
• Status: VERIFIED FIXED

[Samuel Allen]

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.

[Samuel Allen]

Created attachment 4623 [details]
Screenshot of top of Yahoo! Mail

[Dave Hyatt]

What do they look like in Firefox?

[Samuel Allen]

Created attachment 4624 [details]
Firefox screenshot (correct rendering)

[Dave Hyatt]

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.

[Samuel Allen]

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.

[Samuel Allen]

Created attachment 4717 [details]
A reduced testcase for Yahoo! Mail bug

This works on all browsers except ToT.

[Samuel Allen]

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.

[Samuel Allen]

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.

[Samuel Allen]

Created attachment 4720 [details]
Minimized testcase

Works correctly in Firefox and Safari, not on ToT.

[Eric Seidel (no email)]

The test case looks like ot works correclty.  Or?

[Joost de Valk (AlthA)]

No it doesn't, buttons should be next to each other. Reopening.

[Eric Seidel (no email)]

They appear next to each other to me [search mail] [search the web] ?

[Joost de Valk (AlthA)]

They don't to me, and this is latest ToT...

[David Kilzer (:ddkilzer)]

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!

[Alice Liu]

<rdar://problem/4404335>

[David Kilzer (:ddkilzer)]

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!

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

Created attachment 5793 [details]
Image v1

[David Kilzer (:ddkilzer)]

Hrm...should probably mention the Radar bug number in the log message as well.

[Maciej Stachowiak]

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.

[David Kilzer (:ddkilzer)]

(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).

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

Created attachment 5828 [details]
Test Image Expected v2

[David Kilzer (:ddkilzer)]

Created attachment 5846 [details]
Proposed layout test

[Maciej Stachowiak]

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).

[Darin Adler]

Comment on attachment 5827 [details]
Patch v2

Dave will do another round of revision on it the layout test and get reviewed again.

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

Created attachment 5853 [details]
Test Image Expected v3

[David Kilzer (:ddkilzer)]

Created attachment 5855 [details]
Patch v4

Fixed HTML of layout test.  Made sure expected image is correct, too.

[David Kilzer (:ddkilzer)]

Created attachment 5856 [details]
Test Image Expected v4

Fixed expected image.

[David Kilzer (:ddkilzer)]

Verified this issue has been fixed by nightly build r12295 (commit was made in r12294).

[Eric Seidel (no email)]

Removing Regression keyword from bugs already fixed.