297537 - Gmail "rich text" composer buttons appear cut off

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Uri Bernstein]

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]

Attachment: screenshot

Comment 2

[Uri Bernstein]

Attachment: testcase

Minimal testcase. Notice that the problem is only in Quirks Mode.

Comment 3

[Martijn Wargers (dead)]

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 (dead)]

Attachment: Same testcase in standards mode

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

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]

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

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]

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 [:bzbarsky]]

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

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

http://weblogs.mozillazine.org/roc/archives/2005/06/displayinline_a.html

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

... and then I'll have to fix this bug for real.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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)]

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-5, no longer working on Mozilla)]

Setting available height based on bands is definitely broken.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

This fixes the testcases, and gmail too.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Checked in.