293761 - Layout screw-up after clicking on "Attach a file" at gmail

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

To reproduce, you need to have an account at Gmail (I can provide one, if asked).

- Click on "Compose Mail". Layout looks normal.
- Click on "Attach a file". Layout is "wrong". 

I get a needless horizontal scrollbar and the height of the textarea becomes
much smaller. I'll attach a screenshot.
Unfortunately, I haven't been able to make a testcase :(.

It doesn't happen with a 2005-04-28 build, but it does happen with a 2005-04-29
build, hence my guess that the fix for bug 240276 is to blame.

Comment 1

[Martijn Wargers (dead)]

Attachment: Screenshot of affected area in gmail

Comment 2

[Martijn Wargers (dead)]

Attachment: Not minimised testcase

Ok, I have been able to at least isolate the gmail code in this absolutely not
yet minimised case. The minimising still needs to happen.
See this js code that gets called when clicking on "Attach a file":
document.getElementById('fc').style.height='99.9%';
id="fc" is the parent table (with table-layout:fixed).

Comment 3

[Oliver Saier]

Clicking "rich formatting" then back to "plain text" makes the text area 3 lines
height. clicking "check spelling" restores the correct height.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050513
Firefox/1.0+ ID:2005051306

Comment 4

[Boris Zbarsky [:bzbarsky]]

Looks like fallout from roc's scrollframe changes....

Comment 5

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

Attachment: minimzed testcase

This testcase demonstrates a bug, and I'm betting it's the same one as the big
testcase. It looks like table reflow issue to me. In the inital reflow the
100%-height DIV is not getting its height set correctly. If you resize the
window it gets reflowed again with the correct height.

This might have been revealed by the scrollframe changes since in some cases we
reflow the main document less often that we used to.

Comment 6

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

also, if you remove "table-layout:fixed", the bug goes away.

Comment 7

[Bernd]

The testcase looks like a clash between table-layout:fixed and the percent
height observer in quirks mode. 

Comment 8

[Bernd]

In other words: bug 178762?

Comment 9

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

Probably. Can you fix it?

Comment 10

[Bernd]

Attachment: patch

Robert, I can't fix it, but I can hack it. Fixing would require understanding
the pct observer voodoo. The last guy how did truly understand this did leave
years ago.

Comment 11

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

Does it fix the gmail issue?

Comment 12

[Bernd]

I can't reproduce the gmail problem even without the patch

Comment 13

[Martijn Wargers (dead)]

I just tried Bernd's patch, but it doesn't fix this issue (although it seems to
fix bug 178762).

Comment 14

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

Martijn: which testcases in this bug does it fix, for you?

Comment 15

[Martijn Wargers (dead)]

It fixes "minimzed testcase", but not "Not minimised testcase".

Comment 16

[Bernd]

Martijn, it looks like we can't proceed without a reduced testcase with a layout
breakpoint. ;-), But what the hell they are doing??, I have never seen the dom
inspector to fail so completely.

Comment 17

[Martijn Wargers (dead)]

Attachment: testcase3

This is sort of a minimal testcase.

Bernd, with this testcase you should be able to reproduce. The bug relies on
the existence of a vertical scrollbar (which you don't get with the previous
testcase of me, when you have a large monitor)

Up to (and including) 2005-01-24 build Mozilla only experiences bug 178762 with
this testcase.
From 2005-01-25 build up to (and including) 2005-04-28 build I also get a
unneeded and disabled horizonal scrollbar when clicking on "Clicking on this
text should not change anything".
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-01-24+08%3A00%3A00&maxdate=2005-01-25+10%3A00%3A00&cvsroot=%2Fcvsroot

My guess the bug responsible for this behavior: bug 269566 or bug 219444.

From 2005-04-29 build on I experience this bug behavior with the testcase.

Comment 18

[Bernd]

hmm 1600 x 1200 is a decent monitor ;-). The horizontal scrollbar is a result of
table row group frames reporting UC as part of the overflow. I see the problem
now with  testcase3. 

I guess we should assert on UC in the overflow area as we do on emptyness.

Comment 19

[chris hofmann]

lets be sure to get this in b3

Comment 20

[Wolf [:wolf]]

*** Bug 296535 has been marked as a duplicate of this bug. ***

Comment 21

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

bernd, does this patch fix testcase3? 

Comment 22

[Bernd]

I think it fixes the testcase3, Martijn does it fix for you the testcase3? (you
know I am bug blind)

Comment 23

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050608
Firefox/1.0+ ID:2005060811
For me, testcase3 is fixed, in that clicking the link does not change anything.
But as soon as I resize the browser window, the page changes its layout.
The same goes for 'minimized testcase' - no changes when the page is loaded
whilst clicking; but as soon as I resize the browser window its layout changes.
Don't know if that is a different bug from the 'Click on this link to get a
reflow' testcase, but I thought i'd mention it anyway.

Comment 24

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050608
Firefox/1.0+ ID:2005060823
  
testcase 3 fails for me

clicking "Clicking on this text should not change anything" creates a horizontal
scrollbar of an "infinitly" wide <div> (after 100 clicks on the scrollbar the
slider had barely moved a pixel)

Comment 25

[José Jeria]

Testcase 3:
1. click on the link, nothing changes
2. click on the text above the text, scrollbars appear

Comment 26

[Steve England [:stevee]]

re: comment 23
I do not do my own builds, it was a pacifica build I downloaded. Sorry if I
mis-understood that the patch was not included in any tinderbox build.

Comment 27

[Martijn Wargers (dead)]

(In reply to comment #22)
> I think it fixes the testcase3, Martijn does it fix for you the testcase3? (you
> know I am bug blind)
No, the patch does not fix testcase3. 
To reproduce the bug in testcase3, you should only have to click on the
"Clicking on this text should not change anything" text.

Comment 28

[Bernd]

Martijn, are you sure that you tested the patch?
With the patch the tables covers 400px and does not change whether the window is
resized or the link is hit, without the table height looks like auto and goes to
400px on window resize.

Comment 29

[Martijn Wargers (dead)]

(In reply to comment #28)
> Martijn, are you sure that you tested the patch?
Yes, I'm sure (in fact I tested it already in comment 13)
> With the patch the tables covers 400px and does not change whether the window is
> resized or the link is hit, without the table height looks like auto and goes to
> 400px on window resize.
Yes, that's what I'm seeing also with the patch, so the patch works all right
and fixes bug 178762, but not this bug. 
You should see the bug, when you click on the text just above the link (the text
called "Clicking on this text should not change anything").

Comment 30

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

Comment on attachment 183744 [details] [diff] [review]
patch

I hope you guys can sort this out...

Comment 31

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

Setting correct assignment

Comment 32

[Bernd]

Attachment: patch

The patch fixes the issue in testcase3, for me at least, Martijn does it do
same for you?

Comment 33

[Bernd]

Comment on attachment 186676 [details] [diff] [review]
patch

>I hope you guys can sort this out...
I dont think that you sneak out of your review duties ;-)

First review hint:

+      nextReason = eReflowReason_Resize;

There is 3rd pass reflow
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/tables/nsTableFrame.
cpp&mark=1788-1840#1780
for tables. The comment discusses only auto layout tables. The testcase
requires this special height reflow to give the pct. constrained stuff inside
auto height table cells the opportunity to get a meaningfull pct height. This
happens only in quirks mode, in standards mode all those pct heights would be
treated as auto. 

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/tables/nsTableFrame.
cpp&mark=1896,1905,1915,1916,1926#1888
For auto tables a style change reflow will cause first a unconstrained reflow,
then a resize reflow and then the height reflow. Now for fixed layout table
there will be no unconstrained reflow. The layout strategy is based just on the
style info. So the previous second reflow will now be the first reflow but with
a style change reason and width constrained. The next thing would be a resize
reflow to give the cell children a opportunity to resize based on the pct info.
Without the patch there will be no second reflow as the necessity of such
reflow was computed before the style change reflow. So one needs to reevaluate
the necessity. Further more this reflow should be resize and not again a style
change reflow.

Comment 34

[Bernd]

clickable links...

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/tables/nsTableFrame.cpp&mark=1788-1840#1780

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/tables/nsTableFrame.cpp&mark=1896,1905,1915,1916,1926#1888

Comment 35

[Martijn Wargers (dead)]

(In reply to comment #32)
> The patch fixes the issue in testcase3, for me at least, Martijn does it do
> same for you?
Yes, the patch works for testcase3 as for gmail. And it also seems to fix bug
178762.

Comment 36

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

Comment on attachment 186676 [details] [diff] [review]
patch

I think you can put
+    willInitiateSpecialReflow = ((NeedToInitiateSpecialReflow() ||
InitiatedSpecialReflow()) && 
+				  (aReflowState.mFlags.mSpecialHeightReflow ||
!NeedSpecialReflow()));
inside the previous if().

Comment 37

[Bernd]

Comment on attachment 186676 [details] [diff] [review]
patch

I rtested the patch the risk is medium but it fixes a prominent regression.

Comment 38

[Bernd]

fix checked in