617076 - Large white space under Add-on Manager when switching tabs and panes

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Nochum Sossonko [:Natch]]

From bug 571970 comment 53:

With the Add-on Manager open the status bar will suddenly show when I move from pane to pane in the Add-on Manager. Very strange interaction, STR:

1) Open Add-on Manager.
2) Open a blank tab (with Ctrl-T) (open, don't switch to)
3) Switch to (with the mouse) the tab with the Add-on Manager
4) Change panes (with the mouse) within the Add-on Manager (e.g. go from "Get Add-ons" to
"Extensions").

Results in a large white bottom bar (actually larger than the statusbar) across
the bottom of the page.

Mozilla/5.0 (Windows NT 6.0; rv:2.0b8pre) Gecko/20101202 Firefox/4.0b8pre

Comment 1

[Dave Townsend [:mossop]]

How do you open a blank tab without switching to it?

Comment 2

[Nochum Sossonko [:Natch]]

Ugh, that's misleading...I meant to emphasize that a new tab needs to be created and that you can't just switch to an already existent tab. Really would have been easier to just write "Hit Ctrl-T" as the 2nd step.

Comment 3

[Nochum Sossonko [:Natch]]

New STR:

1) Open Add-On Manager (via the Firefox Button Menu).
2) Change panes to "Appearance" (via click on pane).
3) Open new tab (via Ctrl-T)
4) Switch back to Add-On Manager tab (via click on tab)
5) Change panes to "Extensions" (via click on pane).

These exact STR do it every time for me. Do it when you first open Firefox (which is how I was testing).

Comment 5

[Sean Smith]

I can confirm the STR in comment 3.

Mozilla/5.0 (Windows NT 5.1; rv:2.0b8pre) Gecko/20101209 Firefox/4.0b8pre

Comment 6

[Johnathan Nightingale [:johnath]]

Blair - can you diagnose this and move it to layout or wherever it needs to go, if necessary?

Comment 7

[Alan Thomas]

Attachment: screenshot of bug

Here's a screenshot. (Note that I am running Windows 7 at 120 dpi.)

Also, it's not possible to select the white space using the DOM Inspector's or Firebug's "Select Element By Click" tool - it's not even part of the root <page> element in about:addons. However, it appears to be part of the <browser> in browser.xul. Very strange.

Comment 8

[Alice0775 White]

This also happens on Mozilla/5.0 (X11; Linux i686; rv:2.0b9pre) Gecko/20101218 Firefox/4.0b9pre ID:20101218030347

[STR]
1. Open Add-On Manager (Ctril+Shift+A) and any pages in new tab
2. Switch to tab other than Add-ons Manager and wait for 5 seconds
3. Switch back to Add-On Manager tab 
4. Mouse hover Back/Forward-button or Pane-Label and wait till tooltip pops up

And the problem happens  after step 2-4

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The white area at the bottom of the AOM is exactly the same height of the visible toolbars in the other tabs. You can vary it by enabling disabling of those (i.e. bookmarks toolbar, navigation toolbar).

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Oops, thought I moved this over before going on holiday. AFAICT, this is indeed a layout problem. There's no DOM there - as Henrik says, the blank area is the same height of the toolbars that are hidden, and for some reason the browser element isn't always getting resized.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Probably related to the hacks in deck frame (and the view manager) for tabbrowser resizing.  (I'm assuming this is using a <deck>.)  Probably something doesn't work right when decks are nested.

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #11)
> (I'm assuming this is using a <deck>.)

You assume correctly.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It's definitely related to the delayed resize handling; if I hack nsViewManager::SetWindowDimensions to never set delayed resizes (i.e., make the inner if always be true), then the bug goes away.

Comment 14

[Mats Palmgren (inactive)]

Attachment: assertion stack

The STR triggers:
###!!! ASSERTION: UpdateView called on view we don't own: 'view->GetViewManager() == this', file view/src/nsViewManager.cpp, line 678

The view at this point is for the root frame (Viewport) and the
height shrinks by 35px leading to the gap (apparently as a result
of a reflow).

Comment 15

[Mats Palmgren (inactive)]

Attachment: part 1

This fixes the assertion, but not the bug.

Comment 16

[Timothy Nikkel (:tnikkel)]

(In reply to comment #15)
> Created attachment 505346 [details] [diff] [review]
> part 1
> 
> This fixes the assertion, but not the bug.

Yeah, that assertion will only cause problems if the two view manager's involved are at different full zoom levels. We should still fix the assertion though.

Comment 17

[Timothy Nikkel (:tnikkel)]

Comment on attachment 505346 [details] [diff] [review]
part 1

Unsolicited feedback+ :).

Comment 18

[Timothy Nikkel (:tnikkel)]

Actually thinking back now I think it was intentional that I didn't call Resize on the parent view's view manager. I think it is because we should only be changing the size of the root view via SetWindowDimensions (and I think now via some displayport things for Fennec). And I think that might explain the white gap in this case: changing the root view size without going through SetWindowDimensions.

Comment 19

[Mats Palmgren (inactive)]

The problem is that the PresContext::mVisibleArea is out-of-sync with
the root view size, we get into that state by:

(0. correct state: root view and PresContext is 1241,1141 px)
1. call to SetWindowDimensions(1241,1106) -- this is DELAYED
2. the refresh driver calls FlushDelayedResize which propagates
   1241,1106 to the PresContext
3. call to SetWindowDimensions(1241,1141) which is NOT DELAYED --
   this clears mDelayedResize and since "oldDim.IsExactEqual(newDim)"
   is true in DoSetWindowDimensions, it doesn't trigger a reflow
4. the refresh driver triggers a reflow, the PresContext size is still
   1241,1106  ==> BUG

Comment 20

[Mats Palmgren (inactive)]

Attachment: part 2, fix + testcase

The test does trigger the bug state, but I don't see how
it can be probed from JS.  I'm including it anyway in case
it triggers some future error/assertion.

Comment 21

[Timothy Nikkel (:tnikkel)]

Comment on attachment 505346 [details] [diff] [review]
part 1

Actually, I'm not sure if we should take this. The assert was correctly pointing out a problem, just not quite the problem we thought.

We should add an assert to ResizeView that we aren't resizing a root view though.

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

(Hardblocker since this is likely due to the hide/show of chrome, and I can think of a bunch of other likely STR that might hit it)

Comment 23

[Timothy Nikkel (:tnikkel)]

Probably caused by http://hg.mozilla.org/mozilla-central/rev/386d56a5167f (part 1) of bug 575336.

Comment 24

[Michael Yoshitaka Erlewine [:mitcho]]

I believe bug 612232 may also be an instance of this.

Comment 25

[Mats Palmgren (inactive)]

(In reply to comment #18)
> changing the root view size without going through SetWindowDimensions.

I have verified this isn't what happens in this bug.
(We could add a DEBUG bit to nsViewManager, set it during
DoSetWindowDimensions and assert it's set in nsView::SetDimensions
for root views...)

(In reply to comment #21)
> We should add an assert to ResizeView that we aren't resizing a root view
> though.

Good point.  That actually happens all the time though...

Comment 26

[Mats Palmgren (inactive)]

Attachment: part 1 v2, don't ResizeView root views + assert parent's VM is 'this' (paranoid)

Comment 27

[Mats Palmgren (inactive)]

TryServer builds passed unit tests:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-5afbdd8a3772/

Comment 28

[Timothy Nikkel (:tnikkel)]

Comment on attachment 505640 [details] [diff] [review]
part 2, fix + testcase

Would it be better to always unconditionally call ResizeReflow in DoSetWindowDimensions and have ResizeReflow check if the size is actually different?

Comment 29

[Bill Gianopoulos [:WG9s]]

Perhaps I am stupid, but I really fail to see how something as trivial as this is a hardblocker, while bug 585946 is not even a softblocker.  I realize there is some personal judgement involved here, but these decisions seem very arbitrary to me.

Comment 30

[Timothy Nikkel (:tnikkel)]

Comment on attachment 505640 [details] [diff] [review]
part 2, fix + testcase

The changes from bug 575336 disconnect the view manager's notion of the root view's size and the prescontext's notion of the size of the viewport. This patch tries to use the information that the view manager knows about the prescontext's size to do the right thing. But I think trying to figure out what is going on in the prescontext from the viewmanager is going to be confusing. This patch is certainly confusing. Is comment 28 feasible so we don't have to have as much confusion going on here?

Comment 31

[Mats Palmgren (inactive)]

(In reply to comment #28)
> Would it be better to always unconditionally call ResizeReflow in
> DoSetWindowDimensions and have ResizeReflow check if the size is actually
> different?

That looks like a risky change at this stage.
* there are other callers to ResizeReflow
* any call to PresContext::SetVisibleArea potentially disables the next
  ResizeReflow.  nsViewManager::FlushDelayedResize calls SetVisibleArea.

I think the current patch is safer wrt subtle regressions.

We could try what you suggest in a followup bug post-4.0 ?

Comment 32

[Timothy Nikkel (:tnikkel)]

Can we make a special ResizeReflow function that only the view manager calls that checks if the size is different first before calling the normal ResizeReflow?

Comment 33

[Mats Palmgren (inactive)]

I tried your first suggestion and found a regression quite quickly
1. start with multiple tabs
2. resize the window
3. select a different tab  ==> ResizeReflow is called but bails because
     it's the same size as mPresContext->GetVisibleArea()

PresShell::ResizeReflow
nsViewManager::DoSetWindowDimensions
nsViewManager::FlushDelayedResize
PresShell::FlushPendingNotifications
nsViewManager::CallWillPaintOnObservers
nsViewManager::DispatchEvent
...

It seems to me that we would need a "dirty bit" somewhere to know if
we have actually reflowed at the given size.  Or, if we separate the
nsViewManager's call from the rest, make the non-nsViewManager ResizeReflow
start by doing nsViewManager::FlushDelayedResize ?
I don't think we should make risky changes like this before the release.

My fix is only using nsViewManager's own internal delayed-resizing state.
It's nsViewManager's own fault that ResizeReflow isn't called.
(see comment 19)

Comment 34

[Mats Palmgren (inactive)]

Attachment: part 2 (alternative)

Alternatively, instead of the FlushDelayedResize(PR_FALSE) I added
we could propagate this fact to DoSetWindowDimensions and make it call
ResizeReflow unconditionally in this specific case.

Comment 35

[Timothy Nikkel (:tnikkel)]

Comment on attachment 505640 [details] [diff] [review]
part 2, fix + testcase

Ok. After thinking about it more I'm fine with this.

Comment 36

[Timothy Nikkel (:tnikkel)]

Comment on attachment 506163 [details] [diff] [review]
part 1 v2, don't ResizeView root views + assert parent's VM is 'this' (paranoid)

I don't think we want to prevent view size changes in PresShell::DoReflow. Just that if they happen there it is wrong and we want to fix whatever bug caused that. Is that the case or did you find otherwise?

Comment 37

[Mats Palmgren (inactive)]

Timothy, are you fine if I land part 2 without part 1 then?
It would be nice to get that in to beta 11.  Part 1 isn't needed to fix the
reported problem and I think we can spawn that off to its own bug.

Comment 38

[Timothy Nikkel (:tnikkel)]

Yes, that's perfectly fine.

Comment 39

[Mats Palmgren (inactive)]

Thanks, I filed bug 629823 to follow-up on part 1.

http://hg.mozilla.org/mozilla-central/rev/bfd144a54f0a

Comment 40

[Steve Scott (pxbugz)]

Verified fixed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre