If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

doc shell visibility check should check if the doc shell is active

RESOLVED INVALID

Status

()

Core
Layout
RESOLVED INVALID
6 years ago
6 years ago

People

(Reporter: tnikkel, Assigned: tnikkel)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Comment 1

6 years ago
Created attachment 533365 [details] [diff] [review]
patch
Assignee: nobody → tnikkel
Attachment #533365 - Flags: review?(bzbarsky)
What situations is this function actually called in?  That is, why are we changing it to look at active at all?

And if it _should_ look at active, why does it care about being an offscreen browser if !active?
(Assignee)

Comment 3

6 years ago
We check the visibility of doc shells for event and focus stuff (http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsEventStateManager.cpp#4541 http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#6691 http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsXULPopupManager.cpp#1446 http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp#1370 http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#4837) and at http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#3017 which only seems to be concerned with hidden tabs.

I'm not sure we _need_ to check IsActive in GetVisibility (after my other changes for removing views from decks, specifically the changes to nsIFrame::AreAncestorViewsVisible in bug 658005) but I think we should and I don't think it will hurt.

The offscreen browser stuff was added for the mobile browser back when it used the tile browser. My understanding is that the tile browser is basically dead code, but it is still in our tree. The tile browser painted web content to the screen using a canvas via drawWindow, so the docshell wasn't actually on screen, so they needed something to say that the browser was actually being drawn to the screen. See bug 454235. I'm basically just propagating the same check for consistency but ideally getting rid of the offscreen browser stuff and the unused tile browser is what should happen.
> I'm basically just propagating the same check for consistency

I think my point is that an inactive offscreen browser should still be considered invisible... that's what inactive means.
(Assignee)

Comment 5

6 years ago
My understanding of IsOffScreenBrowser is that it is an override of the normal ways of determining if a document is visible. So that if it is true you consider the docshell visible and ignore the normal methods of determining visibility. Is that not the case?
Hmm.  I don't know...

I can live with assuming it means that, I guess.
Comment on attachment 533365 [details] [diff] [review]
patch

r=me
Attachment #533365 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 8

6 years ago
Due to other changes in how I'm doing this we don't need this so I'd rather not add the additional complexity. (It was always more of insurance, but I don't need the insurance now.)
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.