Closed Bug 617076 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Layout, defect)

x86
All
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla2.0b11
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: Natch, Assigned: mats)

References

Details

(Whiteboard: [hardblocker][fx4-fixed-bugday])

Attachments

(3 files, 3 obsolete files)

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
How do you open a blank tab without switching to it?
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.
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).
Duplicate of this bug: 617482
I can confirm the STR in comment 3.

Mozilla/5.0 (Windows NT 5.1; rv:2.0b8pre) Gecko/20101209 Firefox/4.0b8pre
Component: General → Add-ons Manager
Product: Firefox → Toolkit
QA Contact: general → add-ons.manager
Blair - can you diagnose this and move it to layout or wherever it needs to go, if necessary?
Attached image 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.
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
OS: Windows Vista → All
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).
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.
Component: Add-ons Manager → Layout
OS: All → Windows Vista
Product: Toolkit → Core
QA Contact: add-ons.manager → layout
OS: Windows Vista → All
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.
Whiteboard: [hardblocker](?)
(In reply to comment #11)
> (I'm assuming this is using a <deck>.)

You assume correctly.
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.
Attached file 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).
Attached patch part 1 (obsolete) — Splinter Review
This fixes the assertion, but not the bug.
(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 on attachment 505346 [details] [diff] [review]
part 1

Unsolicited feedback+ :).
Attachment #505346 - Flags: feedback+
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.
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
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.
Attachment #505640 - Flags: review?(tnikkel)
Attachment #505346 - Flags: review?(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.
Whiteboard: [hardblocker](?) → [hardblocker]
(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)
I believe bug 612232 may also be an instance of this.
(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...
Attachment #505346 - Attachment is obsolete: true
Attachment #506163 - Flags: review?(tnikkel)
Attachment #505346 - Flags: review?(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?
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 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?
(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 ?
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?
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)
Attached patch part 2 (alternative) (obsolete) — Splinter Review
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.
Attachment #507300 - Flags: review?(tnikkel)
Comment on attachment 505640 [details] [diff] [review]
part 2, fix + testcase

Ok. After thinking about it more I'm fine with this.
Attachment #505640 - Flags: review?(tnikkel) → review+
Attachment #507300 - Flags: review?(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?
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.
Attachment #507300 - Attachment is obsolete: true
Yes, that's perfectly fine.
Thanks, I filed bug 629823 to follow-up on part 1.

http://hg.mozilla.org/mozilla-central/rev/bfd144a54f0a
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b11
Attachment #506163 - Flags: review?(tnikkel)
Attachment #506163 - Attachment is obsolete: true
Verified fixed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre
Status: RESOLVED → VERIFIED
Whiteboard: [hardblocker] → [hardblocker][fx4-fixed-bugday]
You need to log in before you can comment on or make changes to this bug.