Bug 119806 - [Mac] Add a way to easily test attributed string generation
Summary: [Mac] Add a way to easily test attributed string generation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-08-14 10:45 PDT by Ryosuke Niwa
Modified: 2013-08-28 20:02 PDT (History)
8 users (show)

See Also:


Attachments
First cut approach (9.21 KB, patch)
2013-08-14 10:49 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
basic test (722 bytes, text/html)
2013-08-14 19:28 PDT, Ryosuke Niwa
no flags Details
rtf of basic (561 bytes, text/rtf)
2013-08-14 19:35 PDT, Ryosuke Niwa
no flags Details
Work in progress 2 (20.51 KB, patch)
2013-08-14 19:37 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Add the missing test (22.79 KB, patch)
2013-08-14 19:42 PDT, Ryosuke Niwa
buildbot: commit-queue-
Details | Formatted Diff | Diff
Work in progress 3 (25.99 KB, patch)
2013-08-15 00:09 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Work in progress 3 (26.03 KB, patch)
2013-08-15 00:11 PDT, Ryosuke Niwa
buildbot: commit-queue-
Details | Formatted Diff | Diff
New approach (WIP4) (50.45 KB, patch)
2013-08-15 20:08 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (812.30 KB, application/zip)
2013-08-15 22:10 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (584.21 KB, application/zip)
2013-08-16 13:27 PDT, Build Bot
no flags Details
Patch (55.90 KB, patch)
2013-08-16 14:12 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Addressed Darin's comments (30.59 KB, patch)
2013-08-28 15:25 PDT, Ryosuke Niwa
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2013-08-14 10:45:01 PDT
We need a way to easily test HTMLConverter.mm so that we can rewrite it in C++.

Unfortunately, the current approach of hard-coding range and expected attributes in Objective-C as done in Tests/mac/AttributedString.mm wouldn't scale well for that purpose.
Comment 1 Ryosuke Niwa 2013-08-14 10:49:38 PDT
Created attachment 208744 [details]
First cut approach

I'm experimenting with an approach to serialize NSAttributedString as text. The following markup generates the output beneath it:

hello <b style="background-color:blue; color:white;">world</b> WebKit.<br>
this is a <u>test</u> of attributed string.

[  0,  6]:[hello ]
          color: rgba(0.00, 0.00, 0.00, 1.00)
          font: "Times Roman" 16.00pt
[  6,  5]:[world]
          background-color: rgba(0.00, 0.00, 1.00, 1.00)
          color: rgba(1.00, 1.00, 1.00, 1.00)
          font: "Times Bold" 16.00pt
[ 11, 19]:[ WebKit.
this is a ]
          color: rgba(0.00, 0.00, 0.00, 1.00)
          font: "Times Roman" 16.00pt
[ 30, 26]:[test of attributed string.]
          color: rgba(0.00, 0.00, 0.00, 1.00)
          font: "Times Roman" 16.00pt
          Unknown NSUnderline:1

I might need to tweak the syntax a little but I think this is quite human-readable.

My current plan is put this serializer into DRT/WTR (probably shared through WebCore) and add a method like testRunner.dumpAttributedString(DOMRange) so that we can write layout tests to test attribute string generators.
Comment 2 Darin Adler 2013-08-14 12:30:11 PDT
I think that some special cases will make this more readable.

Color names for common colors like black, blue, and white.

Also numeric formatting that omits all those ".00".

And escaping so that newlines are \n instead of actual newlines (backslashes can be \\).
Comment 3 Ryosuke Niwa 2013-08-14 17:37:10 PDT
(In reply to comment #2)
> Color names for common colors like black, blue, and white.
> 
> Also numeric formatting that omits all those ".00".

I'll use Color::serialized().

> And escaping so that newlines are \n instead of actual newlines (backslashes can be \\).

Good idea. Will do.
Comment 4 Ryosuke Niwa 2013-08-14 19:28:53 PDT
Created attachment 208782 [details]
basic test

Here's a sample test.
Comment 5 Ryosuke Niwa 2013-08-14 19:35:58 PDT
Created attachment 208783 [details]
rtf of basic

Apparently this feature is almost completely broken. Initially thought my test code was bad but
I've added a code to convert the generated attributed string back to RTF and this is what we see.
Comment 6 Ryosuke Niwa 2013-08-14 19:37:55 PDT
Created attachment 208785 [details]
Work in progress 2
Comment 7 Ryosuke Niwa 2013-08-14 19:42:43 PDT
Created attachment 208786 [details]
Add the missing test
Comment 8 Build Bot 2013-08-14 20:31:46 PDT
Comment on attachment 208786 [details]
Add the missing test

Attachment 208786 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1471034
Comment 9 Build Bot 2013-08-14 20:52:05 PDT
Comment on attachment 208786 [details]
Add the missing test

Attachment 208786 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1466154
Comment 10 kov's GTK+ EWS bot 2013-08-14 20:55:55 PDT
Comment on attachment 208786 [details]
Add the missing test

Attachment 208786 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1469066
Comment 11 kov's GTK+ EWS bot 2013-08-14 21:47:37 PDT
Comment on attachment 208786 [details]
Add the missing test

Attachment 208786 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1462343
Comment 12 Ryosuke Niwa 2013-08-15 00:06:28 PDT
(In reply to comment #5)
> Created an attachment (id=208783) [details]
> rtf of basic
> 
> Apparently this feature is almost completely broken. Initially thought my test code was bad but
> I've added a code to convert the generated attributed string back to RTF and this is what we see.

This was because I was calling WebHTMLConverter::editingAttributedStringFromRange, which is completely broken. WebHTMLConverter::attributedString yields a much better result.
Comment 13 Ryosuke Niwa 2013-08-15 00:09:45 PDT
Created attachment 208794 [details]
Work in progress 3
Comment 14 Ryosuke Niwa 2013-08-15 00:11:26 PDT
Created attachment 208795 [details]
Work in progress 3
Comment 15 Build Bot 2013-08-15 00:32:15 PDT
Comment on attachment 208795 [details]
Work in progress 3

Attachment 208795 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1476055
Comment 16 Build Bot 2013-08-15 01:38:51 PDT
Comment on attachment 208795 [details]
Work in progress 3

Attachment 208795 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1458982
Comment 17 Alexey Proskuryakov 2013-08-15 10:41:11 PDT
Can you use textInputController.attributedSubstringFromRange?
Comment 18 Ryosuke Niwa 2013-08-15 12:12:18 PDT
(In reply to comment #17)
> Can you use textInputController.attributedSubstringFromRange?

Doesn't that just return plain text?

- (NSString *)substringFrom:(int)from length:(int)length
{
    NSObject <NSTextInput> *textInput = [self textInput];

    if (textInput)
        return [[textInput attributedSubstringFromRange:NSMakeRange(from, length)] string];
    
    return @"";
}
Comment 19 Darin Adler 2013-08-15 12:14:33 PDT
(In reply to comment #18)
>         return [[textInput attributedSubstringFromRange:NSMakeRange(from, length)] string];

The call to the -[NSAttributedString string] method here is what converts the attributed string into plain text. Does that answer your question?
Comment 20 Ryosuke Niwa 2013-08-15 12:14:53 PDT
Oh I guess we have attributedSubstringFrom but that doesn't support enumerating all ranges & attributes unfortunately.
Comment 21 Ryosuke Niwa 2013-08-15 12:16:37 PDT
(In reply to comment #19)
> (In reply to comment #18)
> >         return [[textInput attributedSubstringFromRange:NSMakeRange(from, length)] string];
> 
> The call to the -[NSAttributedString string] method here is what converts the attributed string into plain text. Does that answer your question?

I still don't think textInputController provides an adaquate API for this purpose.
Comment 22 Alexey Proskuryakov 2013-08-15 12:26:13 PDT
> Oh I guess we have attributedSubstringFrom but that doesn't support enumerating all ranges & attributes unfortunately.

It does. It returns an object that is bridged to a JavaScript one with a number of accessors, search for "valueOfAttribute:atIndex:" in TextInputController.m.
Comment 23 Alexey Proskuryakov 2013-08-15 12:29:54 PDT
Also, its toString implementation logs all relevant information, see LayoutTests/editing/input/attributed-substring-from-range-lines-expected.txt
Comment 24 Ryosuke Niwa 2013-08-15 12:32:30 PDT
(In reply to comment #23)
> Also, its toString implementation logs all relevant information, see LayoutTests/editing/input/attributed-substring-from-range-lines-expected.txt

Hm... I guess I can call this function at each index and record whenever it changes?  I guess that's doable.

I don't think the current format of attributed-substring-from-range-lines-expected.txt is human readable. There's just too much noise in there.
Comment 25 Darin Adler 2013-08-15 12:34:05 PDT
(In reply to comment #24)
> I don't think the current format of attributed-substring-from-range-lines-expected.txt is human readable. There's just too much noise in there.

Maybe the right thing to do is to fix that format instead of adding something new.
Comment 26 Ryosuke Niwa 2013-08-15 12:42:11 PDT
(In reply to comment #25)
> (In reply to comment #24)
> > I don't think the current format of attributed-substring-from-range-lines-expected.txt is human readable. There's just too much noise in there.
> 
> Maybe the right thing to do is to fix that format instead of adding something new.

But the existing API appears to be broken because of bug 5610 and others. There is even a bunch of comments indicating bugs in LayoutTests/editing/input/attributed-substring-from-range-lines.html-disabled.

My preference is to add a new API that directly takes Range and get rid of textInputController.attributedSubstringFromRange.  We can separately test convertNSRangeToDOMRange but I don't want this work be blocked by fixing that API.
Comment 27 Ryosuke Niwa 2013-08-15 14:05:54 PDT
I've looked into modifying textInputController's method but it goes through NSTextInput, and it's going to be hard to modify it to take DOMRange instead.
Comment 28 Alexey Proskuryakov 2013-08-15 15:27:30 PDT
I think that most or all of the tests can just do textInputController.attributedSubstringFromRange(0, 100000), and then it doesn't matter what range is passed in. It looks like the accessors we currently expose on NSMutableAttributedString bridged object may be insufficient, but that set can be easily expanded.
Comment 29 Ryosuke Niwa 2013-08-15 20:08:22 PDT
Created attachment 208877 [details]
New approach (WIP4)

Here's new approach to use textInputController. It's nice that this significantly offloads the reformatting work to javascript but I still can't use the existing attributedSubstringFromRange because that uses broken WebHTMLConverter::editingAttributedStringFromRange.  So I had to add a new method legacyAttributedFromRange that calls WebHTMLView::_attributeStringFromDOMRange.
Comment 30 Build Bot 2013-08-15 22:10:23 PDT
Comment on attachment 208877 [details]
New approach (WIP4)

Attachment 208877 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1475388

New failing tests:
platform/mac/editing/attributed-string/font-style-variant-effect.html
platform/mac/editing/attributed-string/basic.html
platform/mac/editing/attributed-string/letter-spacing.html
platform/mac/editing/attributed-string/vertical-align.html
platform/mac/editing/attributed-string/anchor-element.html
platform/mac/editing/attributed-string/text-decorations.html
platform/mac/editing/attributed-string/font-size.html
platform/mac/editing/attributed-string/font-weight.html
Comment 31 Build Bot 2013-08-15 22:10:27 PDT
Created attachment 208881 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.4
Comment 32 Darin Adler 2013-08-15 23:25:41 PDT
Comment on attachment 208877 [details]
New approach (WIP4)

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

> Tools/DumpRenderTree/mac/TextInputController.m:79
> ++ (BOOL)isSelectorExcludedFromWebScript:(SEL)aSelector;
> ++ (NSString *)webScriptNameForSelector:(SEL)aSelector;

Do we really have to do this old school WebScriptObject-style binding? Can’t we use the modern Objective-C binding instead?

> Tools/DumpRenderTree/mac/TextInputController.m:114
> +    if (aSelector == @selector(location))
> +        return @"location";
> +    if (aSelector == @selector(length))
> +        return @"length";

Is this really needed? I thought the default was to use the selector name?

> Tools/DumpRenderTree/mac/TextInputController.m:117
> +}
> +@end

Should have a blank line here.

> Tools/DumpRenderTree/mac/TextInputController.m:169
> +- (NSArray *)getRanges

Why include “get” in this method name? Lets just call it “ranges”.

> Tools/DumpRenderTree/mac/TextInputController.m:259
> +        return @"legacyAttributedFromRange";

“attributed” is a noun, so this is not a great function name. Also, what does “legacy” mean in this function name? I think that our JavaScriptNames should not have Objective-C style “FromRange” in them unless we think others it’s too unclear what their argument types are.

> Tools/DumpRenderTree/mac/TextInputController.m:373
> +    NSMutableAttributedString *ret = [[[NSMutableAttributedString alloc] init] autorelease];

How about “string” or “result” rather than “ret”?

> Tools/DumpRenderTree/mac/TextInputController.m:376
> +    if ([documentView isKindOfClass:[WebHTMLView class]])
> +        [ret setAttributedString:[(WebHTMLView *)documentView _attributeStringFromDOMRange:range]];

I like early return rather than nesting the work inside an if statement.
Comment 33 Alexey Proskuryakov 2013-08-16 09:36:43 PDT
> I still can't use the existing attributedSubstringFromRange because that uses broken WebHTMLConverter::editingAttributedStringFromRange.  So I had to add a new method legacyAttributedFromRange that calls WebHTMLView::_attributeStringFromDOMRange.

Does the existing textInputController method use the same code path as input methods do?
Comment 34 Ryosuke Niwa 2013-08-16 09:48:32 PDT
(In reply to comment #33)
> > I still can't use the existing attributedSubstringFromRange because that uses broken WebHTMLConverter::editingAttributedStringFromRange.  So I had to add a new method legacyAttributedFromRange that calls WebHTMLView::_attributeStringFromDOMRange.
> 
> Does the existing textInputController method use the same code path as input methods do?

Yes, but that's not what I'm trying to test here. What I want to test is the version that's used in copy & paste.
Comment 35 Alexey Proskuryakov 2013-08-16 10:01:31 PDT
It's sad that we have multiple code paths. We need to make it very clear in the testing API that they are different (possibly exposing the new version on something other than textInputController, as its purpose is to test input method APIs).
Comment 36 Ryosuke Niwa 2013-08-16 10:21:58 PDT
(In reply to comment #35)
> It's sad that we have multiple code paths. We need to make it very clear in the testing API that they are different (possibly exposing the new version on something other than textInputController, as its purpose is to test input method APIs).

Right. I think we always want to use TestIterator-based WebHTMLConverter::editingAttributedStringFromRange and get rid of the other one. That's why I added "legacy" prefix.
Comment 37 Build Bot 2013-08-16 13:27:05 PDT
Comment on attachment 208877 [details]
New approach (WIP4)

Attachment 208877 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1468671

New failing tests:
platform/mac/editing/attributed-string/font-style-variant-effect.html
platform/mac/editing/attributed-string/basic.html
platform/mac/editing/attributed-string/letter-spacing.html
platform/mac/editing/attributed-string/vertical-align.html
platform/mac/editing/attributed-string/anchor-element.html
platform/mac/editing/attributed-string/text-decorations.html
platform/mac/editing/attributed-string/font-size.html
platform/mac/editing/attributed-string/font-weight.html
Comment 38 Build Bot 2013-08-16 13:27:07 PDT
Created attachment 208950 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4
Comment 39 Ryosuke Niwa 2013-08-16 14:12:50 PDT
Created attachment 208957 [details]
Patch
Comment 40 Darin Adler 2013-08-17 04:59:53 PDT
Comment on attachment 208957 [details]
Patch

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

> Tools/DumpRenderTree/mac/TextInputController.m:57
> +- (NSAttributedString *)_attributeStringFromDOMRange:(DOMRange *)range;

Typo: Should be _attributedString, not _attributeString.

> Tools/DumpRenderTree/mac/TextInputController.m:78
> ++ (BOOL)isSelectorExcludedFromWebScript:(SEL)aSelector;

Should just be “selector”, not “aSelector”.

> Tools/DumpRenderTree/mac/TextInputController.m:93
> +- (unsigned)location

Could we code this so it works properly with 64-bit offsets, or is that impractical? Not important in practice, but a little sloppy, to chop the 64-bit value to 32-bit.

> Tools/DumpRenderTree/mac/TextInputController.m:103
> ++ (BOOL)isSelectorExcludedFromWebScript:(SEL)aSelector

Should just be “selector”, not “aSelector”.

> Tools/DumpRenderTree/mac/TextInputController.m:163
> +    [self enumerateAttributesInRange:NSMakeRange(0, [self length]) options:0 usingBlock:^(NSDictionary *attrs, NSRange range, BOOL *stop) {

Why not use the word “attributes” instead of the abbreviation “attrs”.

> Tools/DumpRenderTree/mac/TextInputController.m:164
> +        [array addObject:[[WebNSRange alloc] initWithNSRange:range]];

All these WebNSRange objects are leaking. There is no release to balance the alloc.

> Tools/DumpRenderTree/mac/TextInputController.m:364
> +    NSMutableAttributedString *ret = [[[NSMutableAttributedString alloc] init] autorelease];

Why not call this “string” instead of “ret”?

Also, why not use initWithAttributedString: instead of allocating the string first and then calling setAttributedString:? Then you would not need a local variable at all.

> Tools/DumpRenderTree/mac/TextInputController.m:369
> +    id documentView = [[[webView mainFrame] frameView] documentView];
> +    if ([documentView isKindOfClass:[WebHTMLView class]])
> +        [ret setAttributedString:[(WebHTMLView *)documentView _attributeStringFromDOMRange:range]];
> +
> +    return ret;

I like early return better than putting the main path of the code into an if statement.

> LayoutTests/ChangeLog:9
> +        * platform/mac-wk2/TestExpectations:

No explanation here of why we are skipping all the tests for WebKit2. Maybe obvious to others.

> LayoutTests/platform/mac/editing/attributed-string/text-decorations-expected.txt:28
> +    NSFont: Times-Roman 16.00 pt.
> +    NSParagraphStyle:
> +        Alignment=natural
> +        LineSpacing=0
> +        ParagraphSpacing=0
> +        ParagraphSpacingBefore=0
> +        HeadIndent=0
> +        TailIndent=0
> +        FirstLineHeadIndent=0
> +        LineHeight=0/0
> +        LineHeightMultiple=0
> +        LineBreakMode=0
> +        Tabs=()
> +        DefaultTabInterval=36
> +        Blocks=(null)
> +        Lists=(null)
> +        BaseWritingDirection=0
> +        HyphenationFactor=0
> +        TighteningFactor=0.05
> +        HeaderLevel=0
> +    NSUnderline: true

This output seems way too wordy. It’s going to be hard to spot bugs in dumps like this one. Can we do something to collapse all the normal NSParagraphStyle values? It’s not an arbitrary dictionary, so unlike the attributes dictionary there is no need to distinguish “paragraph spacing not specified” from “paragraph spacing is zero”.

Why no spaces around the "=" characters, but spaces after the ":" characters?

Do we really have to dump the point size out as "16.00 pt." instead of "16 pt."? Also, is the period really helpful?
Comment 41 Darin Adler 2013-08-17 05:08:28 PDT
Comment on attachment 208957 [details]
Patch

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

>> LayoutTests/platform/mac/editing/attributed-string/text-decorations-expected.txt:28
>> +    NSUnderline: true
> 
> This output seems way too wordy. It’s going to be hard to spot bugs in dumps like this one. Can we do something to collapse all the normal NSParagraphStyle values? It’s not an arbitrary dictionary, so unlike the attributes dictionary there is no need to distinguish “paragraph spacing not specified” from “paragraph spacing is zero”.
> 
> Why no spaces around the "=" characters, but spaces after the ":" characters?
> 
> Do we really have to dump the point size out as "16.00 pt." instead of "16 pt."? Also, is the period really helpful?

Why say “pt.” at all? I think that Times-Roman 16 is pretty clear without "pt.".
Comment 42 Ryosuke Niwa 2013-08-28 13:53:27 PDT
Comment on attachment 208957 [details]
Patch

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

>> Tools/DumpRenderTree/mac/TextInputController.m:57
>> +- (NSAttributedString *)_attributeStringFromDOMRange:(DOMRange *)range;
> 
> Typo: Should be _attributedString, not _attributeString.

This is the name of the member function exposed on WebHTMLView. I can file a separate bug to rename it if you're so inclined.
Comment 43 Ryosuke Niwa 2013-08-28 15:25:56 PDT
Created attachment 209925 [details]
Addressed Darin's comments
Comment 44 Darin Adler 2013-08-28 18:17:24 PDT
Comment on attachment 209925 [details]
Addressed Darin's comments

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

> Tools/DumpRenderTree/mac/TextInputController.m:78
> ++ (BOOL)isSelectorExcludedFromWebScript:(SEL)selector;

There’s no need to declare this. You can just define it.
Comment 45 Ryosuke Niwa 2013-08-28 20:02:57 PDT
Committed r154803: <http://trac.webkit.org/changeset/154803>