Bug 173912 – Safari's Speedometer score massively regresses when accessibility is enabled

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Ryosuke Niwa]

When the accessibility tree is turned on, we trigger a synchronous layout in HTMLTextFormControlElement::setInnerTextValue. We need to avoid that.

<rdar://problem/32686777>

[Ryosuke Niwa]

Created attachment 314001 [details]
WIP

[Ryosuke Niwa]

Created attachment 314002 [details]
WIP

[Ryosuke Niwa]

Created attachment 314067 [details]
Fixes the bug

[chris fleizach]

Comment on attachment 314067 [details]
Fixes the bug

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

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:699
> +    auto *cache = m_object->axObjectCache();

we probably shouldn't assert cache here, it is possible to have a null cache (happens when our wrapper outlives the backing object), in which can we should return a nil value

[Ryosuke Niwa]

Comment on attachment 314067 [details]
Fixes the bug

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

>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:699
>> +    auto *cache = m_object->axObjectCache();
> 
> we probably shouldn't assert cache here, it is possible to have a null cache (happens when our wrapper outlives the backing object), in which can we should return a nil value

Okay. I can fix that.

[Ryosuke Niwa]

Created attachment 314070 [details]
Addressed Chris's comment

[chris fleizach]

Comment on attachment 314070 [details]
Addressed Chris's comment

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

small suggestions. otherwise change looks ok to me

> Source/WebCore/accessibility/AXObjectCache.cpp:2170
> +    textMarkerData.offset = 0;

these few lines should be unnecessary now that you initialize them

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:412
> +    if (position.isNotNull()) {

should use early return here

[Ryosuke Niwa]

Created attachment 314074 [details]
Patch for landing

[Ryosuke Niwa]

Thanks for the review!

(In reply to chris fleizach from comment #7)
> Comment on attachment 314070 [details]
> Addressed Chris's comment
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=314070&action=review
> 
> small suggestions. otherwise change looks ok to me
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:2170
> > +    textMarkerData.offset = 0;
> 
> these few lines should be unnecessary now that you initialize them

Removed.

> > Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:412
> > +    if (position.isNotNull()) {
> 
> should use early return here

Fixed.

[Ryosuke Niwa]

Comment on attachment 314074 [details]
Patch for landing

Wait for EWS.

[Ryosuke Niwa]

Created attachment 314075 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 314075 [details]
Patch for landing

Wait for EWS again.

[Ryosuke Niwa]

Created attachment 314085 [details]
Another GTK+/WPE build fix

[Ryosuke Niwa]

Created attachment 314087 [details]
Patch for landing

[Ryosuke Niwa]

Committed r218910: <http://trac.webkit.org/changeset/218910>

[Daniel Bates]

(In reply to Ryosuke Niwa from comment #15)
> Committed r218910: <http://trac.webkit.org/changeset/218910>

I am able to hit ASSERT(LayoutDisallowedScope::isLayoutAllowed()), added in this commit, whenever I have VoiceOver enabled and update the value of text field. See bug #174559.

[Daniel Bates]

(In reply to Daniel Bates from comment #16)
> (In reply to Ryosuke Niwa from comment #15)
> > Committed r218910: <http://trac.webkit.org/changeset/218910>
> 
> I am able to hit ASSERT(LayoutDisallowedScope::isLayoutAllowed()), added in
> this commit, whenever I have VoiceOver enabled and update the value of text
> field. See bug #174559.

*whenever I have VoiceOver enabled and update the value of a text field from a mouseup event listener.