Last Comment Bug 293761 - Layout screw-up after clicking on "Attach a file" at gmail
: Layout screw-up after clicking on "Attach a file" at gmail
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86 Windows 2000
: -- major with 2 votes (vote)
: ---
Assigned To: Bernd
:
: Jet Villegas (:jet)
Mentors:
http://gmail.google.com/gmail
: 296535 (view as bug list)
Depends on:
Blocks: 240276
  Show dependency treegraph
 
Reported: 2005-05-11 07:14 PDT by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2006-03-12 18:33 PST (History)
14 users (show)
chofmann: blocking1.8b3+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Screenshot of affected area in gmail (30.40 KB, image/gif)
2005-05-11 07:16 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
Not minimised testcase (15.39 KB, text/html)
2005-05-11 16:53 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
minimzed testcase (207 bytes, text/html)
2005-05-15 18:47 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
patch (1.50 KB, patch)
2005-05-16 11:47 PDT, Bernd
no flags Details | Diff | Splinter Review
testcase3 (937 bytes, text/html)
2005-05-17 06:11 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
patch (1.71 KB, patch)
2005-06-18 05:17 PDT, Bernd
roc: review+
roc: superreview+
asa: approval1.8b3+
Details | Diff | Splinter Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2005-05-11 07:14:11 PDT
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 [:mwargers] (not working for Mozilla) 2005-05-11 07:16:48 PDT
Created attachment 183286 [details]
Screenshot of affected area in gmail
Comment 2 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-05-11 16:53:05 PDT
Created attachment 183329 [details]
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 2005-05-13 10:27:09 PDT
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 [:bz] (still a bit busy) 2005-05-15 17:00:18 PDT
Looks like fallout from roc's scrollframe changes....
Comment 5 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-05-15 18:47:57 PDT
Created attachment 183686 [details]
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) 2005-05-15 18:48:40 PDT
also, if you remove "table-layout:fixed", the bug goes away.
Comment 7 Bernd 2005-05-15 21:48:36 PDT
The testcase looks like a clash between table-layout:fixed and the percent
height observer in quirks mode. 
Comment 8 Bernd 2005-05-15 21:50:09 PDT
In other words: bug 178762?
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-05-15 22:25:08 PDT
Probably. Can you fix it?
Comment 10 Bernd 2005-05-16 11:47:11 PDT
Created attachment 183744 [details] [diff] [review]
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) 2005-05-16 12:47:57 PDT
Does it fix the gmail issue?
Comment 12 Bernd 2005-05-16 13:41:33 PDT
I can't reproduce the gmail problem even without the patch
Comment 13 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-05-16 13:50:55 PDT
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) 2005-05-16 13:59:57 PDT
Martijn: which testcases in this bug does it fix, for you?
Comment 15 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-05-16 14:31:58 PDT
It fixes "minimzed testcase", but not "Not minimised testcase".
Comment 16 Bernd 2005-05-16 22:12:21 PDT
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 [:mwargers] (not working for Mozilla) 2005-05-17 06:11:23 PDT
Created attachment 183820 [details]
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 2005-05-17 09:20:01 PDT
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 2005-05-31 12:52:43 PDT
lets be sure to get this in b3
Comment 20 Wolf [:wolf] 2005-06-03 09:59:07 PDT
*** Bug 296535 has been marked as a duplicate of this bug. ***
Comment 21 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-06-08 18:58:44 PDT
bernd, does this patch fix testcase3? 
Comment 22 Bernd 2005-06-08 22:14:30 PDT
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] 2005-06-09 02:37:09 PDT
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] 2005-06-09 03:09:15 PDT
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 2005-06-09 09:47:52 PDT
Testcase 3:
1. click on the link, nothing changes
2. click on the text above the text, scrollbars appear
Comment 26 Steve England [:stevee] 2005-06-09 10:29:59 PDT
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 [:mwargers] (not working for Mozilla) 2005-06-09 15:27:23 PDT
(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 2005-06-11 10:43:14 PDT
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 [:mwargers] (not working for Mozilla) 2005-06-12 13:52:07 PDT
(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) 2005-06-14 20:14:51 PDT
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) 2005-06-16 16:39:38 PDT
Setting correct assignment
Comment 32 Bernd 2005-06-18 05:17:39 PDT
Created attachment 186676 [details] [diff] [review]
patch

The patch fixes the issue in testcase3, for me at least, Martijn does it do
same for you?
Comment 33 Bernd 2005-06-19 06:58:44 PDT
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 35 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-06-19 13:08:14 PDT
(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) 2005-06-19 15:53:41 PDT
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 2005-06-19 23:22:24 PDT
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 2005-06-20 22:42:50 PDT
fix checked in

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