1518919 - Communicate current CSS 'visibility' value to oop-frames

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P3)

Description

[Jonathan Watt [:jwatt]]

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.

Comment 1

[Mats Palmgren (inactive)]

Attachment: 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?

Comment 2

[Jonathan Watt [:jwatt]]

(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

Comment 3

[Jonathan Watt [:jwatt]]

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.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 5

[Jonathan Watt [:jwatt]]

(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

Comment 6

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

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

Comment 8

[Jonathan Watt [:jwatt]]

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).

Comment 9

[Jonathan Watt [:jwatt]]

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

Comment 10

[Neha Kochar [:neha]]

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

Comment 11

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1518919 - Propagate CSS visibility information to descendant documents via IPC call for fission. r=jwatt

Comment 13

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=840fae6464eb0b85dd5b62881f7d81b3cf5e7bcc

Comment 14

[Hiroyuki Ikezoe (:hiro)]

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d77d76d37d4d
Propagate CSS visibility information to descendant documents via IPC call for fission. r=jwatt

Comment 16

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/d77d76d37d4d