Closed Bug 1541705 Opened 6 months ago Closed Last month

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

Categories

(Core :: Layout, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla71
Fission Milestone M4
Tracking Status
firefox71 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

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

Details

Attachments

(8 files, 1 obsolete file)

1.85 KB, patch
Details | Diff | Splinter Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

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.

Type: defect → enhancement

ni so you're aware of this bug.

Flags: needinfo?(rhunt)
Blocks: 1541251
Flags: needinfo?(rhunt)
Fission Milestone: ? → M4

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?

Flags: needinfo?(rhunt)
Priority: P3 → P2

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.

Flags: needinfo?(rhunt)

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

Flags: needinfo?(svoisen)

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

Flags: needinfo?(svoisen) → needinfo?(hikezoe.birchill)

Sure, I will figure out the way.

Assignee: nobody → hikezoe.birchill
Status: NEW → ASSIGNED
Flags: needinfo?(hikezoe.birchill)

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!

Flags: needinfo?(botond)

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.

Timothy ^ ?

Flags: needinfo?(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.

Flags: needinfo?(tnikkel)

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

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

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

Flags: needinfo?(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.

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

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

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

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)

  1. 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!

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

Flags: needinfo?(surkov.alexander)

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

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?

Flags: needinfo?(surkov.alexander)

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

Blocks: a11y-fission

(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>

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

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

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

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

Depends on D44418

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

This will be used for accessibility stuff.

Depends on D44421

Attachment #9089948 - Attachment is obsolete: true
Attachment #9089956 - Attachment is obsolete: true
Attachment #9089956 - Attachment description: Bug 1541705 - Make nsIFrame::UpdateVisibilitySynchronously work in fission world. r?tnikkel → Bug 1541705 - Test for offscreen image in out-of-process iframe. r?tnikkel
Attachment #9089956 - Attachment is obsolete: false
Blocks: 1541256
Attachment #9089951 - Attachment description: Bug 1541705 - Set the visible region to WebRenderLayerScrollData for remote display items. r?botond → Bug 1541705 - Introduce remote document rect. r?botond
Attachment #9089953 - Attachment description: Bug 1541705 - Introduce nsLayoutUtils::FrameIsScrolledOutInCrossProcess and use it for the check whether animating element is scrolled out or not. r?botond!,boris! → Bug 1541705 - Introduce nsLayoutUtils::FrameIsScrolledOutOfViewInCrossProcess and use it for the check whether animating element is scrolled out of view or not. r?botond!,boris!
Attachment #9089954 - Attachment description: Bug 1541705 - Introduce nsLayoutUtils::FrameIsMostlyScrolledOutInCrossProcess. r?botond → Bug 1541705 - Introduce nsLayoutUtils::FrameIsMostlyScrolledOutOfViewInCrossProcess. r?botond
Attachment #9089955 - Attachment description: Bug 1541705 - Try to see whether the target frame is scrolled out in out-of-process iframe if we couldn't walk up the frame tree. r?surkov → 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: 1580703
Depends on: 1580706
Pushed by hikezoe.birchill@mozilla.com:
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
Regressions: 1581113
Regressions: 1581137

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.

You need to log in before you can comment on or make changes to this bug.