Bug 144447 – [GTK] REGRESSION(183368): It made editing tests assert

• Component: New Bugs
• Status: RESOLVED FIXED

[Csaba Osztrogonác]

before: https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug%20%28Tests%29/builds/4350
after: https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug%20%28Tests%29/builds/4351

STDERR: ASSERTION FAILED: type != AXTextEditTypeUnknown
STDERR: ../../Source/WebCore/accessibility/AXObjectCache.cpp(1024) : void WebCore::AXObjectCache::postTextStateChangeNotification(WebCore::Node*, WebCore::AXTextEditType, const WTF::String&, const WebCore::VisiblePosition&)

[Doug Russell]

Do you have the rest of the stack trace for the assert?

[Martin Robinson]

Here's an example of the full stack:


STDERR: ASSERTION FAILED: type != AXTextEditTypeUnknown
STDERR: ../../Source/WebCore/accessibility/AXObjectCache.cpp(1024) : void WebCore::AXObjectCache::postTextStateChangeNotification(WebCore::Node*, WebCore::AXTextEditType, const WTF::String&, const WebCore::VisiblePosition&)
STDERR: 1   0x7fed55778b8e /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(WTFCrash+0x1e) [0x7fed55778b8e]
STDERR: 2   0x7fed5afa005c /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::AXObjectCache::postTextStateChangeNotification(WebCore::Node*, WebCore::AXTextEditType, WTF::String const&, WebCore::VisiblePosition const&)+0x52) [0x7fed5afa005c]
STDERR: 3   0x7fed5b466bce /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::SimpleEditCommand::notifyAccessibilityForTextChange(WebCore::Node*, WebCore::AXTextEditType, WTF::String const&, WebCore::VisiblePosition const&)+0x68) [0x7fed5b466bce]
STDERR: 4   0x7fed5b4adaf6 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::InsertNodeBeforeCommand::doApply()+0x2d0) [0x7fed5b4adaf6]
STDERR: 5   0x7fed5b44a59e /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::CompositeEditCommand::applyCommandToComposite(WTF::PassRefPtr<WebCore::EditCommand>)+0x5a) [0x7fed5b44a59e]
STDERR: 6   0x7fed5b44ad20 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::CompositeEditCommand::insertNodeBefore(WTF::PassRefPtr<WebCore::Node>, WTF::PassRefPtr<WebCore::Node>, WebCore::ShouldAssumeContentIsAlwaysEditable)+0x8c) [0x7fed5b44ad20]
STDERR: 7   0x7fed5b49582f /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::FormatBlockCommand::formatRange(WebCore::Position const&, WebCore::Position const&, WebCore::Position const&, WTF::RefPtr<WebCore::Element>&)+0x50d) [0x7fed5b49582f]
STDERR: 8   0x7fed5b434521 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::ApplyBlockElementCommand::formatSelection(WebCore::VisiblePosition const&, WebCore::VisiblePosition const&)+0x995) [0x7fed5b434521]
STDERR: 9   0x7fed5b4952f4 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::FormatBlockCommand::formatSelection(WebCore::VisiblePosition const&, WebCore::VisiblePosition const&)+0x62) [0x7fed5b4952f4]
STDERR: 10  0x7fed5b433805 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::ApplyBlockElementCommand::doApply()+0x303) [0x7fed5b433805]
STDERR: 11  0x7fed5b44a313 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::CompositeEditCommand::apply()+0xc7) [0x7fed5b44a313]
STDERR: 12  0x7fed5b44a0d3 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::applyCommand(WTF::PassRefPtr<WebCore::CompositeEditCommand>)+0x20) [0x7fed5b44a0d3]
STDERR: 13  0x7fed5b48e9b3 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(+0x4d699b3) [0x7fed5b48e9b3]
STDERR: 14  0x7fed5b492a57 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const+0xdb) [0x7fed5b492a57]
STDERR: 15  0x7fed5b31993e /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&)+0x4a) [0x7fed5b31993e]
STDERR: 16  0x7fed5c390649 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::jsDocumentPrototypeFunctionExecCommand(JSC::ExecState*)+0x2f4) [0x7fed5c390649]
STDERR: 17  0x7fecfffff0a8 [0x7fecfffff0a8]

[Doug Russell]

Any idea what the editingAction is for FormatBlockCommand? Doesn't look like it's being set.

[Doug Russell]

Never mind, it's right in the header, EditActionFormatBlock. We probably should add a text edit type for generic style attributes changes and use that for format block.

[Doug Russell]

If this is blocking you and you want to land something right away, I'd be ok with mapping format to insert as a stopgap.

[Joanmarie Diggs]

(In reply to comment #5)
> If this is blocking you and you want to land something right away, I'd be ok
> with mapping format to insert as a stopgap.

I am just now looking into this. If an immediate stopgap is called for, I'd rather it be to not notify platforms of unknown types. If we don't know the type, the platform accessibility APIs won't either. 

I agree re text attribute changes.

[Joanmarie Diggs]

Doug save me some grepping and googling please. :)

1. Does your platform have any type of text attributes changed notification. Mine does. If yours does too, I'll make a cross platform test.

2. There's a "drag" edit action. I don't see that being handled. Did I miss that? And I don't yet have any thoughts for doing so.

3. Not sure about CreateLink and Unlink. That would presumably result in text attribute changes so maybe it could go there. But that wouldn't indicate the link insertion? Thoughts about that one?

Lastly, once I get the crashes stopped, we can move the other things into new bugs. Item 1 looks like it needs to be part of the crash fix. Don't yet know about the other 2.

[Doug Russell]

(In reply to comment #7)
> Doug save me some grepping and googling please. :)
> 
> 1. Does your platform have any type of text attributes changed notification.
> Mine does. If yours does too, I'll make a cross platform test.

We have a generic Value Changed notification and you can attach a key/value payload, which is how the existing TextEditTypes are posted. So if we add a new style edit type it would be included.

> 
> 2. There's a "drag" edit action. I don't see that being handled. Did I miss
> that? And I don't yet have any thoughts for doing so.

This is definitely something that should have a text edit type, I just haven't implemented it yet.

> 
> 3. Not sure about CreateLink and Unlink. That would presumably result in
> text attribute changes so maybe it could go there. But that wouldn't
> indicate the link insertion? Thoughts about that one?

I'm up in the air on if this one is an attribute change or a replace.

> 
> Lastly, once I get the crashes stopped, we can move the other things into
> new bugs. Item 1 looks like it needs to be part of the crash fix. Don't yet
> know about the other 2.

[Doug Russell]

Created attachment 252088 [details]
style

Here's a very naive pass at adding a style edit type

[Joanmarie Diggs]

(In reply to comment #9)
> Created attachment 252088 [details]
> style
> 
> Here's a very naive pass at adding a style edit type

I'm actually working on that one already. Sorry! And it's not all that needs to be done to stop these crashes.

[Joanmarie Diggs]

Created attachment 252098 [details]
Patch

[Joanmarie Diggs]

I still need to build Safari with the attached patch (will do that next). I haven't yet created a new layout test for the text-attributes-changed signal for ATK. I'll do that soon. There are other, non-editing crashes (like the canvas fallback test mentioned in bug 142719) which are NOT solved by the attached. I will look into that in between Safari and the new layout test.

In the meantime, Doug: does the attached seem right to you?

In addition, I'm seeing two editing tests which are still crashing: execCommand/paste-1.html and pasteboard/cut-text-001.html which are crashing because they are reaching an ASSERT_NOT_REACHED for AXTextEditTypeCut. Should that be an assertion, or just not a cause for a notification?

[Doug Russell]

Looks good to me. I'll give it a test run locally.

Do you have traces for the crashing paste tests?

[Joanmarie Diggs]

STDERR: SHOULD NEVER BE REACHED
STDERR: ../../Source/WebCore/editing/ReplaceInsertIntoTextNodeCommand.cpp(54) : virtual void WebCore::ReplaceInsertIntoTextNodeCommand::notifyAccessibilityForTextChange(WebCore::Node*, WebCore::AXTextEditType, const WTF::String&, const WebCore::VisiblePosition&)
STDERR: 1   0x7fefe2ce59af /home/jd/checkout/WebKitGtk/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(WTFCrash+0x1e) [0x7fefe2ce59af]
STDERR: 2   0x7fefe8a7013b /home/jd/checkout/WebKitGtk/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::ReplaceInsertIntoTextNodeCommand::notifyAccessibilityForTextChange(WebCore::Node*, WebCore::AXTextEditType, WTF::String const&, WebCore::VisiblePosition const&)+0x9f) [0x7fefe8a7013b]
STDERR: 3   0x7fefe8a5a395 /home/jd/checkout/WebKitGtk/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::InsertIntoTextNodeCommand::doApply()+0x233) [0x7fefe8a5a395]
STDERR: 4   0x7fefe89fd0dd /home/jd/checkout/WebKitGtk/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::CompositeEditCommand::applyCommandToComposite(WTF::PassRefPtr<WebCore::EditCommand>)+0x5d) [0x7fefe89fd0dd]
STDERR: 5   0x7fefe89ff97b /home/jd/checkout/WebKitGtk/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::CompositeEditCommand::replaceTextInNode(WTF::PassRefPtr<WebCore::Text>, unsigned int, unsigned int, WTF::String const&)+0x145) [0x7fefe89ff97b]
STDERR: 6   0x7fefe89ffe88 /home/jd/checkout/WebKitGtk/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::CompositeEditCommand::replaceTextInNodePreservingMarkers(WTF::PassRefPtr<WebCore::Text>, unsigned int, unsigned int, WTF::String const&)+0x1bc) [0x7fefe89ffe88]
STDERR: 7   0x7fefe8a14514 /home/jd/checkout/WebKitGtk/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::DeleteSelectionCommand::fixupWhitespace()+0x274) [0x7fefe8a14514]
STDERR: 8   0x7fefe8a16450 /home/jd/checkout/WebKitGtk/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::DeleteSelectionCommand::doApply()+0x494) [0x7fefe8a16450]
STDERR: 9   0x7fefe89fce4d /home/jd/checkout/WebKitGtk/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::CompositeEditCommand::apply()+0xc7) [0x7fefe89fce4d]
STDERR: 10  0x7fefe89fcc23 /home/jd/checkout/WebKitGtk/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::applyCommand(WTF::PassRefPtr<WebCore::CompositeEditCommand>)+0x20) [0x7fefe89fcc23]
STDERR: 11  0x7fefe8a29027 /home/jd/checkout/WebKitGtk/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::Editor::deleteSelectionWithSmartDelete(bool, WebCore::EditAction)+0x8f) [0x7fefe8a29027]
STDERR: 12  0x7fefe8a2d6d0 /home/jd/checkout/WebKitGtk/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::Editor::performCutOrCopy(WebCore::Editor::EditorActionSpecifier)+0x2f2) [0x7fefe8a2d6d0]
STDERR: 13  0x7fefe8a2d38d /home/jd/checkout/WebKitGtk/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::Editor::cut()+0x47) [0x7fefe8a2d38d]
STDERR: 14  0x7fefe8a407ee /home/jd/checkout/WebKitGtk/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(+0x4dd67ee) [0x7fefe8a407ee]
STDERR: 15  0x7fefe8a452c0 /home/jd/checkout/WebKitGtk/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const+0xde) [0x7fefe8a452c0]
STDERR: 16  0x7fefe88cc026 /home/jd/checkout/WebKitGtk/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&)+0x4a) [0x7fefe88cc026]
STDERR: 17  0x7fefe9936d06 /home/jd/checkout/WebKitGtk/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::jsDocumentPrototypeFunctionExecCommand(JSC::ExecState*)+0x2f7) [0x7fefe9936d06]
STDERR: 18  0x7fef83fff0a8 [0x7fef83fff0a8]

[Joanmarie Diggs]

(In reply to comment #13)
> Looks good to me. I'll give it a test run locally.

Thanks! If your test run goes smoothly, then let's ask Chris about the names for the new types. I saw you had gone with 'Style'. I had gone with 'Format' simply because I'd made the change before seeing your patch. And I went with AXTextAttributesChanged because that's what it's called on my platform. Maybe Chris has better ideas.

> Do you have traces for the crashing paste tests?

See previous comment.

[Doug Russell]

(In reply to comment #14)
> STDERR: SHOULD NEVER BE REACHED
> STDERR:
> ../../Source/WebCore/editing/ReplaceInsertIntoTextNodeCommand.cpp(54) :
> virtual void
> WebCore::ReplaceInsertIntoTextNodeCommand::
> notifyAccessibilityForTextChange(WebCore::Node*, WebCore::AXTextEditType,
> const WTF::String&, const WebCore::VisiblePosition&)
> STDERR: 1   0x7fefe2ce59af
> /home/jd/checkout/WebKitGtk/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.
> so.18(WTFCrash+0x1e) [0x7fefe2ce59af]
> STDERR: 2   0x7fefe8a7013b
> /home/jd/checkout/WebKitGtk/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.
> 37(WebCore::ReplaceInsertIntoTextNodeCommand::
> notifyAccessibilityForTextChange(WebCore::Node*, WebCore::AXTextEditType,
> WTF::String const&, WebCore::VisiblePosition const&)+0x9f) [0x7fefe8a7013b]
> STDERR: 3   0x7fefe8a5a395
> /home/jd/checkout/WebKitGtk/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.
> 37(WebCore::InsertIntoTextNodeCommand::doApply()+0x233) [0x7fefe8a5a395]
> STDERR: 4   0x7fefe89fd0dd
> /home/jd/checkout/WebKitGtk/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.
> 37(WebCore::CompositeEditCommand::applyCommandToComposite(WTF::
> PassRefPtr<WebCore::EditCommand>)+0x5d) [0x7fefe89fd0dd]
> STDERR: 5   0x7fefe89ff97b
> /home/jd/checkout/WebKitGtk/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.
> 37(WebCore::CompositeEditCommand::replaceTextInNode(WTF::PassRefPtr<WebCore::
> Text>, unsigned int, unsigned int, WTF::String const&)+0x145)
> [0x7fefe89ff97b]
> STDERR: 6   0x7fefe89ffe88
> /home/jd/checkout/WebKitGtk/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.
> 37(WebCore::CompositeEditCommand::replaceTextInNodePreservingMarkers(WTF::
> PassRefPtr<WebCore::Text>, unsigned int, unsigned int, WTF::String
> const&)+0x1bc) [0x7fefe89ffe88]
> STDERR: 7   0x7fefe8a14514
> /home/jd/checkout/WebKitGtk/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.
> 37(WebCore::DeleteSelectionCommand::fixupWhitespace()+0x274) [0x7fefe8a14514]
> STDERR: 8   0x7fefe8a16450
> /home/jd/checkout/WebKitGtk/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.
> 37(WebCore::DeleteSelectionCommand::doApply()+0x494) [0x7fefe8a16450]
> STDERR: 9   0x7fefe89fce4d
> /home/jd/checkout/WebKitGtk/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.
> 37(WebCore::CompositeEditCommand::apply()+0xc7) [0x7fefe89fce4d]
> STDERR: 10  0x7fefe89fcc23
> /home/jd/checkout/WebKitGtk/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.
> 37(WebCore::applyCommand(WTF::PassRefPtr<WebCore::
> CompositeEditCommand>)+0x20) [0x7fefe89fcc23]
> STDERR: 11  0x7fefe8a29027
> /home/jd/checkout/WebKitGtk/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.
> 37(WebCore::Editor::deleteSelectionWithSmartDelete(bool,
> WebCore::EditAction)+0x8f) [0x7fefe8a29027]
> STDERR: 12  0x7fefe8a2d6d0
> /home/jd/checkout/WebKitGtk/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.
> 37(WebCore::Editor::performCutOrCopy(WebCore::Editor::
> EditorActionSpecifier)+0x2f2) [0x7fefe8a2d6d0]
> STDERR: 13  0x7fefe8a2d38d
> /home/jd/checkout/WebKitGtk/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.
> 37(WebCore::Editor::cut()+0x47) [0x7fefe8a2d38d]
> STDERR: 14  0x7fefe8a407ee
> /home/jd/checkout/WebKitGtk/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.
> 37(+0x4dd67ee) [0x7fefe8a407ee]
> STDERR: 15  0x7fefe8a452c0
> /home/jd/checkout/WebKitGtk/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.
> 37(WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*)
> const+0xde) [0x7fefe8a452c0]
> STDERR: 16  0x7fefe88cc026
> /home/jd/checkout/WebKitGtk/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.
> 37(WebCore::Document::execCommand(WTF::String const&, bool, WTF::String
> const&)+0x4a) [0x7fefe88cc026]
> STDERR: 17  0x7fefe9936d06
> /home/jd/checkout/WebKitGtk/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.
> 37(WebCore::jsDocumentPrototypeFunctionExecCommand(JSC::ExecState*)+0x2f7)
> [0x7fefe9936d06]
> STDERR: 18  0x7fef83fff0a8 [0x7fef83fff0a8]

So that's white space being inserted as part of prep for a cut, so the editingAction passed to ReplaceInsertIntoTextNodeCommand should be insert, but right now it's taking it's parents editingAction so that it's correct plain insert vs paste insert, etc. This one is trick y. :)

[Doug Russell]

Since it's just white space, the right thing for now is probably just to not notify on replaces that occur during cuts.

[Doug Russell]

(In reply to comment #15)
> (In reply to comment #13)
> > Looks good to me. I'll give it a test run locally.
> 
> Thanks! If your test run goes smoothly, then let's ask Chris about the names
> for the new types. I saw you had gone with 'Style'. I had gone with 'Format'
> simply because I'd made the change before seeing your patch. And I went with
> AXTextAttributesChanged because that's what it's called on my platform.
> Maybe Chris has better ideas.

Style Format and AttributesChanged are all probably fine. AttributesChanged is probably the most broad so maybe that's the way to go.

[Joanmarie Diggs]

(In reply to comment #17)
> Since it's just white space, the right thing for now is probably just to not
> notify on replaces that occur during cuts.

Ok. In order to prevent patch collision like before, shall I do a new patch with that change, or are you planning to?

(In reply to comment #18)
> (In reply to comment #15)
> > (In reply to comment #13)
> > > Looks good to me. I'll give it a test run locally.
> > 
> > Thanks! If your test run goes smoothly, then let's ask Chris about the names
> > for the new types. I saw you had gone with 'Style'. I had gone with 'Format'
> > simply because I'd made the change before seeing your patch. And I went with
> > AXTextAttributesChanged because that's what it's called on my platform.
> > Maybe Chris has better ideas.
> 
> Style Format and AttributesChanged are all probably fine. AttributesChanged
> is probably the most broad so maybe that's the way to go.

If you mean AXAttributesChanged instead of AXTextAttributesChanged, that would be inconsistent with AXTextInserted and AXTextDeleted. Plus, at least on my platform, there are text attributes and object attributes.

[Doug Russell]

(In reply to comment #19)
> (In reply to comment #17)
> > Since it's just white space, the right thing for now is probably just to not
> > notify on replaces that occur during cuts.
> 
> Ok. In order to prevent patch collision like before, shall I do a new patch
> with that change, or are you planning to?

I'll leave it to you.

> 
> (In reply to comment #18)
> > (In reply to comment #15)
> > > (In reply to comment #13)
> > > > Looks good to me. I'll give it a test run locally.
> > > 
> > > Thanks! If your test run goes smoothly, then let's ask Chris about the names
> > > for the new types. I saw you had gone with 'Style'. I had gone with 'Format'
> > > simply because I'd made the change before seeing your patch. And I went with
> > > AXTextAttributesChanged because that's what it's called on my platform.
> > > Maybe Chris has better ideas.
> > 
> > Style Format and AttributesChanged are all probably fine. AttributesChanged
> > is probably the most broad so maybe that's the way to go.
> 
> If you mean AXAttributesChanged instead of AXTextAttributesChanged, that
> would be inconsistent with AXTextInserted and AXTextDeleted. Plus, at least
> on my platform, there are text attributes and object attributes.

I was thinking leave AXTextAttributesChanged as is and change AXTextEditTypeFormat to AXTextEditTypeAttributesChanged. Having the mapping between different platform types be really obvious seems good.

[Joanmarie Diggs]

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #17)
> > > Since it's just white space, the right thing for now is probably just to not
> > > notify on replaces that occur during cuts.
> > 
> > Ok. In order to prevent patch collision like before, shall I do a new patch
> > with that change, or are you planning to?
> 
> I'll leave it to you.

Okie doke. Follow-up question.

If it's an AXTextEditTypeDelete, you call postTextReplacementNotification(). Why is AXTextEditTypeCut treated differently?

And for that matter, AXTextEditTypeTyping also results in a notification, but AXTextEditTypeDictation doesn't?

> I was thinking leave AXTextAttributesChanged as is and change
> AXTextEditTypeFormat to AXTextEditTypeAttributesChanged. Having the mapping
> between different platform types be really obvious seems good.

Aha. I'm good with that. Lemme know about the other question and then new patch coming up! :)

[Doug Russell]

(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > (In reply to comment #17)
> > > > Since it's just white space, the right thing for now is probably just to not
> > > > notify on replaces that occur during cuts.
> > > 
> > > Ok. In order to prevent patch collision like before, shall I do a new patch
> > > with that change, or are you planning to?
> > 
> > I'll leave it to you.
> 
> Okie doke. Follow-up question.
> 
> If it's an AXTextEditTypeDelete, you call postTextReplacementNotification().
> Why is AXTextEditTypeCut treated differently?

I wasn't aware of a case where a cut resulted in anything but text being removed and that's handled by the DeleteSelectionCommand posting a notification so a replace here would be a double post. With this case of inserting white space that I'm newly aware of I just made a judgement call that it's probably not worth a notification, but I'm open to changing that.

> 
> And for that matter, AXTextEditTypeTyping also results in a notification,
> but AXTextEditTypeDictation doesn't?

That should probably go in the group with insert.

> 
> > I was thinking leave AXTextAttributesChanged as is and change
> > AXTextEditTypeFormat to AXTextEditTypeAttributesChanged. Having the mapping
> > between different platform types be really obvious seems good.
> 
> Aha. I'm good with that. Lemme know about the other question and then new
> patch coming up! :)

[Joanmarie Diggs]

Created attachment 252121 [details]
Patch

[Joanmarie Diggs]

(In reply to comment #22)

> I wasn't aware of a case where a cut resulted in anything but text being
> removed and that's handled by the DeleteSelectionCommand posting a
> notification so a replace here would be a double post. With this case of
> inserting white space that I'm newly aware of I just made a judgement call
> that it's probably not worth a notification, but I'm open to changing that.

Nah. It's code I'm not familiar with and wasn't thinking about DeleteSelectionCommand posting the notification. So never mind. :) Thank you for the explanations! The new patch doesn't post a notification for cut.

> > And for that matter, AXTextEditTypeTyping also results in a notification,
> > but AXTextEditTypeDictation doesn't?
> 
> That should probably go in the group with insert.

The new patch does this. Also made the change to the type Format->AttributesChange.

I now have 0 crashing editing tests! Woo hoo.

@chris: Please review. Thanks!

[Doug Russell]

Looks good to me. Thanks!

[Joanmarie Diggs]

Since I see Mario is working today.... ;) r?

[chris fleizach]

Comment on attachment 252121 [details]
Patch

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

> Source/WebCore/editing/EditCommand.cpp:163
>      default:

is it possible to return default? have we covered every editing action?

> Source/WebCore/editing/InsertIntoTextNodeCommand.cpp:69
> +    if (AXObjectCache::accessibilityEnabled() && editingAction() != EditActionUnspecified)

is there something we can put into the root TextCommand class that says

shouldPostAccessibilityNotification() {
return AXObjectCache::accessibilityEnabled() && editingAction() != EditActionUnspecified
}

and then call that instead in all these cases?

[Joanmarie Diggs]

(In reply to comment #27)
> Comment on attachment 252121 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=252121&action=review
> 
> > Source/WebCore/editing/EditCommand.cpp:163
> >      default:
> 
> is it possible to return default? have we covered every editing action?

We've not covered every editing action. What is crashing the bulk of our editing tests is that we were not covering hardly any of them and triggering the assert associated with AXTextEditTypeUnknown. What I've done is cover everything that seems to be causing our debug bots to give up due to too many crashes -- something that's getting in the way of my colleagues, unfortunately.

Having covered those cases, I asked Doug in bug 142719 comment 142 to tackle the remainder.

> > Source/WebCore/editing/InsertIntoTextNodeCommand.cpp:69
> > +    if (AXObjectCache::accessibilityEnabled() && editingAction() != EditActionUnspecified)
> 
> is there something we can put into the root TextCommand class that says
> 
> shouldPostAccessibilityNotification() {
> return AXObjectCache::accessibilityEnabled() && editingAction() !=
> EditActionUnspecified
> }
> 
> and then call that instead in all these cases?

That makes sense. Will do. Thanks!

[Joanmarie Diggs]

Created attachment 252158 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 252158 [details]
Patch

Clearing flags on attachment: 252158

Committed r183683: <http://trac.webkit.org/changeset/183683>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.