30.40 KB, image/gif
207 bytes, text/html
937 bytes, text/html
1.71 KB, patch
|Details | Diff | Splinter Review|
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.
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).
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
Looks like fallout from roc's scrollframe changes....
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.
also, if you remove "table-layout:fixed", the bug goes away.
The testcase looks like a clash between table-layout:fixed and the percent height observer in quirks mode.
In other words: bug 178762?
Probably. Can you fix it?
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.
Does it fix the gmail issue?
I can't reproduce the gmail problem even without the patch
I just tried Bernd's patch, but it doesn't fix this issue (although it seems to fix bug 178762).
Martijn: which testcases in this bug does it fix, for you?
It fixes "minimzed testcase", but not "Not minimised testcase".
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.
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.
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.
lets be sure to get this in b3
*** Bug 296535 has been marked as a duplicate of this bug. ***
bernd, does this patch fix testcase3?
I think it fixes the testcase3, Martijn does it fix for you the testcase3? (you know I am bug blind)
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.
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)
Testcase 3: 1. click on the link, nothing changes 2. click on the text above the text, scrollbars appear
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.
(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.
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.
(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 on attachment 183744 [details] [diff] [review] patch I hope you guys can sort this out...
Setting correct assignment
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 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.
(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 on attachment 186676 [details] [diff] [review] patch I think you can put + willInitiateSpecialReflow = ((NeedToInitiateSpecialReflow() || InitiatedSpecialReflow()) && + (aReflowState.mFlags.mSpecialHeightReflow || !NeedSpecialReflow())); inside the previous if().
Comment on attachment 186676 [details] [diff] [review] patch I rtested the patch the risk is medium but it fixes a prominent regression.
fix checked in