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

VERIFIED FIXED in mozilla2.0b11

Status

()

Core
Layout
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: Natch, Assigned: mats)

Tracking

Trunk
mozilla2.0b11
x86
All
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

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

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

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

Comment 2

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

Comment 3

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

Comment 5

8 years ago
I can confirm the STR in comment 3.

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

Updated

8 years ago
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?

Comment 7

8 years ago
Created attachment 498492 [details]
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

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

Updated

8 years ago
OS: Windows Vista → All
Assignee: nobody → matspal
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.
(Assignee)

Comment 14

7 years ago
Created attachment 505345 [details]
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).
(Assignee)

Comment 15

7 years ago
Created attachment 505346 [details] [diff] [review]
part 1

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

Comment 19

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

Comment 20

7 years ago
Created attachment 505640 [details] [diff] [review]
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.
Attachment #505640 - Flags: review?(tnikkel)
(Assignee)

Updated

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

Comment 25

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

Comment 26

7 years ago
Created attachment 506163 [details] [diff] [review]
part 1 v2, don't ResizeView root views + assert parent's VM is 'this' (paranoid)
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?
(Assignee)

Comment 31

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

Comment 33

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

Comment 34

7 years ago
Created attachment 507300 [details] [diff] [review]
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.
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?
(Assignee)

Comment 37

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

Updated

7 years ago
Attachment #507300 - Attachment is obsolete: true
Yes, that's perfectly fine.
(Assignee)

Comment 39

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

http://hg.mozilla.org/mozilla-central/rev/bfd144a54f0a
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b11
(Assignee)

Updated

7 years ago
Attachment #506163 - Flags: review?(tnikkel)
(Assignee)

Updated

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