Bug 164501 – WebHTMLView's _attributeStringFromDOMRange should use HTMLConverter instead of NSAttributedString's _initWithDOMRange

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

[Ryosuke Niwa]

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.

[Radar WebKit Bug Importer]

<rdar://problem/29152282>

[Ryosuke Niwa]

Created attachment 294144 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Darin Adler]

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 ]

[Darin Adler]

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?

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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.

[Alexey Proskuryakov]

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.

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Created attachment 294323 [details]
Addressed review comments

[Ryosuke Niwa]

Resubmitting this patch for a review since there are enough changes in the code and change log descriptions.

[Alexey Proskuryakov]

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

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Created attachment 294328 [details]
Updated for ToT

[Sam Weinig]

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

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Committed r208527: <http://trac.webkit.org/changeset/208527>