Keep track of fixed positioning in accessibility tree.

Keep track of fixed positioning in accessibility tree.

For efficiency, the accessibility tree keeps track of the relative
coordinates of nodes. That makes things like scrolling and other
layer transformations more efficient; only the container needs to
be updated, and all of the coordinates of objects within that
container will be updated on-the-fly.

We weren't handling fixed-positioned elements correctly, though.
Fixed-positioned elements don't move when the root element scrolls,
so there's no way they could be properly encoded.

Fix this by explicitly identifying fixed-positioned elements. The
algorithm change is really simple - fixed-positioned elements ignore
scroll offsets of their containers. That's it. Everything else just
works.

This bug wasn't caught for a while because it only manifests when
layout doesn't change. Any time there's a layout, existing code
detects the new location of the fixed-positioned element and updates
it. So the only case that was broken was when you scroll a page and
then navigate assistive technology to a fixed-position element without
triggering layout first.

Note that it's not really appropriate to unit-test within Blink because
the Blink functions were essentially returning correct coordinates; the
bug was due to the fact that when the page scrolled, the previously
computed relative coordinates of the fixed-positioned element are now
wrong. So this change is covered by a browser test only, plus by
dozens of other existing tests that this patch doesn't break.

BUG=732346

[dmazzoni, 2017-06-28 19:10:01]

The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-28 19:10:27]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dmazzoni, 2017-06-28 19:12:05]

dmazzoni@chromium.org changed reviewers:
+ aboxhall@chromium.org

[commit-bot: I haz the power, 2017-06-28 20:25:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-28 20:25:11]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[aboxhall, 2017-06-30 06:07:41]

https://codereview.chromium.org/2956053005/diff/1/content/browser/accessibili...
File content/browser/accessibility/accessibility_tree_formatter_blink.cc
(right):

https://codereview.chromium.org/2956053005/diff/1/content/browser/accessibili...
content/browser/accessibility/accessibility_tree_formatter_blink.cc:63:
LOG(ERROR) << page_bounds.y() << " page bounds for "
Stray log? Looks like there's a few.

https://codereview.chromium.org/2956053005/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/accessibility/AXSpinButton.cpp (right):

https://codereview.chromium.org/2956053005/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXSpinButton.cpp:111:
out_is_fixed_positioned = false;
Is there some reason we couldn't make is_fixed_positioned part of the
WebAXObject API instead of attaching it to this method?

[aleventhal, 2017-07-05 13:09:58]

> Fixed-positioned elements don't move when the root element scrolls

Unfortunately this is only true if there no transforms are applied to any
ancestor.

According to https://www.w3.org/TR/css-transforms-1/#transform-rendering
"Specifying a value other than ‘none’ for the ‘transform’ property establishes a
new local coordinate system at the element that it is applied to."

Side note: this can be a very frustrating rule, because if you are in a
transformed container you cannot present as fixed. IE is the only browser that
does not follow it.

[dmazzoni, 2017-07-06 07:20:22]

https://codereview.chromium.org/2956053005/diff/1/content/browser/accessibili...
File content/browser/accessibility/accessibility_tree_formatter_blink.cc
(right):

https://codereview.chromium.org/2956053005/diff/1/content/browser/accessibili...
content/browser/accessibility/accessibility_tree_formatter_blink.cc:63:
LOG(ERROR) << page_bounds.y() << " page bounds for "
On 2017/06/30 06:07:40, aboxhall wrote:
> Stray log? Looks like there's a few.

Fixed all, sorry about that

https://codereview.chromium.org/2956053005/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/accessibility/AXSpinButton.cpp (right):

https://codereview.chromium.org/2956053005/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXSpinButton.cpp:111:
out_is_fixed_positioned = false;
On 2017/06/30 06:07:41, aboxhall wrote:
> Is there some reason we couldn't make is_fixed_positioned part of the
> WebAXObject API instead of attaching it to this method?

I don't want to do that in the WebAXObject API because is_fixed_positioned
is a critical piece of information that's intertwined with the rest of the
bounds info, so I want to force callers to fetch all of them together.

How about this, though - I added a new protected method
GetSimpleRelativeBounds that can be overridden by this
class and two others that have to return their own bounds but
have no use for transforms or fixed positioning. Does that
help readability?

[dmazzoni, 2017-07-06 07:20:40]

On 2017/07/05 13:09:58, aleventhal wrote:
> > Fixed-positioned elements don't move when the root element scrolls
> 
> Unfortunately this is only true if there no transforms are applied to any
> ancestor.

Wow, good catch.

Ia added a failing test for this case but I'm not sure the correct fix. I
assumed
that when we called LayoutObject::IsFixedPositioned(), it would return false
since the positioning isn't really fixed.

Adding ikilpatrick for advice here - Ian, is there a function we can call
to determine if a LayoutObject is truly fixed-positioned, i.e. not only is
its style position:fixed but none of its ancestors have a transform? Is
there a high-level API for that so we don't have to reinvent that logic?

[dmazzoni, 2017-07-06 07:20:41]

The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-06 07:20:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dmazzoni, 2017-07-06 07:21:06]

dmazzoni@chromium.org changed reviewers:
+ ikilpatrick@chromium.org

[dmazzoni, 2017-07-06 07:21:06]

Actually adding ikilpatrick - see above comment, thanks!

[commit-bot: I haz the power, 2017-07-06 08:01:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-06 08:01:57]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[ikilpatrick, 2017-07-06 22:15:56]

Description was changed from

==========
Keep track of fixed positioning in accessibility tree.

For efficiency, the accessibility tree keeps track of the relative
coordinates of nodes. That makes things like scrolling and other
layer transformations more efficient; only the container needs to
be updated, and all of the coordinates of objects within that
container will be updated on-the-fly.

We weren't handling fixed-positioned elements correctly, though.
Fixed-positioned elements don't move when the root element scrolls,
so there's no way they could be properly encoded.

Fix this by explicitly identifying fixed-positioned elements. The
algorithm change is really simple - fixed-positioned elements ignore
scroll offsets of their containers. That's it. Everything else just
works.

This bug wasn't caught for a while because it only manifests when
layout doesn't change. Any time there's a layout, existing code
detects the new location of the fixed-positioned element and updates
it. So the only case that was broken was when you scroll a page and
then navigate assistive technology to a fixed-position element without
triggering layout first.

Note that it's not really appropriate to unit-test within Blink because
the Blink functions were essentially returning correct coordinates; the
bug was due to the fact that when the page scrolled, the previously
computed relative coordinates of the fixed-positioned element are now
wrong. So this change is covered by a browser test only, plus by
dozens of other existing tests that this patch doesn't break.

BUG=732346
==========

to

==========
Keep track of fixed positioning in accessibility tree.

For efficiency, the accessibility tree keeps track of the relative
coordinates of nodes. That makes things like scrolling and other
layer transformations more efficient; only the container needs to
be updated, and all of the coordinates of objects within that
container will be updated on-the-fly.

We weren't handling fixed-positioned elements correctly, though.
Fixed-positioned elements don't move when the root element scrolls,
so there's no way they could be properly encoded.

Fix this by explicitly identifying fixed-positioned elements. The
algorithm change is really simple - fixed-positioned elements ignore
scroll offsets of their containers. That's it. Everything else just
works.

This bug wasn't caught for a while because it only manifests when
layout doesn't change. Any time there's a layout, existing code
detects the new location of the fixed-positioned element and updates
it. So the only case that was broken was when you scroll a page and
then navigate assistive technology to a fixed-position element without
triggering layout first.

Note that it's not really appropriate to unit-test within Blink because
the Blink functions were essentially returning correct coordinates; the
bug was due to the fact that when the page scrolled, the previously
computed relative coordinates of the fixed-positioned element are now
wrong. So this change is covered by a browser test only, plus by
dozens of other existing tests that this patch doesn't break.

BUG=732346
==========

[ikilpatrick, 2017-07-06 22:15:56]

ikilpatrick@chromium.org changed reviewers:
+ flackr@chromium.org

[ikilpatrick, 2017-07-06 22:16:32]

+flackr who knows a bunch more than me around fixed/sticky.

[flackr, 2017-07-07 14:52:43]

On 2017/07/06 07:20:40, dmazzoni wrote:
> On 2017/07/05 13:09:58, aleventhal wrote:
> > > Fixed-positioned elements don't move when the root element scrolls
> > 
> > Unfortunately this is only true if there no transforms are applied to any
> > ancestor.
> 
> Wow, good catch.
> 
> Ia added a failing test for this case but I'm not sure the correct fix. I
> assumed
> that when we called LayoutObject::IsFixedPositioned(), it would return false
> since the positioning isn't really fixed.
> 
> Adding ikilpatrick for advice here - Ian, is there a function we can call
> to determine if a LayoutObject is truly fixed-positioned, i.e. not only is
> its style position:fixed but none of its ancestors have a transform? Is
> there a high-level API for that so we don't have to reinvent that logic?

PaintLayer::fixedToViewport will tell you whether the fixed position element is
actually contained by the viewport (and hence doesn't move with scroll). In the
general case though you need to find it's container to know which scroller it
moves with (if there are nested scrollers). Also, it seems like you'll have the
same issue with position sticky, except that for sticky it sometimes moves with
scroll and sometimes doesn't. It seems really unfortunate that you have to
reproduce this positioning logic outside of blink. Can you perhaps record the
items which have special scroll movement behavior and ask blink for their
position after scroll?

[dmazzoni, 2017-07-07 15:18:49]

On 2017/07/07 14:52:43, flackr wrote:
> PaintLayer::fixedToViewport will tell you whether the fixed position element
is
> actually contained by the viewport (and hence doesn't move with scroll). In
the
> general case though you need to find it's container to know which scroller it
> moves with (if there are nested scrollers). Also, it seems like you'll have
the
> same issue with position sticky, except that for sticky it sometimes moves
with
> scroll and sometimes doesn't. It seems really unfortunate that you have to
> reproduce this positioning logic outside of blink. Can you perhaps record the
> items which have special scroll movement behavior and ask blink for their
> position after scroll?

Thanks for the info and the alternative idea! I was actually having basically
the same thought ever since Ian pointed out that "sticky" would be an
issue too, it'd be simpler to just keep track of elements with
special scroll movement behavior and update all of them any time
there's a scroll event. We could try to be more efficient than that, but as
long as the total number of such elements is typically small there's no need
to be overly clever.

Are LayoutObject::IsStickyPositioned() and LayoutObject::IsFixedPositioned()
sufficient to cover the elements that have special scroll behavior?

[flackr, 2017-07-07 17:31:42]

On 2017/07/07 15:18:49, dmazzoni wrote:
> On 2017/07/07 14:52:43, flackr wrote:
> > PaintLayer::fixedToViewport will tell you whether the fixed position element
> is
> > actually contained by the viewport (and hence doesn't move with scroll). In
> the
> > general case though you need to find it's container to know which scroller
it
> > moves with (if there are nested scrollers). Also, it seems like you'll have
> the
> > same issue with position sticky, except that for sticky it sometimes moves
> with
> > scroll and sometimes doesn't. It seems really unfortunate that you have to
> > reproduce this positioning logic outside of blink. Can you perhaps record
the
> > items which have special scroll movement behavior and ask blink for their
> > position after scroll?
> 
> Thanks for the info and the alternative idea! I was actually having basically
> the same thought ever since Ian pointed out that "sticky" would be an
> issue too, it'd be simpler to just keep track of elements with
> special scroll movement behavior and update all of them any time
> there's a scroll event. We could try to be more efficient than that, but as
> long as the total number of such elements is typically small there's no need
> to be overly clever.
> 
> Are LayoutObject::IsStickyPositioned() and LayoutObject::IsFixedPositioned()
> sufficient to cover the elements that have special scroll behavior?

Yes, although if you're looking at the scroller on the containing block chain it
may be more efficient to use PaintLayer::SticksToScroller /
PaintLayer::FixedToViewport, or even more efficient just to use blink's lists
which keep track of these. i.e. the combination of
LocalFrameView::ViewportConstrainedObjects() and on each overflow scroller,
PaintLayerScrollableArea::GetStickyConstraintsMap() will give you all of the
sticky / fixed position objects which have "special" movement for that view or
scroller.

The issue with fixed made me wonder - are you respecting the containing block
rules for position? Absolute position will not move with scroll when its
containing block is above the scroller (e.g. the red box in
http://jsbin.com/mujayikogu/edit?html,css,output is nested in the inner scroller
but does not move with it because it's contained outside of it and the red box
in http://jsbin.com/wanokib/1/edit?html,css,output only moves with the body
scroll).