Closed Bug 506615 Opened 10 years ago Closed 10 years ago

Remove native widgets from decks

Categories

(Core :: XUL, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: roc, Assigned: roc)

References

Details

(Keywords: verified1.9.2)

Attachments

(1 file, 2 obsolete files)

Mobile wants native widgets removed from decks and currently uses a #define to do that. Pre-compositor this used to work, but I'm not sure how. Now it doesn't. The problem is that the native widgets for the <browser>s in a deck aren't hidden when they're not in the deck's selected child. Rather than go back and try to figure out what changed and hack something in to fix the regression for mobile, I'd rather move forward and just remove widgets from deck children for everyone.
Flags: blocking1.9.2?
Attached patch fix (obsolete) — Splinter Review
View visibility is a mess unfortunately, but at least here I'm trying to tighten it up a bit --- see the comment in nsIView.h. Basically we assume that everything under a hidden view is going to be hidden, so we go ahead and hide every widget under a hidden view, and maintain that during dynamic changes.
Attachment #390786 - Flags: review?(dbaron)
Comment on attachment 390786 [details] [diff] [review]
fix

>+NS_IMETHODIMP nsView::SetVisibility(nsViewVisibility aVisibility)
>+{
>+  mVis = aVisibility;
>+  NotifyEffectiveVisibilityChanged(IsEffectivelyVisible());
>   return NS_OK;
> }

Probably you should only call NotifyEffectiveVisibilityChanged if the result of IsEffectivelyVisible changes across the setting of mVis?

Other than that, this looks good, although it's made me realize that bug 465216 blocks one of the later phases of compositor (removing widgets at chrome/content boundaries).

r=dbaron
Attachment #390786 - Flags: review?(dbaron) → review+
(In reply to comment #2)
> (From update of attachment 390786 [details] [diff] [review])
> >+NS_IMETHODIMP nsView::SetVisibility(nsViewVisibility aVisibility)
> >+{
> >+  mVis = aVisibility;
> >+  NotifyEffectiveVisibilityChanged(IsEffectivelyVisible());
> >   return NS_OK;
> > }
> 
> Probably you should only call NotifyEffectiveVisibilityChanged if the result of
> IsEffectivelyVisible changes across the setting of mVis?

I could, but nsViewManager::SetViewVisibility already bails early if mVis == aVisibility, and catching the extra case (where an ancestor is hidden, so this view remains effectively hidden) doesn't seem worth it.

Thanks.
This seems to have caused https://bugzilla.mozilla.org/show_bug.cgi?id=507380
as the backout of this bug fixed the scrollbars.
Depends on: 507380
But 507380 and a lot (if not all) of those test failures was due to a mistake in nsViewManager::GrabMouseEvents:

-  if (aView && static_cast<nsView*>(aView)->GetVisibility()
-               == nsViewVisibility_kHide) {
+  if (aView && static_cast<nsView*>(aView)->IsEffectivelyVisible()) {

That should of course be !IsEffectivelyVisible... Running try-server tests now.
Attached patch fix v2 (obsolete) — Splinter Review
There was another problem where test_focus.xul was failing because we'd treat frames in hidden deck children as focusable. This turned out to be because with the previous version of the patch there is nothing to force deck children to have views, so there's no hidden view associated with hidden deck children, which is what nsFrame::IsFocusable uses to detect deck-hidden frames.

So in this version of the patch as well as the one-line fix in GrabMouseEvents, I'm overriding nsDeckFrame's SetInitialChildList/AppendFrames/InsertFrames to create views for the children. This seems cleaner than the trunk approach of calling a special method on the parent to detect when we need to create a view and widget.
Attachment #390786 - Attachment is obsolete: true
Attachment #391856 - Flags: review?(dbaron)
Whiteboard: [needs review]
Comment on attachment 391856 [details] [diff] [review]
fix v2

>+CreateViewsForFrames(const nsFrameList& aFrames)

Maybe call this ForceCreateViewsForFrames or ForceViewsForFrames since it passes PR_TRUE to the aForce parameter of CreateViewForFrame?

>+  NS_IMETHOD  SetInitialChildList(nsIAtom*        aListName,
>+                                  nsFrameList&    aChildList);

Fix that weird double-space after NS_IMETHOD.

r=dbaron
Attachment #391856 - Flags: review?(dbaron) → review+
Keywords: checkin-needed
Whiteboard: [needs review] → [needs landing]
http://hg.mozilla.org/mozilla-central/rev/79db77b024f7
Status: NEW → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla1.9.2a1
Oops, somehow I attached the wrong patch. Oh well.
Sorry for tagging a 'fixed' bug, but don't have time to file a new one.
See discussion in this thread: http://forums.mozillazine.org/viewtopic.php?f=23&t=1397405

Seems this bug has broken the Options Panel...details in the thread.
Depends on: 508174
Depends on: 509478
This bug has also broken Firebug (editing in HTML and DOM editors) as well as Twitterfox.

bug 509478
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
Blocks: 506444
Depends on: 508134
Some thing is messed up here in bugzilla.

Bug 507380 claims that the fix here has been backed out. It looks like it was re-committed, but now comment 14, bug 509478, and bug 507380 seem to suggest that this fix is still causing problems.
	
Also the Depends-on list seems upside down. Several bugs on that list are really Blocks bugs: if this bug was correctly fixed they would be fixed.

I'm going to set this to reopened just to highlight it as needing attention; I guess we will need to wait for roc to return to sort it out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [firebug-p1]
(In reply to comment #16)
> Bug 507380 claims that the fix here has been backed out.

That's correct, see comment 4 and comment 5.

> It looks like it was re-committed,

Indeed, see comment 10.

> but now comment 14, bug 509478, and bug 507380 seem to suggest
> that this fix is still causing problems.

It is!

> Also the Depends-on list seems upside down.

No, it's not. Regressions caused by this bug block it.
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
(In reply to comment #17)
...
> > Also the Depends-on list seems upside down.
> 
> No, it's not. Regressions caused by this bug block it.

Can you help me understand this? The bug is fixed, so blocking means what? It cannot make any more progress. By their definition the regressions say "that fix, it was not a good one, it should be redone" (as in "it was not really a fix after all).

If independent work is needed to solve the problem on the regression bugs, then they still don't block progress on this one, they are on their own. On the other hand if more work is needed on this patch, then it's not fixed and progress on it prevents progress on the regression reporting bug. 

(sorry for the meta comment).
Do not use this bug as a forum for complaining about how we use dependencies.
Whiteboard: [firebug-p1]
Depends on: 511951
Mass change: adding fixed1.9.2 keyword

(This bug was identified as a mozilla1.9.2 blocker which was fixed before the mozilla-1.9.2 repository was branched (August 13th, 2009) as per this query: http://is.gd/2ydcb - if this bug is not actually fixed on mozilla1.9.2, please remove the keyword. Apologies for the bugspam)
Keywords: fixed1.9.2
verified per re-checkin and assumed pass of try server testing.
Keywords: verified1.9.2
There is code in nsFrameFrame.cpp that seems like it creates widgets for decks here http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrameFrame.cpp#299

Is it still needed?
No, that should be removed.
Depends on: 589634
You need to log in before you can comment on or make changes to this bug.