1732179 - [CTW] Bounds are incorrectly calculated for transformed documents

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Morgan Reschenberg [:morgan]]

STR:

1. Open data:text/html,<iframe style='transform: translate(120px, 50%); height:100px; width:100px;' src='data:text/html,hello world'></iframe>
2. With VoiceOver, focus 'hello world'

Expected:
The VO cursor wraps the text

Actual:
The VO cursor focuses the location the text would be if it didn't have a transform applied.

I tested this with fission off.

This isn't an issue for something like data:text/html,<div style='transform: translate(120px, 50%); height:100px; width:100px;'>hello world</div>

Comment 1

[Morgan Reschenberg [:morgan]]

Attachment: WIP: Bug 1732179: Cache the transform matrix r?Jamie,eeejay

Comment 2

[Morgan Reschenberg [:morgan]]

Attachment: WIP: Bug 1732179: Adjust DocAccessible::RelativeBounds to work with transforms r?Jamie

Comment 3

[Morgan Reschenberg [:morgan]]

Attachment: WIP: Bug 1732179: self-relative transform attemp

Depends on D128039

Comment 4

[Morgan Reschenberg [:morgan]]

Attachment: WIP: Bug 1732179: Cache the transform matrix r?Jamie,eeejay

Comment 5

[Morgan Reschenberg [:morgan]]

Attachment: WIP: Bug 1732179: Cache transform matrix, apply non-doc transforms in RemoteAccessibleBase::Bounds r?Jamie,eeejay

Comment 6

[Phabricator Automation]

Comment on attachment 9251463 [details]
WIP: Bug 1732179: Cache transform matrix, apply non-doc transforms in RemoteAccessibleBase::Bounds r?Jamie,eeejay

Revision D131554 was moved to bug 1744573. Setting attachment 9251463 [details] to obsolete.

Comment 7

[Morgan Reschenberg [:morgan]]

ni?me to check if this is still an issue with CtW and my patches on bug 1772861, given recent bounds changes

Comment 8

[Morgan Reschenberg [:morgan]]

Update:

So this kinda works! When you navigate this example from c0 data:text/html,<iframe style='transform: translate(120px, 50%); height:100px; width:100px;' src='data:text/html,hello world'></iframe> VO now correctly wraps both the iframe and the inner text, with the exception of what looks like a 4-5px translation. The VO visual cursor is slightly closer to the page origin (upper left) compared to where it's supposed to be (tightly wrapping the content). This is true for the VO cursor on both the iframe and the 'hello world' text.

I think this has to do with where we account for the cross-process iframe offset when transforming.
We don't know what that offset is (or really, that we have an offset at all) until we process the outer doc acc. By that time, we've already applied any transforms set on the inner doc. In theory, those inner doc transforms should happen after we've added in the offset.

We can probably fix this by checking "up" the tree instead of "down" the tree like we do now (so instead of keeping track of recentAcc here, we'd check the parent on the current acc)

Comment 9

[Morgan Reschenberg [:morgan]]

In order to test out-of-process iframes, we should probably use a separate origin in our example -- here's a better test case:

data:text/html,<iframe style='transform: translate(50px, 50px); height:100px; width:100px;' src='https://example.com'></iframe>

Comment 10

[Nathan LaPré]

Yup, looks good, thanks! I do think we have a challenge with in-process iframes with respect to the IsTopLevelInContentProcess check. In the original example (using an iframe containing a data:text/html source), the iframe document isn't "top level in content process," though there is a cross-process offset to apply. It seems to me we might still want to apply that offset even though it's in the same process. I have a first stab at this to spark discussion that I'll push to phab in a bit.

Comment 11

[Nathan LaPré]

Attachment: Bug 1732179: Account for cross-process offset properly in bounds calculation, r?morgan

This revision slightly reworks the way that we calculate accessible bounds in
RemoteAccessibleBase::BoundsWithOffset. Rather than looking "below" the current
accessible for a doc accessible to determine whether we should apply a cached
cross-process offset, we instead look "above" the current accessible, to its
parent, to check for a cross-process offset to apply.

Comment 12

[Morgan Reschenberg [:morgan]]

Nathan and I discussed this yesterday -- the main issue here lives in RemoteAccessibleBase::ApplyTransform.

When applying transforms, we first make the so-far-computed-bounds self-relative instead of parent relative by offsetting them to 0,0. We do this because the transformation matrix as we've cached it from layout includes the x/y translation, and we don't want to account for this twice.

Because layout has no cross-process visibility, it cannot include the parent-relative translation between cross-process iframe documents and their outer doc (what we call the "cross process offset") in the transformation matrix for internal frames.
When processing a transform on an <iframe> element, we effectively delete the cross-process offset by attempting to make the bounds-so-far rect self-relative.

We could fix this with a check in ApplyTransform, something like:

aBounds.MoveTo(0,0)
if (IsOuterDoc()) {
  RemoteAccessible* firstChild = FirstChild();
  if (firstChild->IsDoc() && !firstChild->AsDoc()->IsTopLevel && firstChild->AsDoc()->IsTopLevelInContentProcess()) {
      aBounds.MoveTo(GetCrossProcessOffset());
    }
}

We need to do this for the transform case. I'm interested to know if the order-of-operations holds up ok here. I imagine it will, because we're correcting what layout thinks is "self-relative", but we'll just have to see.

We also should account for the non-transform case, which is where the "looking up" adjustment comes in. This isn't strictly necessary to fix the issue described here, but for overall correctness I'd like to make this change too.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/407bcfc6b8e5
Account for cross-process offset properly in bounds calculation, r=morgan

Comment 14

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/407bcfc6b8e5

Comment 15

[Rares Doghi, Desktop QA]

Hi Morgan, We tried to verify this fix but this issue still occurs in our latest builds on both Beta as well as our latest Nightly build, only when we flip the pref accessibility.cache.enabled = true it will correctly wrap around the text for this test case :

data:text/html,<iframe style='transform: translate(120px, 50%); height:100px; width:100px;' src='data:text/html,hello world'></iframe>

Should this fix work with that pref set to false as well ?

Comment 16

[Morgan Reschenberg [:morgan]]

Hey Rares :) The patch attached to this bug is expected to fix the problem for our caching infrastructure only -- we don't consider this to be a priority for our non-caching infrastructure (even though, as you've noted, it is broken there). I'll adjust the bug title to reflect that this is caching-specific.

Thanks for asking!

Comment 17

[Rares Doghi, Desktop QA]

This issue is Verified as fixed with accessibility.cache.enabled - true, Updating the Flags, Thank you @Morgan