Last Comment Bug 658005 - need an API for real visibility of frames
: need an API for real visibility of frames
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Timothy Nikkel (:tnikkel)
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: 655263
  Show dependency treegraph
 
Reported: 2011-05-18 11:51 PDT by Timothy Nikkel (:tnikkel)
Modified: 2011-10-27 01:50 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (13.10 KB, patch)
2011-05-18 11:52 PDT, Timothy Nikkel (:tnikkel)
no flags Details | Diff | Splinter Review
Part 1. Allowing query frame to work on deck frames. (1.59 KB, patch)
2011-10-17 22:08 PDT, Timothy Nikkel (:tnikkel)
roc: review+
Details | Diff | Splinter Review
Part 2. Add the new API and use it in most places. (21.05 KB, patch)
2011-10-17 22:09 PDT, Timothy Nikkel (:tnikkel)
roc: review+
Details | Diff | Splinter Review
Part 3. Make accessibility use the new API and remove code it obsoletes. (4.16 KB, patch)
2011-10-17 22:10 PDT, Timothy Nikkel (:tnikkel)
surkov.alexander: review+
roc: review+
Details | Diff | Splinter Review
Part 4. Remove AreAncestorViewsVisible. (2.33 KB, patch)
2011-10-17 22:12 PDT, Timothy Nikkel (:tnikkel)
roc: review+
Details | Diff | Splinter Review

Description Timothy Nikkel (:tnikkel) 2011-05-18 11:51:25 PDT
We use the hidden-ness of views that are imparted upon them be being a hidden panel of a deck in different places. I think we will need a property to replace this on frames, at least for decks, maybe for other views when they are removed.
Comment 1 Timothy Nikkel (:tnikkel) 2011-05-18 11:52:51 PDT
Created attachment 533353 [details] [diff] [review]
patch

We don't actually use the new property anywhere in this patch, that'll come later.
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-18 15:06:01 PDT
Hmm. nsDocShell::GetVisibility should also be changed I think; currently it checks the hidden-ness of views.

I would really like to avoid introducing new visibility state. Things are complicated enough already.

Could we avoid adding this extra visibility state by making visibility:hidden docshells that are content docshells with chrome parents automatically "hidden tabs"? And then just have decks use visibility:hidden?
Comment 3 Timothy Nikkel (:tnikkel) 2011-05-18 16:13:54 PDT
Another thing to think about is that there are more views to remove (subdocuments, plugins, popups, and the root view) and we may want to use this visibility state when removing those views too.
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-18 16:23:52 PDT
Most visibility issues we can deal with during display list construction, simply by not building display items for children we want to hide.

I don't think subdocuments need special visibility handling except for the "hidden tab" situation.

For popups, we need to hide and show the popup widget but that's all I think.

Plugins already manage visibility correctly because we either control the plugin painting (windowless) or we hide the widget if it's not supposed to be visible (windowed).

Not sure what you meant about the root view.
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2011-05-18 18:26:05 PDT
So is the question in comment 2 whether we can automatically mark visibility:hidden subdocument frames as inactive docshells?
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-18 18:39:24 PDT
That depends on what you mean by "inactive".

I'm suggesting we treat them the way we treat docshells in hidden view subtrees.
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2011-05-18 20:36:03 PDT
> That depends on what you mean by "inactive".

The sense we use it in for background tabs (throttle refresh driver, etc, etc).

> I'm suggesting we treat them the way we treat docshells in hidden view subtrees.

That seems fine to me....
Comment 8 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-18 20:42:16 PDT
(In reply to comment #7)
> > That depends on what you mean by "inactive".
> 
> The sense we use it in for background tabs (throttle refresh driver, etc,
> etc).

Right. Yes, I'm saying make content docshells with visibility:hidden chrome <iframe>s behave like that.
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2011-05-18 20:49:57 PDT
Should be doable, yeah.  The issue is getting the interaction with explicit "is active" state changes from script right.
Comment 10 Timothy Nikkel (:tnikkel) 2011-05-18 22:11:42 PDT
The iframe's aren't directly under the deck, there are two element in between. We'd have to do some sort of frame tree search, but how do we limit it? The deck for Panorama is even further separated from the iframes.
Comment 11 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-18 22:21:06 PDT
We wouldn't have to do a frame tree search, because 'visibility' inherits.
Comment 12 Timothy Nikkel (:tnikkel) 2011-10-15 17:39:34 PDT
So I've got all this working but I've so far failed to come up with a good name for the new function. Here is what I have for the API:

  // CSS visibility just doesn't cut it because it doesn't inherit through
  // documents. Also if this frame is in a hidden card of a deck then it isn't
  // visible either and that isn't expressed using CSS visibility. Also if it
  // is in a hidden view (there are a few cases left and they are hopefully
  // going away soon).
  // If the VISIBILITY_CROSS_CHROME_CONTENT_BOUNDARY flag is passed then we
  // ignore the chrome/content boundary, otherwise we stop looking when we
  // reach it.
  enum {
    VISIBILITY_CROSS_CHROME_CONTENT_BOUNDARY = 0x01
  };
  bool GetForRealsVisibilityYo(PRUint32 aFlags = 0) const;

We need to both ignore and cross the chrome/content boundary in different places. I'm open to a better way to deal with that too.
Comment 13 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-17 05:34:50 PDT
GetVisibilityConsideringAncestors? (i.e. considering deck and tab ancestors)
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2011-10-17 10:02:38 PDT
Is there a reason to not just call this IsVisible()?  Or would that lead to confusion with things that are merely occluded by other content?
Comment 15 Timothy Nikkel (:tnikkel) 2011-10-17 10:47:27 PDT
IsVisible is used on nsStyleVisibility to indicate 'visibility: visible', I wanted to make it clear that this is based on more than just straight CSS visibility.
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2011-10-17 10:51:07 PDT
But you're adding this on nsIFrame, not on nsStyleVisibility, right?

Maybe GetFrameVisibility() then?  I guess comment 13 would work too....
Comment 17 Timothy Nikkel (:tnikkel) 2011-10-17 22:08:27 PDT
Created attachment 567678 [details] [diff] [review]
Part 1. Allowing query frame to work on deck frames.
Comment 18 Timothy Nikkel (:tnikkel) 2011-10-17 22:09:08 PDT
Created attachment 567679 [details] [diff] [review]
Part 2. Add the new API and use it in most places.
Comment 19 Timothy Nikkel (:tnikkel) 2011-10-17 22:10:47 PDT
Created attachment 567681 [details] [diff] [review]
Part 3. Make accessibility use the new API and remove code it obsoletes.

Note that this is a behaviour change, but I think it is more correct.
Comment 20 Timothy Nikkel (:tnikkel) 2011-10-17 22:12:11 PDT
Created attachment 567682 [details] [diff] [review]
Part 4. Remove AreAncestorViewsVisible.
Comment 21 alexander :surkov 2011-10-18 00:49:55 PDT
Comment on attachment 567681 [details] [diff] [review]
Part 3. Make accessibility use the new API and remove code it obsoletes.

r=me since it seems equivalent to what we have assuming that similar layout changes are equivalent to old code
Comment 22 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-18 02:50:11 PDT
Comment on attachment 567679 [details] [diff] [review]
Part 2. Add the new API and use it in most places.

Review of attachment 567679 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/base/nsDocShell.cpp
@@ +4826,5 @@
>          bool isDocShellOffScreen = false;
>          docShell->GetIsOffScreenBrowser(&isDocShellOffScreen);
> +        if (frame &&
> +            !frame->GetVisibilityConsideringAncestors(nsIFrame::VISIBILITY_CROSS_CHROME_CONTENT_BOUNDARY) &&
> +            !isDocShellOffScreen) {

Why are you changing the behavior here by crossing the content/chrome boundary?

::: layout/base/nsPresShell.cpp
@@ +5234,5 @@
>      return nsnull;
>  
>    nsIFrame* frame = static_cast<nsIFrame*>(aView->GetClientData());
> +  if (frame) {
> +    if (!frame->GetVisibilityConsideringAncestors(nsIFrame::VISIBILITY_CROSS_CHROME_CONTENT_BOUNDARY) ||

And here?

@@ +5272,5 @@
>  
>    nsIFrame* frame = static_cast<nsIFrame*>(aView->GetClientData());
> +  if (frame) {
> +    if (!frame->GetVisibilityConsideringAncestors(nsIFrame::VISIBILITY_CROSS_CHROME_CONTENT_BOUNDARY) ||
> +        !frame->PresContext()->PresShell()->IsActive()) {

and here?

::: layout/generic/nsIFrame.h
@@ +2763,5 @@
> +  // reach it.
> +  enum {
> +    VISIBILITY_CROSS_CHROME_CONTENT_BOUNDARY = 0x01
> +  };
> +  bool GetVisibilityConsideringAncestors(PRUint32 aFlags = 0) const;

Actually, I think IsVisibleConsideringAncestors().
Comment 23 Timothy Nikkel (:tnikkel) 2011-10-18 12:44:53 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> ::: docshell/base/nsDocShell.cpp
> @@ +4826,5 @@
> >          bool isDocShellOffScreen = false;
> >          docShell->GetIsOffScreenBrowser(&isDocShellOffScreen);
> > +        if (frame &&
> > +            !frame->GetVisibilityConsideringAncestors(nsIFrame::VISIBILITY_CROSS_CHROME_CONTENT_BOUNDARY) &&
> > +            !isDocShellOffScreen) {
> 
> Why are you changing the behavior here by crossing the content/chrome
> boundary?

It's actually not really a behaviour change because nsDocShell::GetVisibility ends up jumping the chrome content boundary itself no matter what. AreAncestorViewsVisible stopped at the chrome content boundary, but this function ignored that and jumped the chrome content boundary and then called AreAncestorViewsVisible on the chrome parent document.

> ::: layout/base/nsPresShell.cpp
> @@ +5234,5 @@
> >      return nsnull;
> >  
> >    nsIFrame* frame = static_cast<nsIFrame*>(aView->GetClientData());
> > +  if (frame) {
> > +    if (!frame->GetVisibilityConsideringAncestors(nsIFrame::VISIBILITY_CROSS_CHROME_CONTENT_BOUNDARY) ||
> 
> And here?
> 
> @@ +5272,5 @@
> >  
> >    nsIFrame* frame = static_cast<nsIFrame*>(aView->GetClientData());
> > +  if (frame) {
> > +    if (!frame->GetVisibilityConsideringAncestors(nsIFrame::VISIBILITY_CROSS_CHROME_CONTENT_BOUNDARY) ||
> > +        !frame->PresContext()->PresShell()->IsActive()) {
> 
> and here?

I don't think these are behaviour changes: if GetVisibilityConsideringAncestors(VISIBILITY_CROSS_CHROME_CONTENT_BOUNDARY) is false then that view or one of its ancestor views would be hidden. Am I missing something?

> ::: layout/generic/nsIFrame.h
> @@ +2763,5 @@
> > +  // reach it.
> > +  enum {
> > +    VISIBILITY_CROSS_CHROME_CONTENT_BOUNDARY = 0x01
> > +  };
> > +  bool GetVisibilityConsideringAncestors(PRUint32 aFlags = 0) const;
> 
> Actually, I think IsVisibleConsideringAncestors().

Can do.

Note You need to log in before you can comment on or make changes to this bug.