Note: There are a few cases of duplicates in user autocompletion which are being worked on.

need an API for real visibility of frames

RESOLVED FIXED in mozilla10

Status

()

Core
Layout
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: tnikkel, Assigned: tnikkel)

Tracking

Trunk
mozilla10
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

6 years ago
Created attachment 533353 [details] [diff] [review]
patch

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

Comment 3

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

Comment 5

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

Comment 7

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

Comment 9

6 years ago
Should be doable, yeah.  The issue is getting the interaction with explicit "is active" state changes from script right.
(Assignee)

Comment 10

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

Comment 12

6 years ago
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)

Comment 14

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

Comment 15

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

6 years ago
But you're adding this on nsIFrame, not on nsStyleVisibility, right?

Maybe GetFrameVisibility() then?  I guess comment 13 would work too....
(Assignee)

Updated

6 years ago
Attachment #533353 - Attachment is obsolete: true
Attachment #533353 - Flags: review?(roc)
(Assignee)

Comment 17

6 years ago
Created attachment 567678 [details] [diff] [review]
Part 1. Allowing query frame to work on deck frames.
Attachment #567678 - Flags: review?(roc)
(Assignee)

Comment 18

6 years ago
Created attachment 567679 [details] [diff] [review]
Part 2. Add the new API and use it in most places.
Attachment #567679 - Flags: review?(roc)
(Assignee)

Comment 19

6 years ago
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.
Attachment #567681 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #567681 - Attachment is patch: true
Attachment #567681 - Flags: review?(surkov.alexander)
Attachment #567681 - Flags: review?(roc)
Attachment #567681 - Flags: review?
(Assignee)

Comment 20

6 years ago
Created attachment 567682 [details] [diff] [review]
Part 4. Remove AreAncestorViewsVisible.
Attachment #567682 - Flags: review?(roc)

Comment 21

6 years ago
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+
Attachment #567678 - Flags: review?(roc) → review+
Attachment #567682 - Flags: review?(roc) → 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().
Attachment #567681 - Flags: review?(roc) → review+
(Assignee)

Updated

6 years ago
Summary: need a hidden-ness property on frames to replace hidden views → need an API for real visibility of frames
(Assignee)

Comment 23

6 years ago
(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.
Attachment #567679 - Flags: review?(roc) → review+
(Assignee)

Comment 24

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/25e0254f4bda
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd1754632d8c
https://hg.mozilla.org/integration/mozilla-inbound/rev/6eda7e1b7225
https://hg.mozilla.org/integration/mozilla-inbound/rev/36c457f93e0f
https://hg.mozilla.org/mozilla-central/rev/25e0254f4bda
https://hg.mozilla.org/mozilla-central/rev/bd1754632d8c
https://hg.mozilla.org/mozilla-central/rev/6eda7e1b7225
https://hg.mozilla.org/mozilla-central/rev/36c457f93e0f
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.