Bug 157406 - AX: <attachment> element should have a replacement character
Summary: AX: <attachment> element should have a replacement character
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-05-05 19:18 PDT by Nan Wang
Modified: 2016-05-06 00:44 PDT (History)
4 users (show)

See Also:


Attachments
patch (1.47 KB, patch)
2016-05-05 19:27 PDT, Nan Wang
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-yosemite (1.02 MB, application/zip)
2016-05-05 19:52 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (935.81 KB, application/zip)
2016-05-05 20:15 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-yosemite (1.38 MB, application/zip)
2016-05-05 20:23 PDT, Build Bot
no flags Details
patch (5.58 KB, patch)
2016-05-06 00:08 PDT, Nan Wang
cfleizach: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nan Wang 2016-05-05 19:18:06 PDT
When getting string from text marker range, we should make sure <attachment> element gets a replacement character.

<rdar://problem/25297609>
Comment 1 Radar WebKit Bug Importer 2016-05-05 19:18:47 PDT
<rdar://problem/26132136>
Comment 2 Nan Wang 2016-05-05 19:27:08 PDT
Created attachment 278221 [details]
patch

<attachment> is being ignored in Safari so not sure how to write a test to verify the replacement character. 
Did test with Mac Mail and it was working.
Comment 3 Build Bot 2016-05-05 19:52:23 PDT
Comment on attachment 278221 [details]
patch

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

New failing tests:
editing/pasteboard/drag-and-drop-attachment-contenteditable.html
accessibility/attachment-element.html
Comment 4 Build Bot 2016-05-05 19:52:26 PDT
Created attachment 278223 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 5 Build Bot 2016-05-05 20:15:10 PDT
Comment on attachment 278221 [details]
patch

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

New failing tests:
accessibility/attachment-element.html
Comment 6 Build Bot 2016-05-05 20:15:13 PDT
Created attachment 278226 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 7 Nan Wang 2016-05-05 20:16:54 PDT
Seems this is affecting other stuff. I'll try to make the changes in accessibility code.
Comment 8 Build Bot 2016-05-05 20:23:42 PDT
Comment on attachment 278221 [details]
patch

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

New failing tests:
editing/pasteboard/drag-and-drop-attachment-contenteditable.html
accessibility/attachment-element.html
Comment 9 Build Bot 2016-05-05 20:23:46 PDT
Created attachment 278227 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 10 chris fleizach 2016-05-05 23:22:46 PDT
Comment on attachment 278221 [details]
patch

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

> Source/WebCore/ChangeLog:9
> +

in the attachment layout test in accessibility you'll see a way to enable attachment element through an internals call
Comment 11 Nan Wang 2016-05-06 00:08:39 PDT
Created attachment 278241 [details]
patch

Fixed up build and added a layout test.
Comment 12 chris fleizach 2016-05-06 00:15:19 PDT
Comment on attachment 278241 [details]
patch

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

> Source/WebCore/editing/TextIterator.cpp:258
> +    if (renderer->isImage() || renderer->isWidget() || renderer->isMedia() || renderer->isAttachment())

I would also do this like
bool isAttachment = false;
#if ENABLE()
isAttachment = renderer->...
#endif

if (renderer... 

so that we don't have to duplicate the logic checks
Comment 13 Nan Wang 2016-05-06 00:29:48 PDT
Comment on attachment 278241 [details]
patch

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

>> Source/WebCore/editing/TextIterator.cpp:258
>> +    if (renderer->isImage() || renderer->isWidget() || renderer->isMedia() || renderer->isAttachment())
> 
> I would also do this like
> bool isAttachment = false;
> #if ENABLE()
> isAttachment = renderer->...
> #endif
> 
> if (renderer... 
> 
> so that we don't have to duplicate the logic checks

Ok, will make the change before committing.
Comment 14 Nan Wang 2016-05-06 00:44:23 PDT
Committed r200509: <http://trac.webkit.org/changeset/200509>