Send AXLayoutComplete even when document hasn't finished loading yet (reland)

Send AXLayoutComplete even when document hasn't finished loading yet (reland)

Because these notifications only got sent if the corresponding AXObject
already existed, they were effectively never sent before the first
load complete notification. This change creates the AXObject for the
document element after the first layout, allowing future notifications too.

BUG=353067
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170085

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170343

[eseidel, 2014-03-17 15:40:08]

The first layout may be very incomplete.

I would expect that AX would want to use the same "flash of unstyled content"
avoidance that the visual parts of blink do as well.

For example, we only paint white until all stylesheets are loaded.  We may do
many layouts (forced by grabbing element.offsetTop, etc) before that.  Are you
sure you want screen reader users to see that content (visual users can't).

[eseidel, 2014-03-17 15:41:31]

https://codereview.chromium.org/199693003/diff/1/Source/core/accessibility/AX...
File Source/core/accessibility/AXObjectCache.cpp (right):

https://codereview.chromium.org/199693003/diff/1/Source/core/accessibility/AX...
Source/core/accessibility/AXObjectCache.cpp:815: // allows an AX notification to
be sent when a page has its first layout, rather than when the
The document may be styled by the UA stylesheet at this point and may be very
far from it's eventual styles/renderer types.

[dmazzoni, 2014-03-17 15:52:52]

On 2014/03/17 15:40:08, eseidel wrote:
> The first layout may be very incomplete.
> 
> I would expect that AX would want to use the same "flash of unstyled content"
> avoidance that the visual parts of blink do as well.
> 
> For example, we only paint white until all stylesheets are loaded.  We may do
> many layouts (forced by grabbing element.offsetTop, etc) before that.  Are you
> sure you want screen reader users to see that content (visual users can't).

I tested it and this fixed pathological cases (where the page is 99% loaded but
onload hasn't fired yet) and the experience for average pages was fine. But
I'd be happy to delay until the first paint, that makes perfect sense - what'd
be the best place to check for that?

[eseidel, 2014-03-17 16:59:36]

The correct solution here is for us to unify all our FOUC logic into a state on
the DocumentLifecycle.  Then both the visual and non-visual representations of
the page could go through the same states instead of AX starting to expose
information about the page at a differnet time from when the visual systems do.

[eseidel, 2014-03-17 17:01:51]

We could land this, but then we'll just get different bugs.  If you have a page
with a slow stylesheet which has some JS which calls document.body.offsetTop (or
any other property which triggers layout), you'll be able to AX-interact with
the page, before we ever paint the real pixels of the page (or have the right
style information for the page).

[dmazzoni, 2014-03-24 05:09:40]

+esprehn

Now waiting until "first visually nonempty layout"

[esprehn, 2014-03-24 09:12:56]

lgtm to me, but no tests?

You might also want to update the description.

[dmazzoni, 2014-03-24 15:41:53]

I updated the change description.

How would I test this? Can you think of a similar test I could use
as a guide?

To do this from a layout test I'd need a way to prevent a document
from finishing loading without stalling the main event loop. For manual
testing I did this by modifying a server to delay sending a script
resource.

[esprehn, 2014-03-24 15:51:16]

On 2014/03/24 15:41:53, dmazzoni wrote:
> I updated the change description.
> 
> How would I test this? Can you think of a similar test I could use
> as a guide?
> 
> To do this from a layout test I'd need a way to prevent a document
> from finishing loading without stalling the main event loop. For manual
> testing I did this by modifying a server to delay sending a script
> resource.

You should write an http/ test that hangs for a while when sending the main
document so you can get progressive rendering. abarth@ can probably help. I'm
okay landing this as you sort out the test if you're confident. You could also
add something to ManaulTests/ if you have one.

[dmazzoni, 2014-03-25 17:56:45]

After doing some more testing it wasn't working very well. I looked into how
isVisuallyNonEmpty is computed - it's set when more than 200 characters or more
than 1024 pixels worth of image data has been added to the document. This seems
pretty arbitrary, and it doesn't seem to address Eric's concern, which was that
a layout can be triggered by JavaScript (offsetTop, etc.). It also doesn't
completely address the problem I was trying to solve if the page happens to be
small (<200 characters with no images is definitely possible, especially on
mobile sites!)

I think a better heuristic might be to wait for the first paint. Any objections
to that? I'm going to try that change and add a test too.

[dmazzoni, 2014-03-25 21:04:27]

...which proves hard to test, since painting doesn't happen in layout tests
unless explicitly requested.

[esprehn, 2014-03-25 21:06:30]

On 2014/03/25 21:04:27, dmazzoni wrote:
> ...which proves hard to test, since painting doesn't happen in layout tests
> unless explicitly requested.

I don't think painting is easy either, since we paint the entire screen white
while stylesheets are loading.

[dmazzoni, 2014-03-25 22:08:13]

OK, I went back to the original change, but with a test this time. I think this
is the best step forward.

[dmazzoni, 2014-03-26 18:12:51]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-03-26 18:12:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/199693003/70001

[commit-bot: I haz the power, 2014-03-26 18:48:13]

Change committed as 170085

[enne (OOO), 2014-03-27 00:07:53]

A revert of this CL has been created in
https://codereview.chromium.org/213883003/ by enne@chromium.org.

The reason for reverting is: Causes content_browertests to fail:

[ RUN      ] DumpAccessibilityTreeTest.AccessibilityModalDialogInIframeOpened
Testing:
E:\b\build\slave\Win7_Tests\build\src\content\test\data\accessibility\modal-dialog-in-iframe-opened.html
c:\b\build\slave\win-latest-rel\build\src\content\browser\accessibility\dump_accessibility_tree_browsertest.cc(209):
error: Value of: is_different
  Actual: true
Expected: false
* Line Expected
- ---- --------
     1 ROLE_SYSTEM_DOCUMENT READONLY FOCUSABLE
     2     IA2_ROLE_SECTION READONLY
     3         ROLE_SYSTEM_STATICTEXT name='Test for modal dialog opened in an
iframe. '
     4         ROLE_SYSTEM_CLIENT FOCUSABLE
     5             IA2_ROLE_SCROLL_PANE
     6                 ROLE_SYSTEM_DOCUMENT READONLY FOCUSABLE
     7                     ROLE_SYSTEM_DIALOG READONLY
     8                         ROLE_SYSTEM_STATICTEXT name='Text in the dialog.'
*    9                 ROLE_SYSTEM_SCROLLBAR
    10 <-- End-of-file -->

Actual
------
ROLE_SYSTEM_DOCUMENT READONLY FOCUSABLE
    IA2_ROLE_SECTION READONLY
        ROLE_SYSTEM_STATICTEXT name='Test for modal dialog opened in an iframe.
'
        ROLE_SYSTEM_CLIENT FOCUSABLE
            IA2_ROLE_SCROLL_PANE
                ROLE_SYSTEM_DOCUMENT READONLY FOCUSABLE
                    ROLE_SYSTEM_DIALOG READONLY
                        ROLE_SYSTEM_STATICTEXT name='Text in the dialog.'.

[dmazzoni, 2014-03-28 16:44:47]

Re-landing. The test that broke when this landed has been disabled;
i'll fix it to be more robust after this lands.

[dmazzoni, 2014-03-28 17:03:17]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-03-28 17:03:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/199693003/130001

[commit-bot: I haz the power, 2014-03-28 18:06:12]

Change committed as 170343