HTMLWebView's _attributeStringFromDOMRange is still using NSAttributedString's _initWithDOMRange to create the attributed string. We should use HTMLConverter's attributedStringFromRange instead since that's what we're using for copy & paste and everywhere else in WebKit.
<rdar://problem/29152282>
Created attachment 294144 [details] Patch
Comment on attachment 294144 [details] Patch Attachment 294144 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2477299 New failing tests: editing/mac/attributed-string/font-size.html editing/mac/attributed-string/font-weight.html editing/mac/attributed-string/letter-spacing.html editing/mac/attributed-string/font-style-variant-effect.html editing/mac/attributed-string/vertical-align.html editing/mac/attributed-string/text-decorations.html editing/mac/attributed-string/anchor-element.html editing/mac/attributed-string/basic.html
Created attachment 294151 [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 on attachment 294144 [details] Patch Tests failing on the EWS bot named "mac": Regressions: Unexpected text-only failures (9) editing/mac/attributed-string/anchor-element.html [ Failure ] editing/mac/attributed-string/basic.html [ Failure ] editing/mac/attributed-string/font-size.html [ Failure ] editing/mac/attributed-string/font-style-variant-effect.html [ Failure ] editing/mac/attributed-string/font-weight.html [ Failure ] editing/mac/attributed-string/letter-spacing.html [ Failure ] editing/mac/attributed-string/text-decorations.html [ Failure ] editing/mac/attributed-string/vertical-align.html [ Failure ]
Comment on attachment 294144 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=294144&action=review r=me but please find out why this is failing on the "mac" EWS bot > Source/WebKit/mac/ChangeLog:9 > + Use HTMLConverter to extract the attributed strings in WebHTMLView _attributeStringFromDOMRange. What a strange name this method has. "attribute string" is not a thing! > Source/WebKit/mac/WebView/WebHTMLView.mm:-7423 > - attributedString = [[[NSAttributedString alloc] _initWithDOMRange:range] autorelease]; Please also remove the declaration of _initWithDOMRange at the top of the file. > LayoutTests/ChangeLog:12 > + I've manually confirmed that these results didn't change across the large refactoring we did. This is a confusing comment. I think that what you mean is this: 1) We thought these tests were testing HTMLConverter. 2) We did a large refactoring project on HTMLConverter, thinking these tests would detect any mistakes. 3) We got lucky and didn’t make any mistakes that affected the test output; you have now verified that. But the comment didn’t make that clear. I had to think hard about it. > LayoutTests/editing/mac/attributed-string/anchor-element-expected.txt:13 > - LineHeight: 18/0 > + LineHeight: 0/0 Could you comment on changes like this one, explaining why they are desirable? Why is LineHeight 0 better than 18 for this? I assume it is, but I would like you to state why explicitly for posterity. > LayoutTests/editing/mac/attributed-string/anchor-element-expected.txt:-35 > - NSColor: #000000 (NSDeviceRGBColorSpace) Why is no color at all better than explicit black for this? Same reason as above. > LayoutTests/editing/mac/attributed-string/font-size-expected.txt:-61 > -NSParagraphStyle: > -Alignment 4 > - LineSpacing: 0 > - ParagraphSpacing: 0 > - ParagraphSpacingBefore: 0 > - HeadIndent: 0 > - TailIndent: 0 > - FirstLineHeadIndent: 0 > - LineHeight: 18/0 > - LineHeightMultiple: 0 > - LineBreakMode: 0 > - Tabs: () > - DefaultTabInterval: 36 > - Blocks: ( > -) > - Lists: ( > -) > - BaseWritingDirection: 0 > - HyphenationFactor: 0 > - TighteningForTruncation: YES > - HeaderLevel: 0 Why is all this being omitted better than including it?
Comment on attachment 294144 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=294144&action=review >> Source/WebKit/mac/ChangeLog:9 >> + Use HTMLConverter to extract the attributed strings in WebHTMLView _attributeStringFromDOMRange. > > What a strange name this method has. "attribute string" is not a thing! Renamed it to _attributedStringFromDOMRange. >> Source/WebKit/mac/WebView/WebHTMLView.mm:-7423 >> - attributedString = [[[NSAttributedString alloc] _initWithDOMRange:range] autorelease]; > > Please also remove the declaration of _initWithDOMRange at the top of the file. Done. >> LayoutTests/ChangeLog:12 >> + I've manually confirmed that these results didn't change across the large refactoring we did. > > This is a confusing comment. I think that what you mean is this: > > 1) We thought these tests were testing HTMLConverter. > 2) We did a large refactoring project on HTMLConverter, thinking these tests would detect any mistakes. > 3) We got lucky and didn’t make any mistakes that affected the test output; you have now verified that. > > But the comment didn’t make that clear. I had to think hard about it. Fixed. >> LayoutTests/editing/mac/attributed-string/anchor-element-expected.txt:13 >> + LineHeight: 0/0 > > Could you comment on changes like this one, explaining why they are desirable? Why is LineHeight 0 better than 18 for this? I assume it is, but I would like you to state why explicitly for posterity. Not really, I think specifying the correct line height is probably better. What this patch accomplishes is really about alining what WebHTMLView to return what we'd use for copy & paste, and what we're seeing in these diff is the difference between what AppKit implements and what we implement. An alternative approach for this patch is to add another testing facility for HTMLConverter and then align its behavior to what AppKit does, and then do this transition.
Let me rephrase it again. What this patch accomplishes is alining what [WebHTMLView attributedString] returns to what would be used on copy. The differences we're seeing stems from the existing behavior difference between AppKit's converter and HTMLConverter. An alternative approach would be to 1. Add another testing facility for HTMLConverter 2. Align its behavior to what AppKit does 3. Change [WebHTMLView attributedString] to use HTMLConverter I didn't feel that we have to go through this process because WebView and WKWebView already use HTMLConverter when the user tries to copy content but we can take the alternative route if anyone feels strongly about it since that would definitely be a safer approach.
I don't know of any principled way to decide - the metric of success will be whether input methods and/or Dictionary popup will break. LineHeight of 0 in particular seems wrong, as that won't let Dictionary render its highlight correctly.
(In reply to comment #9) > I don't know of any principled way to decide - the metric of success will be > whether input methods and/or Dictionary popup will break. > > LineHeight of 0 in particular seems wrong, as that won't let Dictionary > render its highlight correctly. I've tested both Dictionary lookup & IME on Safari & mini browser and both them seem to work.
Created attachment 294323 [details] Addressed review comments
Resubmitting this patch for a review since there are enough changes in the code and change log descriptions.
> I've tested both Dictionary lookup & IME on Safari & mini browser and both them seem to work. This is a WebKit1 change, so Safari wouldn't be affected. MiniBrowser has modes for both, of course (but then again, there are so many input method clients with different requirements that it's hard to fully test).
(In reply to comment #13) > > I've tested both Dictionary lookup & IME on Safari & mini browser and both them seem to work. > > This is a WebKit1 change, so Safari wouldn't be affected. MiniBrowser has > modes for both, of course (but then again, there are so many input method > clients with different requirements that it's hard to fully test). Obviously, I checked WK1 with mini browser.
Created attachment 294328 [details] Updated for ToT
Comment on attachment 294328 [details] Updated for ToT View in context: https://bugs.webkit.org/attachment.cgi?id=294328&action=review r=me > Source/WebKit/mac/ChangeLog:3 > + HTMLWebView's _attributeStringFromDOMRange should use HTMLConverter instead of NSAttributedString's _initWithDOMRange I think you mean WebHTMLView
Thanks for the review! (In reply to comment #16) > Comment on attachment 294328 [details] > Updated for ToT > > View in context: > https://bugs.webkit.org/attachment.cgi?id=294328&action=review > > r=me > > > Source/WebKit/mac/ChangeLog:3 > > + HTMLWebView's _attributeStringFromDOMRange should use HTMLConverter instead of NSAttributedString's _initWithDOMRange > > I think you mean WebHTMLView Oops, will fix before landing.
Committed r208527: <http://trac.webkit.org/changeset/208527>