Closed Bug 658005 Opened 13 years ago Closed 13 years ago

need an API for real visibility of frames

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: tnikkel, Assigned: tnikkel)

References

Details

Attachments

(4 files, 1 obsolete file)

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.
Attached patch patch (obsolete) — Splinter Review
We don't actually use the new property anywhere in this patch, that'll come later.
Attachment #533353 - Flags: review?(roc)
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?
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.
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.
So is the question in comment 2 whether we can automatically mark visibility:hidden subdocument frames as inactive docshells?
That depends on what you mean by "inactive".

I'm suggesting we treat them the way we treat docshells in hidden view subtrees.
> 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....
(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.
Should be doable, yeah.  The issue is getting the interaction with explicit "is active" state changes from script right.
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.
We wouldn't have to do a frame tree search, because 'visibility' inherits.
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.
GetVisibilityConsideringAncestors? (i.e. considering deck and tab ancestors)
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?
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.
But you're adding this on nsIFrame, not on nsStyleVisibility, right?

Maybe GetFrameVisibility() then?  I guess comment 13 would work too....
Attachment #533353 - Attachment is obsolete: true
Attachment #533353 - Flags: review?(roc)
Note that this is a behaviour change, but I think it is more correct.
Attachment #567681 - Flags: review?
Attachment #567681 - Attachment is patch: true
Attachment #567681 - Flags: review?(surkov.alexander)
Attachment #567681 - Flags: review?(roc)
Attachment #567681 - Flags: review?
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
Attachment #567681 - Flags: review?(surkov.alexander) → review+
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().
Summary: need a hidden-ness property on frames to replace hidden views → need an API for real visibility of frames
(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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: