Closed Bug 1518919 Opened 10 months ago Closed 6 months ago

Communicate current CSS 'visibility' value to oop-frames

Categories

(Core :: Layout, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla68
Fission Milestone M3
Tracking Status
firefox68 --- fixed

People

(Reporter: jwatt, Assigned: hiro)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Accessible::VisibilityState needs to know whether a given node is visible. It currently does that by looking up the frame tree to the root of the top-level document. In a Fission world this will miss visibility:hidden on an embedding element.

Blocks: rendering-fission
No longer blocks: 1518917
Attached file Testcase

This testcase seems to work fine for me (with browser.fission.oopif.attribute set to true). I'm not sure if it's actually using a separate process for the iframe though, is there a way to tell?

(In reply to Mats Palmgren (:mats) from comment #1)

I'm not sure if it's actually using a separate process for the iframe though, is there a way to tell?

Currently you can tell by things like:

  • the cursor doesn't change to the text selection cursor
    when hovering over the text in the iframe

  • you can't select the text in the iframe

  • the context menu that opens for the iframe seems unaware
    that there in an iframe there

Note that this bug is not about making sure that the iframe doesn't paint when it's 'visibility: hidden' (that works, as you note).

This bug is about notifying the oop-iframe's embedded document that its embedding element has 'visibility: hidden', and then using that as appropriate. Specifically, the Accessible::VisibilityState code checks whether the embedding frame is hidden here:

https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/accessible/generic/Accessible.cpp#369

That code will need to check whether it is running for an oop-iframe and, if so, get the visibility that has been passed to the process.

There are probably other places that will use this passed visibility state.

The code in Accessible::VisibilityState looks odd to me. The function returns INVISIBLE when the target frame's visibility style value is not visible or if it's visible then we walk up the tree and see if there is an ancestor whose style is not visiblity:visible. Given that a visibility:visible element in a visibility:hidden element should be visible..

I think what we need to do is even if the target style is visibility:hidden we have to descend down the tree and see if there is NO visibility:visible descendants (I don't recommend this way though). Or if we don't care about the descendant's visibility, the function can just check the style data for the style frame, we don't need to walk up its ancestors.

Eitan, does this sound correct? I honestly don't quite understand how Accessible::VisibilityState is used.

If I am right, there is no need to do for fission, I think.

That's being said, though I didn't know that, visibility:visible element in an iframe in a visibility:hidden element is not visible at all. I confirmed that Chrome does the same behavior. The DOM tree I am testing is;

<div style="visibility: hidden">                                                
  <span style="visibility: visible">hello</span>                                
  <iframe id="iframe" srcdoc='<span style="visibility:visible">can you see me?</span>' width="100" height="100"></iframe>
</div> 

Even if we need to care about this case, as far as I can tell nsIPresShell::IsVisible() correctly reflects the invisible state, so we can just check it there.

Flags: needinfo?(eitan)

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

The code in Accessible::VisibilityState looks odd to me. The function returns INVISIBLE when the target frame's visibility style value is not visible or if it's visible then we walk up the tree and see if there is an ancestor whose style is not visiblity:visible. Given that a visibility:visible element in a visibility:hidden element should be visible..

Yes, that's clearly wrong. I thought I'd filed a bug on that, but apparently not...

I think what we need to do is even if the target style is visibility:hidden we have to descend down the tree and see if there is NO visibility:visible descendants (I don't recommend this way though). Or if we don't care about the descendant's visibility, the function can just check the style data for the style frame, we don't need to walk up its ancestors.

The accessibility code checks whether a specific element is "visible", and I don't think it intends to check "is any sub-part (element) visible", so I don't think we need to walk down.

That's being said, though I didn't know that, visibility:visible element in an iframe in a visibility:hidden element is not visible at all. I confirmed that Chrome does the same behavior. The DOM tree I am testing is;

Yes, an iframe should be treated atomically with regards to visibility as I recall.

Even if we need to care about this case, as far as I can tell nsIPresShell::IsVisible() correctly reflects the invisible state, so we can just check it there.

I don't think so. There is no code that communicates this information to the presShell in the OOP-iframe. That's what this bug is about.

FWIW Accessible::VisibilityState is called by Accessible::NativeState which is called by a bunch of other functions but I think the only one we need to care about is DocAccessibleChild::RecvNativeState (possibly also Accessible::State, but I'm not sure what that's for).

https://searchfox.org/mozilla-central/rev/00f3836a87b844b5e4bc82f698c559b9966e4be2/accessible/ipc/other/PDocAccessible.ipdl#125

Thanks jwatt for the clarification. I did miss that nsIPresShel::IsVisible ends up calling nsIFrame::IsVisibleConsideringAncestors with VISIBILITY_CROSS_CHROME_CONTENT_BOUNDARY. That's indeed a problem. A way what I can think of when style visibility change happens on iframe we tell it to the child pres shell and use it in IsVisible() instead of calling IsVisibleConsideringAncestors.

I think I can try it. Taking.

Assignee: nobody → hikezoe
Status: NEW → ASSIGNED

Now I noticed that the fission meeting log says mats is going to take this. :)

Assignee: hikezoe → nobody
Status: ASSIGNED → NEW

That was tentative. Mats is still busy with grid I believe and hadn't actually assigned this to himself so presumably he hadn't done anything more than taken a cursory look. Please go ahead and work on this, hiro. We should fix this as one of the bugs for Fission M2 (May 6).

Assignee: nobody → hikezoe
Fission Milestone: --- → ?
Status: NEW → ASSIGNED
Fission Milestone: ? → M2
Depends on: 1541251
See Also: → 1541705

Neha, the patches for this bug have ended up over in bug 1541253. Can you move the M2 flag over there?

Flags: needinfo?(nkochar)
Fission Milestone: M2 → ---
Flags: needinfo?(nkochar)
No longer depends on: 1541251
Depends on: 1541253

Hiro, should this be closed as a dup of bug 1541253? Or is there more work expected on this?

Flags: needinfo?(hikezoe)

No, bug 1541253 aims to introduce a machinery to tell whether a given document is hidden by an element whose visibility style is 'hidden' in an ancestor document without walking up the document tree. It works regardless of fission. In this bug, I am going to make the machinery work on fission too.

Flags: needinfo?(hikezoe)
Fission Milestone: --- → M3

Clearing NI to eeejay since the code in question has been fixed in bug 1541253, I think.

Flags: needinfo?(eitan)
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d77d76d37d4d
Propagate CSS visibility information to descendant documents via IPC call for fission. r=jwatt
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Regressions: 1551241
You need to log in before you can comment on or make changes to this bug.