Bug 119806 – [Mac] Add a way to easily test attributed string generation

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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.

[Darin Adler]

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 \\).

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Created attachment 208782 [details]
basic test

Here's a sample test.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 208785 [details]
Work in progress 2

[Ryosuke Niwa]

Created attachment 208786 [details]
Add the missing test

[Build Bot]

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

[Build Bot]

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

[kov's GTK+ EWS bot]

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

[kov's GTK+ EWS bot]

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

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Created attachment 208794 [details]
Work in progress 3

[Ryosuke Niwa]

Created attachment 208795 [details]
Work in progress 3

[Build Bot]

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

[Build Bot]

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

[Alexey Proskuryakov]

Can you use textInputController.attributedSubstringFromRange?

[Ryosuke Niwa]

(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 @"";
}

[Darin Adler]

(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?

[Ryosuke Niwa]

Oh I guess we have attributedSubstringFrom but that doesn't support enumerating all ranges & attributes unfortunately.

[Ryosuke Niwa]

(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.

[Alexey Proskuryakov]

> 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.

[Alexey Proskuryakov]

Also, its toString implementation logs all relevant information, see LayoutTests/editing/input/attributed-substring-from-range-lines-expected.txt

[Ryosuke Niwa]

(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.

[Darin Adler]

(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.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

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.

[Alexey Proskuryakov]

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.

[Ryosuke Niwa]

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.

[Build Bot]

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

[Build Bot]

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

[Darin Adler]

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.

[Alexey Proskuryakov]

> 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?

[Ryosuke Niwa]

(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.

[Alexey Proskuryakov]

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).

[Ryosuke Niwa]

(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.

[Build Bot]

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

[Build Bot]

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

[Ryosuke Niwa]

Created attachment 208957 [details]
Patch

[Darin Adler]

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?

[Darin Adler]

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.".

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 209925 [details]
Addressed Darin's comments

[Darin Adler]

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.

[Ryosuke Niwa]

Committed r154803: <http://trac.webkit.org/changeset/154803>