Closed Bug 1541251 Opened 5 years ago Closed 5 years ago

Avoid walking up frame tree across document boundaries in nsIFrame::IsVisibleConsideringAncestors and Accessible::VisibilityState

Categories

(Core :: Layout, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
Fission Milestone M4

People

(Reporter: hiro, Assigned: hiro)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

I've checked all call sites of StyleVisibility(), as of now, there are two call sites that the style is used to tell whether the frame is invisible or not across documents boundaries. They are nsIFrame::IsVisibleConsideringAncestors and Accessible::VisibilityState.

Instead of walking up the tree there, we can notify CSS visibility state changes to descendant documents and also we can notify the info when we switch frame in nsDeckFrame. I am going to split these part into other bugs for the safety, I mean, we can easily track down if it caused regressions.

As for nsView::GetVisibility(), the call sites of nsViewManager::SetViewVisibility (i.e. the place where we change the visibility of the given nsView) are in nsComboboxControlFrame.cpp, nsPluginFrame.cpp, nsMenuPopupFrame.cpp and nsFrame.cpp. I think the call sites other than the nsFrame.cpp don't need to be cared the across document boundary cases since each the nsView is for popup or dropdown or plugin which means it's in the same document or inside a plugin.

Depends on: 1541253
Type: defect → enhancement
No longer depends on: 1541253
Depends on: 1541253
Depends on: 1541256

I did split the part that is to drop the nsLayoutUtils::GetCrossDocParentFrame call in Accessible::VisibilityState() as bug 1541705. The function also checks the given nsIFrame is scrolled out or not.

hiro, should this bug block bug 1518919? It seems like it should actually depend on it, right?

Flags: needinfo?(hikezoe)

Oh right. As for CSS visibility stuff (bug 1518919), we just need bug 1541253 instead. I am fixing the dependency.

Blocks: rendering-fission
No longer blocks: 1518919
No longer depends on: 1541253
Flags: needinfo?(hikezoe)
Depends on: 1541705, 1541253
Fission Milestone: --- → M4
Depends on: 1579247
Blocks: a11y-fission

The last bit in this bug is nsView::GetVisiblity check in nsIFrame::IsVisibleConsideringAncestors and in Accessible::VisibilityState. (There is also nsLayoutUtils::IsPopup() check in Accessibile::VisibilityState, but I don't think we have cross-origin iframe as menu item in popup menu.)

Timothy, do we also need to do something for fission in terms of the GetVisibility check? I have totally no idea about nsView things.

Flags: needinfo?(tnikkel)

(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)

The last bit in this bug is nsView::GetVisiblity check in nsIFrame::IsVisibleConsideringAncestors and in Accessible::VisibilityState. (There is also nsLayoutUtils::IsPopup() check in Accessibile::VisibilityState, but I don't think we have cross-origin iframe as menu item in popup menu.)

Timothy, do we also need to do something for fission in terms of the GetVisibility check? I have totally no idea about nsView things.

I'm kinda hoping we don't have to do anything here.

Views might get hidden via nsViewManager::SetViewVisibility, looking at the callers of that function we have:

  1. nsComboboxControlFrame
  2. nsPluginFrame
  3. nsMenuPopupFrame
  4. nsSubDocumentFrame

For 1,2, and 3 I haven't thought through it fully but I hope we don't need to do anything for them.
For 4 it is exactly in sync with the CSS property visibility. So we just need to check if a visibility hidden on an iframe "works" with these two functions.

Flags: needinfo?(tnikkel)

(In reply to Timothy Nikkel (:tnikkel) from comment #5)

(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)

The last bit in this bug is nsView::GetVisiblity check in nsIFrame::IsVisibleConsideringAncestors and in Accessible::VisibilityState. (There is also nsLayoutUtils::IsPopup() check in Accessibile::VisibilityState, but I don't think we have cross-origin iframe as menu item in popup menu.)

Timothy, do we also need to do something for fission in terms of the GetVisibility check? I have totally no idea about nsView things.

I'm kinda hoping we don't have to do anything here.

Views might get hidden via nsViewManager::SetViewVisibility, looking at the callers of that function we have:

  1. nsComboboxControlFrame
  2. nsPluginFrame
  3. nsMenuPopupFrame
  4. nsSubDocumentFrame

For 1,2, and 3 I haven't thought through it fully but I hope we don't need to do anything for them.
For 4 it is exactly in sync with the CSS property visibility. So we just need to check if a visibility hidden on an iframe "works" with these two functions.

Great to know! Thanks!

I will close this bug once bug 1541256 and bug 1541705 are fixed.

Now bug 1541256 and bug 1541705 landed on m-c. Closing.

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.