Closed
Bug 658005
Opened 14 years ago
Closed 13 years ago
need an API for real visibility of frames
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla10
People
(Reporter: tnikkel, Assigned: tnikkel)
References
Details
Attachments
(4 files, 1 obsolete file)
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
surkov
:
review+
roc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
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•14 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•14 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•14 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•14 years ago
|
||
Should be doable, yeah. The issue is getting the interaction with explicit "is active" state changes from script right.
Assignee | ||
Comment 10•14 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•13 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•13 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•13 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•13 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•13 years ago
|
Attachment #533353 -
Attachment is obsolete: true
Attachment #533353 -
Flags: review?(roc)
Assignee | ||
Comment 17•13 years ago
|
||
Attachment #567678 -
Flags: review?(roc)
Assignee | ||
Comment 18•13 years ago
|
||
Attachment #567679 -
Flags: review?(roc)
Assignee | ||
Comment 19•13 years ago
|
||
Note that this is a behaviour change, but I think it is more correct.
Attachment #567681 -
Flags: review?
Assignee | ||
Updated•13 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•13 years ago
|
||
Attachment #567682 -
Flags: review?(roc)
Comment 21•13 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•13 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•13 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•13 years ago
|
||
Comment 25•13 years ago
|
||
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
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in
before you can comment on or make changes to this bug.
Description
•