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.
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.
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 \\).
(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.
Created attachment 208782 [details] basic test Here's a sample test.
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.
Created attachment 208785 [details] Work in progress 2
Created attachment 208786 [details] Add the missing test
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 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 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 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
(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.
Created attachment 208794 [details] Work in progress 3
Created attachment 208795 [details] Work in progress 3
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 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
Can you use textInputController.attributedSubstringFromRange?
(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 @""; }
(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?
Oh I guess we have attributedSubstringFrom but that doesn't support enumerating all ranges & attributes unfortunately.
(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.
> 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.
Also, its toString implementation logs all relevant information, see LayoutTests/editing/input/attributed-substring-from-range-lines-expected.txt
(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.
(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.
(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.
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.
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.
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 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
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 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.
> 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?
(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.
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).
(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 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
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
Created attachment 208957 [details] Patch
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 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 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.
Created attachment 209925 [details] Addressed Darin's comments
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.
Committed r154803: <http://trac.webkit.org/changeset/154803>