Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED

Status

()

Core
Layout
--
major
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: Bernd)

Tracking

({regression})

Trunk
x86
Windows 2000
regression
Points:
---
Bug Flags:
blocking1.8b3 +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

12 years ago
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.
(Reporter)

Comment 1

12 years ago
Created attachment 183286 [details]
Screenshot of affected area in gmail
(Reporter)

Comment 2

12 years ago
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

12 years ago
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

12 years ago
Looks like fallout from roc's scrollframe changes....
Assignee: nobody → roc
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.
(Assignee)

Comment 7

12 years ago
The testcase looks like a clash between table-layout:fixed and the percent
height observer in quirks mode. 
(Assignee)

Comment 8

12 years ago
In other words: bug 178762?
Probably. Can you fix it?
(Assignee)

Comment 10

12 years ago
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?
(Assignee)

Comment 12

12 years ago
I can't reproduce the gmail problem even without the patch
(Reporter)

Comment 13

12 years ago
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?
(Reporter)

Comment 15

12 years ago
It fixes "minimzed testcase", but not "Not minimised testcase".
(Assignee)

Comment 16

12 years ago
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.
(Reporter)

Comment 17

12 years ago
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.
Attachment #183329 - Attachment is obsolete: true
(Assignee)

Comment 18

12 years ago
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.

Flags: blocking1.8b3?

Comment 19

12 years ago
lets be sure to get this in b3
Flags: blocking1.8b3? → blocking1.8b3+

Comment 20

12 years ago
*** Bug 296535 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

12 years ago
Attachment #183744 - Flags: superreview?(roc)
Attachment #183744 - Flags: review?(roc)
bernd, does this patch fix testcase3? 
(Assignee)

Comment 22

12 years ago
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)

Comment 25

12 years ago
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.
(Reporter)

Comment 27

12 years ago
(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.
(Assignee)

Comment 28

12 years ago
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.
(Reporter)

Comment 29

12 years ago
(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...
Attachment #183744 - Flags: superreview?(roc)
Attachment #183744 - Flags: review?(roc)
Setting correct assignment
Assignee: roc → bernd_mozilla
(Assignee)

Comment 32

12 years ago
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?
Attachment #183744 - Attachment is obsolete: true
(Assignee)

Comment 33

12 years ago
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.
Attachment #186676 - Flags: superreview?(roc)
Attachment #186676 - Flags: review?(roc)
(Assignee)

Comment 34

12 years ago
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
(Reporter)

Comment 35

12 years ago
(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().
Attachment #186676 - Flags: superreview?(roc)
Attachment #186676 - Flags: superreview+
Attachment #186676 - Flags: review?(roc)
Attachment #186676 - Flags: review+
(Assignee)

Comment 37

12 years ago
Comment on attachment 186676 [details] [diff] [review]
patch

I rtested the patch the risk is medium but it fixes a prominent regression.
Attachment #186676 - Flags: approval1.8b3?

Updated

12 years ago
Attachment #186676 - Flags: approval1.8b3? → approval1.8b3+
(Assignee)

Comment 38

12 years ago
fix checked in
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.