Last Comment Bug 297537 - Gmail "rich text" composer buttons appear cut off
: Gmail "rich text" composer buttons appear cut off
Status: RESOLVED FIXED
: regression, testcase
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Robert O'Callahan (:roc) (email my personal email if necessary)
:
: Jet Villegas (:jet)
Mentors:
http://gmail.com
Depends on:
Blocks: 135994 276602
  Show dependency treegraph
 
Reported: 2005-06-13 03:56 PDT by Uri Bernstein (Google)
Modified: 2005-06-16 19:49 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
screenshot (35.54 KB, image/png)
2005-06-13 03:57 PDT, Uri Bernstein (Google)
no flags Details
testcase (463 bytes, text/html)
2005-06-13 04:00 PDT, Uri Bernstein (Google)
no flags Details
Same testcase in standards mode (517 bytes, application/xhtml+xml)
2005-06-13 05:33 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
testcase that fails in standards mode (361 bytes, text/html)
2005-06-16 16:29 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
fix (7.49 KB, patch)
2005-06-16 16:37 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
dbaron: review+
dbaron: superreview+
asa: approval1.8b3+
Details | Diff | Splinter Review

Description Uri Bernstein (Google) 2005-06-13 03:56:22 PDT
On the Gmail compose page, the "rich text" buttons appear below where they
should, and are mostly obscured by the main textbox.

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050611
Firefox/1.0+

This WFM on Gecko 1.7 => regression.

Screenshot and minimal testcase coming up.
Comment 1 Uri Bernstein (Google) 2005-06-13 03:57:16 PDT
Created attachment 186077 [details]
screenshot
Comment 2 Uri Bernstein (Google) 2005-06-13 04:00:03 PDT
Created attachment 186078 [details]
testcase

Minimal testcase. Notice that the problem is only in Quirks Mode.
Comment 3 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-06-13 05:32:02 PDT
I don't see the bug with the url, but I can see the bug with the testcase.
This regressed between 2005-03-06 and 2005-03-07.
I've applied the patch from bug 276602 in a build that didn't have this bug and
afterwards I did have this bug.
Comment 4 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-06-13 05:33:00 PDT
Created attachment 186088 [details]
Same testcase in standards mode
Comment 5 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-06-14 22:06:28 PDT
We build a *very* strange frame tree in this testcase. Here's a teaser:

              Block(div)(1)@0x1a08fb8 next=0x1a205d0 {0,0,8910,90}
[state=00000018] [overflow=0,0,8910,360] sc=0x1a08f68(i=1,b=0)<
                line 0x1a20590: count=5 state=inline,clean,prevmarginclean,not
impacted,not wrapped,before:nobr,after:nobr[0x14100] {15,15,4455,60}
ca={15,15,4455,345} <
                  Text(0)@0x1a090e8[0,9,T]  next=0x1a1e370 {15,75,0,0}
[state=51100020] sc=0x1a09050 pst=:-moz-non-element<
                    "\n        "
                  >
                  Placeholder(table)(1)@0x1a1e370 {15,75,0,0}
outOfFlowFrame=TableOuter(table)(1)@0x1a1dfd8
                  Text(2)@0x1a1f418[0,10,T]  next=0x1a1f5f8 {15,75,0,0}
[state=51100020] sc=0x1a09050 pst=:-moz-non-element<
                    "\n\n        "
                  >
                  Inline(table)(3)@0x1a1f5f8 next=0x1a20530 {15,15,4455,60}
[state=00000008] [content=0xef6ba0] [overflow=0,0,4455,345] [sc=0x1a1f4c8]<
                    Text(0)@0x1a1f6f0[0,13,T]  next=0x1a1f920 {0,60,0,0}
[state=51100020] sc=0x1a1f658 pst=:-moz-non-element<
                      "\n            "
                    >
                    TableOuter(table)(3)@0x1a1f920 {0,0,4455,60}
[state=00010008] [content=0xef6ba0] [overflow=0,0,4455,345] [sc=0x1a1f9a0]
pst=:-moz-table-outer<
                      Table(table)(3)@0x1a1fb80 {0,0,4455,60} [state=00010008]
[content=0xef6ba0] [overflow=0,0,4455,345] [sc=0x1a1f888] pst=:-moz-table<
                        TableRowGroup(tbody)(1)@0x1a1fc88 {30,30,4395,0}
[state=00010008] [content=0x1a00820] [overflow=0,0,4395,315] [sc=0x1a1f750]<

Note that the table has ended up as a child of an inline. That's very wrong. We
should have triggered an IB reconstruction and put the table on its own line.
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-06-14 22:12:05 PDT
We actually build this for both testcases, but somehow this actually works OK in
standards mode.

I think what should be happening in both modes is equivalent to replacing <table
style="display:inline"> with <span>: it becomes an inline element, and we ignore
the <td>s and <tr>s inside it.
Comment 7 Bernd 2005-06-14 22:15:32 PDT
Oh, I have seen this frame tree very often before it is like this always when
display:inline on a table is specified. I thought thats OK. 
Comment 8 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-06-14 22:50:15 PDT
Okay, what's happening here is that we create an inline frame for the <table>
(good), then we create anonymous tableouter/table frames for the <tbody> that
the parser generated (okay), and we treat those frames as if they were
inline-table (which we don't support); this violates the CSS 2.1 spec which
clearly says anonymous table boxes should be 'table' (not 'inline-table'). So
there's one bug, although we might need to stick to our existing behaviour in
quirks mode, because according to bernd this is intentional on our part.

If we accept that we need limited inline-table support here, then we need to fix
this bug with the frame tree we've got. I'll look at that tomorrow.
Comment 9 Uri Bernstein (Google) 2005-06-15 00:13:02 PDT
BTW, if you want to see the bug on Gmail on Windows, use ctrl-minus to reduce
the font size. The bug only appears if the floated (first) table is small enough
- which happens to be true on Mac with the default font size, but not on Windows.
Thanks to Jomel who figured this out.

Changing to All/All.
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2005-06-15 05:38:16 PDT
It's "intentional" as in "when IB splits were implemented, waterson probably
didn't catch the fact that table frame construction uses none of the standard
frame construction codepaths".

I believe that the fact that we don't create an IB split here is in fact bug
135994 and we need to fix it.
Comment 11 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-06-15 18:23:11 PDT
I have a fix for bug 135994. Gmail doesn't work with the fix. There are five
sets of buttons; each set is given as a table with display:inline, so each set
of buttons appears on its own line. I believe this is correct as per the CSS
spec. So what to do? Do we land the fix and get GMail and possibly other sites
to fix themselves? Or do we do some quirk thing?
Comment 12 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-06-15 18:55:50 PDT
http://weblogs.mozillazine.org/roc/archives/2005/06/displayinline_a.html
Comment 13 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-06-15 22:56:47 PDT
I'm convinced that we need to keep supporting this. Basically the spec should be
amended so that the anonymous table frame becomes an inline-table. If the spec
cannot be so amended, then we'll have to make it a quirk. I will hook up
nsCSSFrameConstructor to support inline-table and make the anonymous table frame
an inline-table, probably by introducing a new nsCSSAnonBoxes psuedoelement.
Comment 14 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-06-15 22:57:21 PDT
... and then I'll have to fix this bug for real.
Comment 15 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-06-16 16:29:27 PDT
Created attachment 186528 [details]
testcase that fails in standards mode

This is a further simplified testcase that fails in standards mode. I've made
it more clear what's really going on here.

The bug is that nsLineLayout sets the availableHeight of the frames it reflows
to be the height of the "band" that it's currently using. This band is a
rectangle representing the currently available space for laying out text
between floats. In this testcase the first band is only 1 pixel high. The table
frame sees that its height is constrained to one pixel and tries to split
itself as if there's a page break there, but of course that doesn't work right
because we're not really in a constrained height situation.

nsLineLayout should probably just set the availableHeight to unconstrained,
always. If the line doesn't fit on the page it will be moved to the next page.
If we start getting into situations when inline-blocks and inline-tables need
to split across pages, we can deal with that then. It's not immediately obvious
how to handle that.
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-06-16 16:32:36 PDT
I believe that currently no genuine inline frames actually use the available
height for anything, otherwise we'd have seen problems here before.

Also note that although this is a regression from bug 276602, that's only
because 276602 made zero-width floats start to affect the band setup (which is
correct). I think attachment 186528 [details] would have failed on builds before that.
Comment 17 David Baron :dbaron: ⌚️UTC-10 (vacation, returning December 19) 2005-06-16 16:35:38 PDT
Setting available height based on bands is definitely broken.
Comment 18 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-06-16 16:37:52 PDT
Created attachment 186530 [details] [diff] [review]
fix

This fixes the testcases, and gmail too.
Comment 19 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-06-16 16:54:42 PDT
Comment on attachment 186530 [details] [diff] [review]
fix

Fixes a gmail layout issue. Pretty low risk.
Comment 20 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-06-16 19:46:48 PDT
Checked in.

Note You need to log in before you can comment on or make changes to this bug.