Bug 167116 – innerText should replace existing text node

• Component: DOM
• Status: RESOLVED FIXED

[Simon Pieters (:zcorpan)]

From https://bugs.webkit.org/show_bug.cgi?id=160971

> WebKit will update existing text node's data if element has just 1 text node.
> The HTML spec and Gecko always replace with a new text node.

Tests:
https://github.com/w3c/web-platform-tests/pull/3493

[Chris Dumez]

I'll try it in the latest WebKit when the test lands because I thought we had fixed this already.

[Simon Pieters (:zcorpan)]

The test landed in web-platform-tests just before I filed this bug. It's possible that it's fixed already but I didn't see it in the patch for https://bugs.webkit.org/show_bug.cgi?id=160971 so...

[Chris Dumez]

Created attachment 299312 [details]
WIP Patch

WIP patch which unfortunately causes a couple of Accessibility tests to fail. I have trouble understanding why but the reason is that we no longer seem to post a AXLiveRegionChanged notification when innerText is set. I believe that previously, those tests were relying on the optimization that was re-using the existing text node and we were posting a AXLiveRegionChanged that the RenderText's text was changed. However, with this patch, we no longer have this optimization and we always remove the existing text node then create a new one. For some reason, this does not seem to post a AXLiveRegionChanged and I do not know if this is expected or not (or why).

[Chris Dumez]

For example, the test LayoutTests/accessibility/mac/aria-multiple-liveregions-notification.html does:
    function textChangeOperation() {
        // this should trigger a live region change for a text change.
        document.getElementById("innerlive").innerText = "changed text";
    }

    function newElementOperation() {
        // this should trigger a live region change for a new element.
        document.getElementById("liveregion1").innerHTML += "new text element";
    }

It expects both to cause a AXLiveRegionChanged notification. However, since both occur in close time proximity, it expects the notifications to be coalesced and get only 1 notification.

After my patch to innerText, we get 0 because setting innerText no longer causes a AXLiveRegionChanged notification. Interestingly, this means that innerHTML already did not post a AXLiveRegionChanged notification before my change.

Hopefully, Nan Wang who wrote this test understands this and can point me in the right direction.

[Build Bot]

Comment on attachment 299312 [details]
WIP Patch

Attachment 299312 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2918406

New failing tests:
accessibility/mac/aria-liveregions-changedtext.html
accessibility/mac/aria-multiple-liveregions-notification.html

[Build Bot]

Created attachment 299314 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 299312 [details]
WIP Patch

Attachment 299312 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2918432

New failing tests:
accessibility/mac/aria-liveregions-changedtext.html
accessibility/mac/aria-multiple-liveregions-notification.html

[Build Bot]

Created attachment 299318 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 299312 [details]
WIP Patch

Attachment 299312 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2918477

New failing tests:
accessibility/mac/aria-liveregions-changedtext.html
accessibility/mac/aria-multiple-liveregions-notification.html
fast/repaint/vertical-text-repaint.html

[Build Bot]

Created attachment 299319 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Nan Wang]

(In reply to comment #4)
> For example, the test
> LayoutTests/accessibility/mac/aria-multiple-liveregions-notification.html
> does:
>     function textChangeOperation() {
>         // this should trigger a live region change for a text change.
>         document.getElementById("innerlive").innerText = "changed text";
>     }
> 
>     function newElementOperation() {
>         // this should trigger a live region change for a new element.
>         document.getElementById("liveregion1").innerHTML += "new text
> element";
>     }
> 
> It expects both to cause a AXLiveRegionChanged notification. However, since
> both occur in close time proximity, it expects the notifications to be
> coalesced and get only 1 notification.
> 
> After my patch to innerText, we get 0 because setting innerText no longer
> causes a AXLiveRegionChanged notification. Interestingly, this means that
> innerHTML already did not post a AXLiveRegionChanged notification before my
> change.
> 
> Hopefully, Nan Wang who wrote this test understands this and can point me in
> the right direction.

I think the original notification was posted in RenderText::setText(), we have:
if (AXObjectCache* cache = document().existingAXObjectCache())
        cache->textChanged(this);
And also we post AXLiveRegionChanged in AccessibilityNodeObject::childrenChanged(), which was called in AXObjectCache::childrenChanged(AccessibilityObject* obj).

So in your implementation, we should either call accessibility's textChanged() on the text node or let the parent call cache->childrenChanged(this).

[Chris Dumez]

(In reply to comment #11)
> (In reply to comment #4)
> > For example, the test
> > LayoutTests/accessibility/mac/aria-multiple-liveregions-notification.html
> > does:
> >     function textChangeOperation() {
> >         // this should trigger a live region change for a text change.
> >         document.getElementById("innerlive").innerText = "changed text";
> >     }
> > 
> >     function newElementOperation() {
> >         // this should trigger a live region change for a new element.
> >         document.getElementById("liveregion1").innerHTML += "new text
> > element";
> >     }
> > 
> > It expects both to cause a AXLiveRegionChanged notification. However, since
> > both occur in close time proximity, it expects the notifications to be
> > coalesced and get only 1 notification.
> > 
> > After my patch to innerText, we get 0 because setting innerText no longer
> > causes a AXLiveRegionChanged notification. Interestingly, this means that
> > innerHTML already did not post a AXLiveRegionChanged notification before my
> > change.
> > 
> > Hopefully, Nan Wang who wrote this test understands this and can point me in
> > the right direction.
> 
> I think the original notification was posted in RenderText::setText(), we
> have:
> if (AXObjectCache* cache = document().existingAXObjectCache())
>         cache->textChanged(this);
> And also we post AXLiveRegionChanged in
> AccessibilityNodeObject::childrenChanged(), which was called in
> AXObjectCache::childrenChanged(AccessibilityObject* obj).
> 
> So in your implementation, we should either call accessibility's
> textChanged() on the text node or let the parent call
> cache->childrenChanged(this).

Yes, my patch should have caused the innerText setter to go from textChanged() to childrenChanged(). However, for some reason, the childrenChanged() is not triggered or does not trigger the notification.

By the way, I believe that before my patch the innerHTML setter already did not fire the AXLiveRegionChanged which seems like a bug. We merely did not notice because of the expected coalescing with the innerText notification.

[Nan Wang]

(In reply to comment #12)
> Yes, my patch should have caused the innerText setter to go from
> textChanged() to childrenChanged(). However, for some reason, the
> childrenChanged() is not triggered or does not trigger the notification.

I see childrenChanged() are called in
RenderElement::insertChildInternal() and RenderElement::removeChildInternal()
Not sure if these are getting called or not.

> 
> By the way, I believe that before my patch the innerHTML setter already did
> not fire the AXLiveRegionChanged which seems like a bug. We merely did not
> notice because of the expected coalescing with the innerText notification.

If that's the case we should have a separate bug for this I think.

[Chris Dumez]

(In reply to comment #13)
> (In reply to comment #12)
> > Yes, my patch should have caused the innerText setter to go from
> > textChanged() to childrenChanged(). However, for some reason, the
> > childrenChanged() is not triggered or does not trigger the notification.
> 
> I see childrenChanged() are called in
> RenderElement::insertChildInternal() and RenderElement::removeChildInternal()
> Not sure if these are getting called or not.
> 
> > 
> > By the way, I believe that before my patch the innerHTML setter already did
> > not fire the AXLiveRegionChanged which seems like a bug. We merely did not
> > notice because of the expected coalescing with the innerText notification.
> 
> If that's the case we should have a separate bug for this I think.

Interestingly, if I change that test's timer to use 100ms instead of 10ms, then I get 3 AXLiveRegionChanged instead of the expected 2. So the notifications get fired eventually.

[Chris Dumez]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > Yes, my patch should have caused the innerText setter to go from
> > > textChanged() to childrenChanged(). However, for some reason, the
> > > childrenChanged() is not triggered or does not trigger the notification.
> > 
> > I see childrenChanged() are called in
> > RenderElement::insertChildInternal() and RenderElement::removeChildInternal()
> > Not sure if these are getting called or not.
> > 
> > > 
> > > By the way, I believe that before my patch the innerHTML setter already did
> > > not fire the AXLiveRegionChanged which seems like a bug. We merely did not
> > > notice because of the expected coalescing with the innerText notification.
> > 
> > If that's the case we should have a separate bug for this I think.
> 
> Interestingly, if I change that test's timer to use 100ms instead of 10ms,
> then I get 3 AXLiveRegionChanged instead of the expected 2. So the
> notifications get fired eventually.

What I see with the 100ms timer delay (instead of 10ms) is that innerText setter fires 2 AXLiveRegionChanged notifications and innerHTML setter fires 2 AXLiveRegionChanged as well. When I run innerText and innerHTML setter one after the one, I get only 2 AXLiveRegionChanged notifications, meaning that there was coalescing happening (from 4 to 2). However, the test is only expecting 1. Is it expected that we get 2 here?

We use to just change the text previously so this would obviously fire a single notification. Now we remove the Text child then add a new Text child with the new text. Technically we do more operations although I would expect coalescing to merge them still?

[Nan Wang]

(In reply to comment #15)
> (In reply to comment #14)
> > Interestingly, if I change that test's timer to use 100ms instead of 10ms,
> > then I get 3 AXLiveRegionChanged instead of the expected 2. So the
> > notifications get fired eventually.
> 
> What I see with the 100ms timer delay (instead of 10ms) is that innerText
> setter fires 2 AXLiveRegionChanged notifications and innerHTML setter fires
> 2 AXLiveRegionChanged as well. When I run innerText and innerHTML setter one
> after the one, I get only 2 AXLiveRegionChanged notifications, meaning that
> there was coalescing happening (from 4 to 2). However, the test is only
> expecting 1. Is it expected that we get 2 here?
> 
> We use to just change the text previously so this would obviously fire a
> single notification. Now we remove the Text child then add a new Text child
> with the new text. Technically we do more operations although I would expect
> coalescing to merge them still?

Hmmm, so the AccessibilityLiveRegionChangedNotificationInterval is 20ms. I guess maybe the innerHTML setter operation takes longer than the interval that some notifications are not coalesced.

[Chris Dumez]

Created attachment 299382 [details]
WIP Patch

[Chris Dumez]

Comment on attachment 299382 [details]
WIP Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=299382&action=review

Making some progress but there is still 1 accessibility test failing.

> LayoutTests/accessibility/mac/aria-multiple-liveregions-notification.html:50
> +        setTimeout("finishTest()", 50);

Increasing timeout to make sure it gets all notifications. When doing so, I started getting 3 instead of the expected 2.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2946
> +            cache->postLiveRegionChangeNotification(parent);

This code was bypassing the AXLiveRegionChanged notification coalescing, causing the test above to fail.

[Nan Wang]

Comment on attachment 299382 [details]
WIP Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=299382&action=review

>> LayoutTests/accessibility/mac/aria-multiple-liveregions-notification.html:50
>> +        setTimeout("finishTest()", 50);
> 
> Increasing timeout to make sure it gets all notifications. When doing so, I started getting 3 instead of the expected 2.

Is the extra one coming from liveregion1 ?  If so, would increasing the AccessibilityLiveRegionChangedNotificationInterval make it work?

[Chris Dumez]

Created attachment 299392 [details]
Patch

[Chris Dumez]

(In reply to comment #19)
> Comment on attachment 299382 [details]
> WIP Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=299382&action=review
> 
> >> LayoutTests/accessibility/mac/aria-multiple-liveregions-notification.html:50
> >> +        setTimeout("finishTest()", 50);
> > 
> > Increasing timeout to make sure it gets all notifications. When doing so, I started getting 3 instead of the expected 2.
> 
> Is the extra one coming from liveregion1 ?  If so, would increasing the
> AccessibilityLiveRegionChangedNotificationInterval make it work?

No, as I explained above, the extra one was coming from AccessibilityRenderObject::textChanged() which was calling postNotification(renderParent, AXObjectCache::AXLiveRegionChanged) directly, thus bypassing the coalescing. Calling postLiveRegionChangeNotification(parent) instead fixes the issue.

Could you take a look at the accessibility changes in the latest patch iteration and let me know if it looks alright?

[Build Bot]

Comment on attachment 299392 [details]
Patch

Attachment 299392 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2922423

New failing tests:
fast/repaint/vertical-text-repaint.html

[Build Bot]

Created attachment 299397 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Chris Dumez]

Created attachment 299400 [details]
Patch

[Nan Wang]

(In reply to comment #21)
> No, as I explained above, the extra one was coming from
> AccessibilityRenderObject::textChanged() which was calling
> postNotification(renderParent, AXObjectCache::AXLiveRegionChanged) directly,
> thus bypassing the coalescing. Calling
> postLiveRegionChangeNotification(parent) instead fixes the issue.
> 
> Could you take a look at the accessibility changes in the latest patch
> iteration and let me know if it looks alright?

The accessibility change looks good to me. Thanks.

[Chris Dumez]

(In reply to comment #25)
> (In reply to comment #21)
> > No, as I explained above, the extra one was coming from
> > AccessibilityRenderObject::textChanged() which was calling
> > postNotification(renderParent, AXObjectCache::AXLiveRegionChanged) directly,
> > thus bypassing the coalescing. Calling
> > postLiveRegionChangeNotification(parent) instead fixes the issue.
> > 
> > Could you take a look at the accessibility changes in the latest patch
> > iteration and let me know if it looks alright?
> 
> The accessibility change looks good to me. Thanks.

Thanks, did you look at the changes to both accessibility tests as well?

[Nan Wang]

(In reply to comment #26)
> (In reply to comment #25)
> > (In reply to comment #21)
> > > No, as I explained above, the extra one was coming from
> > > AccessibilityRenderObject::textChanged() which was calling
> > > postNotification(renderParent, AXObjectCache::AXLiveRegionChanged) directly,
> > > thus bypassing the coalescing. Calling
> > > postLiveRegionChangeNotification(parent) instead fixes the issue.
> > > 
> > > Could you take a look at the accessibility changes in the latest patch
> > > iteration and let me know if it looks alright?
> > 
> > The accessibility change looks good to me. Thanks.
> 
> Thanks, did you look at the changes to both accessibility tests as well?

Yes.

[Darin Adler]

Comment on attachment 299400 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=299400&action=review

> LayoutTests/fast/repaint/vertical-text-repaint.html:99
> +    debug(window.internals.repaintRectsAsText());
> +    shouldNotBe("window.internals.repaintRectsAsText().indexOf('25 25')", "-1");
> +    shouldNotBe("window.internals.repaintRectsAsText().indexOf('25 155')", "-1");
> +    shouldNotBe("window.internals.repaintRectsAsText().indexOf('25 285')", "-1");
> +    shouldNotBe("window.internals.repaintRectsAsText().indexOf('155 25')", "-1");
> +    shouldNotBe("window.internals.repaintRectsAsText().indexOf('155 155')", "-1");
> +    shouldNotBe("window.internals.repaintRectsAsText().indexOf('155 285')", "-1");

These should all just be "internals", no need for "window.internals".

[Chris Dumez]

Created attachment 299452 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 299452 [details]
Patch

Rejecting attachment 299452 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 299452, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/imported/w3c/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/2928208

[Chris Dumez]

Created attachment 299453 [details]
Patch

[Chris Dumez]

Comment on attachment 299453 [details]
Patch

Clearing flags on attachment: 299453

Committed r211023: <http://trac.webkit.org/changeset/211023>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.