1541705 - Need a way to tell whether the given nsIFrame is scrolled out without walking up frame tree across document boundaries

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P2)

Description

[Hiroyuki Ikezoe (:hiro)]

In Accessible::VisibilityState() we walk up frame tree to see whether a given nsIFrame is scrolled out or not, also, in nsIFrame::UpdateVisibilitySynchronously we do similar thing. There might be other places though.

We need a way to tell it without crossing document boundaries for fission.

Comment 1

[Jonathan Watt [:jwatt]]

ni so you're aware of this bug.

Comment 2

[Jonathan Watt [:jwatt]]

Ryan, did you work on passing affects down to oop-iframes fix this, or clear the way for fixing this? Or is this a dupe of another bug?

Comment 3

[Ryan Hunt [:rhunt] (on leave until early May)]

I don't think that work solves this perfectly. We transmit whether an OOP-iframe was painted and if so what the visible rect was, down to the iframe. This may not mean the OOP-iframe is actually visible due to a larger than the screen APZ displayport.

This bug looks like it needs more precise layout information about what's actually visible or scrolled. We could attach that to EffectsInfo, but it's a design-call whether we only want painting information or should expand it to include layout information.

Comment 4

[Neha Kochar [:neha]]

Sean, could you find an assignee for this fission M4 bug?

Comment 5

[Sean Voisen (:svoisen)]

Hiro, is this something you think you can look at for M4?

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Sure, I will figure out the way.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Botond, I have a couple of questions about HitTestingTreeNode and APZ.

In this bug, for fission world we need to provide a way to tell whether a given element in an out-of-process iframe is scrolled out by a scrollable element in an ancestor document. As of now what I've done for it

1. Walking up HitTestingTreeNode from the corresponding node for out-of-process iframe to the root node and the calculate size and position of the iframe on the screen coordinate
2. Notify them to corresponding processes for the out-of-process iframes along with the transform matrix of the iframe
3. Calculate the given element rectangle with the transform matrix on the screen coordinate and check the intersection of the given element rectangle and the iframe rectangle given by 1), and if there is no intersection we consider the given element is scrolled out

My questions are;

1. Does this sound a reasonable way?
2. Do all scrollable nsIFrames have an individual HitTestingTreeNode (and HitTestingTreeNode::mApzc) even if 'overflow: hidden' is specified? (I've confirmed it's true in a simple test case)
3. Does HitTestingTreeNode::mVisibleRegion represent iframe size? (I am using it for the calculation of the iframe size)

FWIW, here is a try; The most important part is the function to calculate iframe rectangle by walking up HitTestingTreeNode

Thank you!

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: animated-gif-test

Timothy, I am going to mend nsIFrame::UpdateVisibilitySynchronously() to make it work in fission world, but I am confused about it.

Initially I was thinking that animated GIFs are skipped to be painted if it's scrolled out, but it seems not? Attaching file is a mochitest that I used for checking it. (I used ImageDecoderObserverStub). Do we have other ways to check it? Or we don't skip painting such images in the first place?

After I wrote the mochitest, I noticed a comment in a call sites of UpdateVisibilitySynchronously that is saying it's not necessary any more. (The other call site has the same comment). So, are we OK with mending the function without any test cases? I don't want to drop the calls since I am totally unfamiliar with those code.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Timothy ^ ?

Comment 10

[Timothy Nikkel (:tnikkel)]

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

Initially I was thinking that animated GIFs are skipped to be painted if it's scrolled out, but it seems not? Attaching file is a mochitest that I used for checking it. (I used ImageDecoderObserverStub). Do we have other ways to check it? Or we don't skip painting such images in the first place?

The reason that image is decoded is because the displayport is big enough to include it, so we paint it. The displayport is two screen heights it seems, which seems overkill for a 300px high div, but would be reasonable for the root scroll frame.

After I wrote the mochitest, I noticed a comment in a call sites of UpdateVisibilitySynchronously that is saying it's not necessary any more. (The other call site has the same comment). So, are we OK with mending the function without any test cases? I don't want to drop the calls since I am totally unfamiliar with those code.

I'm dubious of those comments, I don't remember the details right now but I think it was part of a refactor that never got finished where the second part of the refactor was "and now we do X (but we have no idea how to actually do X)" and so the second part of the refactor never got done.

Comment 11

[Botond Ballo [:botond]]

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

1. Does this sound a reasonable way?

I haven't looked through the patch in detail, but based on your description in comment 7, yes, that sounds reasonable.

2. Do all scrollable nsIFrames have an individual HitTestingTreeNode (and HitTestingTreeNode::mApzc) even if 'overflow: hidden' is specified? (I've confirmed it's true in a simple test case)

No, generally only active scroll frames get a hit testing tree node, and overflow: hidden scroll frames are usually not active (except for the root).

However:

• The root scroll frame of an OOP iframe should always be active.
• The ancestors of an active scroll frame should also be active.

Therefore, a scroll frame that can scroll an OOP iframe should always be active, and I believe those are the ones we care about here.

3. Does HitTestingTreeNode::mVisibleRegion represent iframe size? (I am using it for the calculation of the iframe size)

It represents the area that is painted. It can be smaller than the entire iframe in cases where e.g. the iframe scroll port is very tall (taller than the screen + displayport margins), or part of the iframe scroll port is obscured by an element that scrolls together with the iframe.

I think a better place to get the iframe size is from the composition bounds of the layer representing the iframe's root scroll frame.

Comment 12

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #11)

• The ancestors of an active scroll frame should also be active.

Though, it's possible that the code that ensures this needs to be adapted for Fission.

Comment 13

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #11)

2. Do all scrollable nsIFrames have an individual HitTestingTreeNode (and HitTestingTreeNode::mApzc) even if 'overflow: hidden' is specified? (I've confirmed it's true in a simple test case)

No, generally only active scroll frames get a hit testing tree node, and overflow: hidden scroll frames are usually not active (except for the root).

However:

• The root scroll frame of an OOP iframe should always be active.
• The ancestors of an active scroll frame should also be active.

Actually, this may not apply to overflow:hidden ancestors.

However, I'm not sure why we would care about those. A overflow:hidden scroll frame can only be scrolled by the main thread (e.g. scrollTo()), and if some of its contents are layerized, the main-thread scroll offset would be reflected in the transforms of those layers. Activating the scroll frame is only required for async scrolling.

Comment 14

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #13)

A overflow:hidden scroll frame can only be scrolled by the main thread (e.g. scrollTo()), [...]

(That could change in bug 1519339, but that's an orthogonal issue.)

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Thank you Timothy and Botond!

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

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

Initially I was thinking that animated GIFs are skipped to be painted if it's scrolled out, but it seems not? Attaching file is a mochitest that I used for checking it. (I used ImageDecoderObserverStub). Do we have other ways to check it? Or we don't skip painting such images in the first place?

The reason that image is decoded is because the displayport is big enough to include it, so we paint it. The displayport is two screen heights it seems, which seems overkill for a 300px high div, but would be reasonable for the root scroll frame.

OK, I am going to write mochitest with a bigger spacer there.

(In reply to Botond Ballo [:botond] from comment #13)

(In reply to Botond Ballo [:botond] from comment #11)

2. Do all scrollable nsIFrames have an individual HitTestingTreeNode (and HitTestingTreeNode::mApzc) even if 'overflow: hidden' is specified? (I've confirmed it's true in a simple test case)

No, generally only active scroll frames get a hit testing tree node, and overflow: hidden scroll frames are usually not active (except for the root).

However:

• The root scroll frame of an OOP iframe should always be active.
• The ancestors of an active scroll frame should also be active.

Actually, this may not apply to overflow:hidden ancestors.

However, I'm not sure why we would care about those. A overflow:hidden scroll frame can only be scrolled by the main thread (e.g. scrollTo()), and if some of its contents are layerized, the main-thread scroll offset would be reflected in the transforms of those layers. Activating the scroll frame is only required for async scrolling.

What I expected is that during walking up HitTestingTreeNode we can cull out areas that the iframe is in overflowed area. It seems to be done by using mVisibleRegion? Or the overflowed area might have been already culled out by nsLayoutUtils::TransformFrameRectToAncestor on the main-thread side. Anyways I will check the case.

Thank you both!

Comment 16

[Hiroyuki Ikezoe (:hiro)]

:surkov, I am trying to understand two checks in Accessible::VisibilityState. There are two checks, one is whether the frame rectangle is contained by scrollport, the other is whether deflated the scrollport by 12px has non-empty intersection with the frame rectangle. So to me, it looks it just ensures that at least 12px width (or height) region of the target frame is visible. Is this right? For fission world (for out-of-process iframes) we will introduce a bit different machinery to check whether the given nsIFrame is scrolled out or not for elements in out-of-process iframes, and with the machinery the region corresponding to the scrollport has been already clipped by ancestors scrollable frames. It would be hard to do the same thing there. So for example:

<div style="height: 300px; overflow-y: scroll;">
<div style="height: 280px;"></div>
<iframe fission=true src="child.html" width="300" height="300" frameborder="0"></iframe> <!-- this is an out-of-process iframe -->
</div>

In this example, we will have 20px height region as the scrollport-ish information, so if we deflate the region, we will have an empty region so that we will never handle elements inside the iframe properly.

I can see similar issues regardless of fission. Here is an example to see the issue in a mochitest.

diff --git a/accessible/tests/mochitest/states/test_visibility.html b/accessible/tests/mochitest/states/test_vis
--- a/accessible/tests/mochitest/states/test_visibility.html
+++ b/accessible/tests/mochitest/states/test_visibility.html
@@ -23,6 +23,7 @@
testStates("div_transformed", STATE_OFFSCREEN, 0, STATE_INVISIBLE);
testStates("div_abschild", 0, 0, STATE_INVISIBLE | STATE_OFFSCREEN);
testStates("ul", STATE_OFFSCREEN, 0, STATE_INVISIBLE);

•  testStates("target", 0, 0, STATE_OFFSCREEN);
  
   SimpleTest.finish();
  

  }
  @@ -70,6 +71,9 @@
  <li>Supermarket 1</li>
  <li>Supermarket 2</li>
  </ul>
• <div style="width: 100%; height:24px; overflow-x: scroll">

•  <div id="target" style="font-size: 10px;">hello world</span>
  

• </div>
  </div>
  </body>
  </html>

So, I am wondering whether we really need the second check or not. If we really need it, we need a different way, what I can think of is to check whether the intersection area's width (or height) is greater than 12px.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

gosh! the diff in above comment was totally broken. :/

Comment 18

[alexander :surkov (:asurkov)]

12 pixels check were introduced in bug 120176. The original propose of the bug was to report partially visible elements as visible elements to a11y clients. Those 12 pixels was an empirical rule, and I'd say it makes practical sense. Say, we have a scrolled off element with a couple of pixels visible, then, if we had no such rule, the element would be reported to AT users as visible, however sighted users could hardly say anything about this element, since practically it is out of screen.

Having said that, we could try to easy out this condition if it's next to impossible to implement it in fission project , and see how it goes. Looping in Jamie.

Btw, I didn't catch about problem with a test:
<div style="width: 100%; height:24px; overflow-x: scroll">
<div id="target" style="font-size: 10px;">hello world</div>
</div>

It appears it's rendered in whole, i.e. nothing to scroll. It has to not expose STATE_INVISIBLE | STATE_OFFSCREEN, which seems match your observations testStates("target", 0, 0, STATE_OFFSCREEN). Could you elaborate it please?

Comment 19

[Hiroyuki Ikezoe (:hiro)]

(In reply to alexander :surkov (:asurkov) from comment #18)

Btw, I didn't catch about problem with a test:
<div style="width: 100%; height:24px; overflow-x: scroll">
<div id="target" style="font-size: 10px;">hello world</div>
</div>

It appears it's rendered in whole, i.e. nothing to scroll. It has to not expose STATE_INVISIBLE | STATE_OFFSCREEN, which seems match your observations testStates("target", 0, 0, STATE_OFFSCREEN). Could you elaborate it please?

On my Linux machine, the target state is STATE_OFFSCREEN, that's the problem I can see.

Comment 20

[alexander :surkov (:asurkov)]

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

(In reply to alexander :surkov (:asurkov) from comment #18)

Btw, I didn't catch about problem with a test:
<div style="width: 100%; height:24px; overflow-x: scroll">
<div id="target" style="font-size: 10px;">hello world</div>
</div>

It appears it's rendered in whole, i.e. nothing to scroll. It has to not expose STATE_INVISIBLE | STATE_OFFSCREEN, which seems match your observations testStates("target", 0, 0, STATE_OFFSCREEN). Could you elaborate it please?

On my Linux machine, the target state is STATE_OFFSCREEN, that's the problem I can see.

that's weird, btw I don't see offscreen style applied in Inspector on OSX, I doubt it is platform specific though. I do test plain data:text,<your_markup>

Comment 21

[Hiroyuki Ikezoe (:hiro)]

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

what I can think of is to check whether the intersection area's width (or height) is greater than 12px.

For the accessibility stuff, I will go with this way, and I am going to add an edge case (around 12px) to make sure it works.

Here is a try;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=850e7b8a461acdb903803713ad178dff6785156b

A TODO for bug 1577084 in a mochitest fails (unexpectedly passes) in TV, so I might need to disable the TODO.

Also note that an accessibility test in the try fails intermittently due to bug 1559841, I hope the patch for bug 1559841 will re-land soon, but if it's not I will have to disable the test too.

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1541705 - Factor out functionalities for obsversing animation style counts into testcommon.js. r=boris

We are going to use these functions in gfx/layers/apz/tests/mochitest/ for
fission.

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1541705 - Factor out functionalities for obsversing animation style counts into testcommon.js. r?boris

We are going to use these functions in gfx/layers/apz/tests/mochitest/ for
fission.

Comment 24

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1541705 - Introduce remote document rect. r?botond

The visible region will be used for calculating the result of the composition
of the remote display item on the compositor.

Depends on D44418

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1541705 - Notify areas of out-of-process subframes from the compositor to the corresponding process. r?botond

Note that the areas are clipped out by all ancestor scroll ports and
their coordinate systems are the screen coordinate. So that we can tell
arbitrary elements in out-of-process iframes are scrolled out or not with
this area and the transform matrix of the iframe on screen coodinate.

Depends on D44419

Comment 26

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1541705 - Introduce nsLayoutUtils::FrameIsScrolledOutOfViewInCrossProcess and use it for the check whether animating element is scrolled out of view or not. r?botond!,boris!

Depends on D44420

Comment 27

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1541705 - Introduce nsLayoutUtils::FrameIsMostlyScrolledOutOfViewInCrossProcess. r?botond

This will be used for accessibility stuff.

Depends on D44421

Comment 28

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1541705 - Try to see whether the target frame is scrolled out of view in out-of-process iframe if we couldn't walk up the frame tree. r?surkov

Depends on D44422

Comment 29

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1541705 - Test for offscreen image in out-of-process iframe. r?tnikkel

Depends on D44423

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/80a073621c53
Factor out functionalities for obsversing animation style counts into testcommon.js. r=boris
https://hg.mozilla.org/integration/autoland/rev/689df124ac2c
Introduce remote document rect. r=botond
https://hg.mozilla.org/integration/autoland/rev/fb5bcfad4822
Notify areas of out-of-process subframes from the compositor to the corresponding process. r=botond
https://hg.mozilla.org/integration/autoland/rev/dd526a39c352
Introduce nsLayoutUtils::FrameIsScrolledOutOfViewInCrossProcess and use it for the check whether animating element is scrolled out of view or not. r=botond,boris
https://hg.mozilla.org/integration/autoland/rev/481dbebe905c
Introduce nsLayoutUtils::FrameIsMostlyScrolledOutOfViewInCrossProcess. r=botond
https://hg.mozilla.org/integration/autoland/rev/10d08f48e097
Try to see whether the target frame is scrolled out of view in out-of-process iframe if we couldn't walk up the frame tree. r=surkov
https://hg.mozilla.org/integration/autoland/rev/4880c3a309c9
Test for offscreen image in out-of-process iframe. r=tnikkel

Comment 31

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/80a073621c53
https://hg.mozilla.org/mozilla-central/rev/689df124ac2c
https://hg.mozilla.org/mozilla-central/rev/fb5bcfad4822
https://hg.mozilla.org/mozilla-central/rev/dd526a39c352
https://hg.mozilla.org/mozilla-central/rev/481dbebe905c
https://hg.mozilla.org/mozilla-central/rev/10d08f48e097
https://hg.mozilla.org/mozilla-central/rev/4880c3a309c9

Comment 32

[Hiroyuki Ikezoe (:hiro)]

Thank you Botond and Timothy. I am pretty sure I couldn't finish this bug without your helps. When I assigned this bug to me, I had totally no idea how we can fix the issue.