Bug 144447 - [GTK] REGRESSION(183368): It made editing tests assert
Summary: [GTK] REGRESSION(183368): It made editing tests assert
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joanmarie Diggs (irc: joanie)
URL:
Keywords:
Depends on:
Blocks: 142719
  Show dependency treegraph
 
Reported: 2015-04-30 04:40 PDT by Csaba Osztrogonác
Modified: 2015-05-01 12:39 PDT (History)
8 users (show)

See Also:


Attachments
style (3.81 KB, patch)
2015-04-30 14:31 PDT, Doug Russell
no flags Details | Formatted Diff | Diff
Patch (12.51 KB, patch)
2015-04-30 15:56 PDT, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
Patch (13.54 KB, patch)
2015-04-30 17:37 PDT, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
Patch (15.34 KB, patch)
2015-05-01 11:01 PDT, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2015-04-30 04:40:40 PDT
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&)
Comment 1 Doug Russell 2015-04-30 11:21:47 PDT
Do you have the rest of the stack trace for the assert?
Comment 2 Martin Robinson 2015-04-30 11:24:09 PDT
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]
Comment 3 Doug Russell 2015-04-30 11:35:23 PDT
Any idea what the editingAction is for FormatBlockCommand? Doesn't look like it's being set.
Comment 4 Doug Russell 2015-04-30 11:37:18 PDT
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.
Comment 5 Doug Russell 2015-04-30 11:58:32 PDT
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.
Comment 6 Joanmarie Diggs (irc: joanie) 2015-04-30 12:36:18 PDT
(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.
Comment 7 Joanmarie Diggs (irc: joanie) 2015-04-30 14:21:49 PDT
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.
Comment 8 Doug Russell 2015-04-30 14:27:26 PDT
(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.
Comment 9 Doug Russell 2015-04-30 14:31:22 PDT
Created attachment 252088 [details]
style

Here's a very naive pass at adding a style edit type
Comment 10 Joanmarie Diggs (irc: joanie) 2015-04-30 14:36:33 PDT
(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.
Comment 11 Joanmarie Diggs (irc: joanie) 2015-04-30 15:56:05 PDT
Created attachment 252098 [details]
Patch
Comment 12 Joanmarie Diggs (irc: joanie) 2015-04-30 16:02:32 PDT
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?
Comment 13 Doug Russell 2015-04-30 16:05:47 PDT
Looks good to me. I'll give it a test run locally.

Do you have traces for the crashing paste tests?
Comment 14 Joanmarie Diggs (irc: joanie) 2015-04-30 16:13:07 PDT
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]
Comment 15 Joanmarie Diggs (irc: joanie) 2015-04-30 16:16:44 PDT
(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.
Comment 16 Doug Russell 2015-04-30 16:21:15 PDT
(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. :)
Comment 17 Doug Russell 2015-04-30 16:22:36 PDT
Since it's just white space, the right thing for now is probably just to not notify on replaces that occur during cuts.
Comment 18 Doug Russell 2015-04-30 16:24:03 PDT
(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.
Comment 19 Joanmarie Diggs (irc: joanie) 2015-04-30 16:39:20 PDT
(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.
Comment 20 Doug Russell 2015-04-30 16:42:54 PDT
(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.
Comment 21 Joanmarie Diggs (irc: joanie) 2015-04-30 16:50:05 PDT
(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! :)
Comment 22 Doug Russell 2015-04-30 16:57:20 PDT
(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! :)
Comment 23 Joanmarie Diggs (irc: joanie) 2015-04-30 17:37:53 PDT
Created attachment 252121 [details]
Patch
Comment 24 Joanmarie Diggs (irc: joanie) 2015-04-30 17:47:08 PDT
(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!
Comment 25 Doug Russell 2015-04-30 17:56:14 PDT
Looks good to me. Thanks!
Comment 26 Joanmarie Diggs (irc: joanie) 2015-05-01 06:32:10 PDT
Since I see Mario is working today.... ;) r?
Comment 27 chris fleizach 2015-05-01 09:56:25 PDT
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?
Comment 28 Joanmarie Diggs (irc: joanie) 2015-05-01 10:11:05 PDT
(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!
Comment 29 Joanmarie Diggs (irc: joanie) 2015-05-01 11:01:23 PDT
Created attachment 252158 [details]
Patch
Comment 30 WebKit Commit Bot 2015-05-01 12:39:12 PDT
Comment on attachment 252158 [details]
Patch

Clearing flags on attachment: 252158

Committed r183683: <http://trac.webkit.org/changeset/183683>
Comment 31 WebKit Commit Bot 2015-05-01 12:39:17 PDT
All reviewed patches have been landed.  Closing bug.