465216 - nsFrame::IsVisibleConsideringAncestors should move to docshell and be based on explicit API

Status: REOPENED

(Core :: Layout, defect)

Description

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

As I described in bug 455891 comment 13, nsIFrame::AreAncestorViewsVisible should:
 (1) move to docshell, and
 (2) be based on an explicit API that tabbrowser and any other callers (we'll need to document this for embedders!) on any docshell that is hidden by virtue of being in an inactive tab or similar UI feature

When we do this, we should revert the fix in bug 455891.

We really need to fix this before the code in question gets any messier, but it seems we only ever end up touching it right before a release when it's a bit close to make API changes.  Therefore, nominating for blocking1.9.2 so that we actually get to this earlier.  It's not hard, it just needs to be done early in the cycle and documented.

Comment 1

[Neil Deakin]

> Boy, AreAncestorViewsVisible sure wasn't meant to be used for things like
> focus; it was only intended to be good enough to say that things can be
> optimized away, not reliable enough that it guarantees something is shown.

Which is why focus doesn't rely on it alone and checks style visibility and other things as well.

So what do you propose for focus?

Focus shouldn't apply to any element in a hidden view (such as a hidden tab). 
Whether there is a docshell inside it is actually irrelevant. So I'm not clear how an api on the docshell can be used for this.

Comment 2

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

The point here is that layout shouldn't be poking through the widget tree to try to guess whether something is in a hidden tab.  The app should be telling the docshell, "you're a hidden tab".

And in the case where you've added the magic attribute, the correct fix there would be that the app in question (Fennec) would *not* tell the docshell "you're a hidden tab".

Comment 3

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

(And the comment quoted in comment 1 was a reaction to "this function was written to make a conservative guess at what's a hidden tab in order to optimize things, and now we're using it for important stuff".)

Comment 4

[Neil Deakin]

(In reply to comment #2)
> The app should be telling the docshell, "you're a hidden tab".

I'm referring to the case where is no docshell in the tab. Tabs or decks, for
instance in the preference pane or page info dialog. Controls in the hidden
pages should not be focusable. I'm asking what you are proposing for this.

Comment 5

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

Then we might need some other mechanism for an element to suppress focusability for its descendants, but it shouldn't have anything to do with views or widgets, which should be going away, and whose presence or absence should not be causing side-effects.  I think it should probably live in content rather than layout.

Comment 6

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

Then again, if you're already checking style visibility, living in layout is probably ok.  (But please note that view visibility has nothing to do with the 'visibility' property.)

Comment 7

[Neil Deakin]

Well, at the moment the best way to express what focus needs is 'is this element (or isn't) inside a hidden nsIView' as decks only express visibility in this manner. There may also be other elements/frame types that I'm not familiar with as well.

Perhaps a different method on nsIFrame that nsDeckFrame (and others) can override.

Comment 8

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

This probably blocks the phase of compositor that involves removing widgets at the chrome/content boundary.

Comment 9

[Bobby Holley (:bholley)]

bug 343515 is fixed now, fwiw.

Comment 10

[Timothy Nikkel (:tnikkel)]

In bug 658005 I removed this function in favour of a different function with similar functionality. Not sure how that affects the goals of this bug.

Comment 11

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

I still want the thing that goes from nsViewManager::SetWindowDimensions uses (per bug 227361) (currently PresShell::IsVisible) to determine whether to do the resizing optimization to be based on an explicit API from embedders.  (It's currently using nsFrame::IsVisibleConsideringAncestors, which looks at view visibility.)

Comment 12

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

And to be clear:  I think poking into things and making assumptions that deck frames are important is the wrong approach -- that's essentially layout poking into the internals of tabbrowser so that this works in Firefox desktop.    Tabbrowser should instead be using an API to say that some tabs are things that aren't currently being displayed, and then Firefox OS could use that same API to get the same benefits.

(The API should probably be on docshell; that doesn't mean much code needs to move there, though.)

Comment 13

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

Oh, and the existing SetIsActive etc. API on the doc shell might be what we want; it reaches the pres shell at PresShell::SetIsActive.  So it might just be a matter of hooking this resizing optimization up to that and replacing IsVisibleConsideringAncestors.  (But it's worth checking that the tabbrowser is already using that API.)

Comment 14

[Boris Zbarsky [:bzbarsky]]

Tabbrowser definitely uses that API.  That's what we use to throttle refresh drivers in background tabs.