Bug 138856 – Add an optional entry transition (from selection highlight) to TextIndicator

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

Add an optional entry transition (from selection highlight) to TextIndicator

[Tim Horton]

Created attachment 241840 [details]
Patch

[WebKit Commit Bot]

Attachment 241840 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/mac/TextIndicatorWindow.mm:300:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

<rdar://problem/18840128>

[Tim Horton]

Need to turn on the transition for the DDChangedUI case, too.

[Tim Horton]

Created attachment 241852 [details]
Patch

Comment on attachment 241852 [details]
Patch

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

> Source/WebKit2/Shared/TextIndicator.cpp:120
> +// FIXME: Ideally the FrameSnapshotting functions would be more flexible and we
> +// wouldn't have to implement this here.

Can you please file a bug about doing this?

> Source/WebKit2/Shared/TextIndicator.h:71
> +    static PassRefPtr<TextIndicator> createWithSelectionInFrame(const WebFrame&, bool transitionFromSelectionColor = false);
> +    static PassRefPtr<TextIndicator> createWithRange(const WebCore::Range&, bool transitionFromSelectionColor = false);

I don’t think transitionFromSelectionColor best captures what this does. The transition is not from a color but rather from a snapshot of the selection (which typically includes the selection highlight, the foreground color of the selection, and any replaced elements). Declaring this as a boolean is also bad, which is evident from call sites below that pass "true". A TextIndicator::Transition parameter with values like FadeIn and Dissolve or whatever.

Comment on attachment 241852 [details]
Patch

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

>> Source/WebKit2/Shared/TextIndicator.h:71
>> +    static PassRefPtr<TextIndicator> createWithRange(const WebCore::Range&, bool transitionFromSelectionColor = false);
> 
> I don’t think transitionFromSelectionColor best captures what this does. The transition is not from a color but rather from a snapshot of the selection (which typically includes the selection highlight, the foreground color of the selection, and any replaced elements). Declaring this as a boolean is also bad, which is evident from call sites below that pass "true". A TextIndicator::Transition parameter with values like FadeIn and Dissolve or whatever.

…might be better.

Comment on attachment 241852 [details]
Patch

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

> Source/WebKit2/UIProcess/mac/TextIndicatorWindow.mm:40
> +const double bounceAnimationDuration = 0.12;
> +const double bounceWithTransitionAnimationDuration = 0.25;
> +const double timeBeforeFadeStarts = bounceAnimationDuration + 0.2;
> +const double fadeOutAnimationDuration = 0.3;

Why aren’t these CFTimeInterval?

> Source/WebKit2/UIProcess/mac/TextIndicatorWindow.mm:69
> +    bool _transitionFromSelectionColor;

Not BOOL? The name seems wrong for the reasons I stated above.

> Source/WebKit2/UIProcess/mac/TextIndicatorWindow.mm:72
> +- (id)_initWithFrame:(NSRect)frame textIndicator:(PassRefPtr<WebKit::TextIndicator>)textIndicator margin:(CGSize)margin;

Why the underscore?

> Source/WebKit2/UIProcess/mac/TextIndicatorWindow.mm:75
> +- (void)bounceWithCompletionHandler:(void(^)(void))completionHandler;
> +- (void)fadeOutWithCompletionHandler:(void(^)(void))completionHandler;

Now that multiple appearance transitions are supported, perhaps these names should be more generic (-appearWithCompletionHandler:, -disappearWithCompletionHandler: or similar).

Comment on attachment 241852 [details]
Patch

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

> Source/WebKit2/UIProcess/mac/TextIndicatorWindow.mm:83
> -    if ((self = [super initWithFrame:NSZeroRect]))
> +    if ((self = [super initWithFrame:frame])) {

Please make this an early return on the reverse condition.

> Source/WebKit2/UIProcess/mac/TextIndicatorWindow.mm:86
> +        _transitionFromSelectionColor = _textIndicator->data().contentImageWithHighlight;

When this is a BOOL, remember to !!

> Source/WebKit2/UIProcess/mac/TextIndicatorWindow.mm:122
> +            // FIXME: Ideally we wouldn't remove the margin in this case, but we need to to ensure
> +            // that the yellow highlight and contentImageWithHighlight overlap precisely.

Can you file a bug about fixing this?

> Source/WebKit2/UIProcess/mac/TextIndicatorWindow.mm:138
> +            [bounceLayer setValue:dropShadowLayer.get() forKey:@"dropShadowLayer"];

Better to define an NSString * const for this

> Source/WebKit2/UIProcess/mac/TextIndicatorWindow.mm:152
> +            [bounceLayer setValue:rimShadowLayer.get() forKey:@"rimShadowLayer"];

…and this

> Source/WebKit2/UIProcess/mac/TextIndicatorWindow.mm:174
> +            [bounceLayer setValue:textLayer.get() forKey:@"textLayer"];

…and this!

> Source/WebKit2/UIProcess/mac/TextIndicatorWindow.mm:190
> +        [NSValue valueWithCATransform3D:CATransform3DMakeScale(1, 1, 1)],

CATransform3DIdentity

> Source/WebKit2/UIProcess/mac/TextIndicatorWindow.mm:192
> +        [NSValue valueWithCATransform3D:CATransform3DMakeScale(1, 1, 1)]

CATransform3DIdentity

> Source/WebKit2/UIProcess/mac/TextIndicatorWindow.mm:201
> +        [transitionAnimation setToValue:(id)contentsImage.get()];

.toVaue =

> Source/WebKit2/UIProcess/mac/TextIndicatorWindow.mm:204
> +        [transitionAnimation setFillMode:kCAFillModeForwards];
> +        [transitionAnimation setRemovedOnCompletion:NO];
> +        [transitionAnimation setDuration:animationDuration];

. . .

> Source/WebKit2/UIProcess/mac/TextIndicatorWindow.mm:211
> +        [fadeShadowInAnimation setFromValue:@0];
> +        [fadeShadowInAnimation setToValue:@1];
> +        [fadeShadowInAnimation setFillMode:kCAFillModeForwards];
> +        [fadeShadowInAnimation setRemovedOnCompletion:NO];
> +        [fadeShadowInAnimation setDuration:animationDuration];

. . . . .

Comment on attachment 241852 [details]
Patch

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

> Source/WebKit2/UIProcess/mac/TextIndicatorWindow.mm:81
> +- (id)_initWithFrame:(NSRect)frame textIndicator:(PassRefPtr<WebKit::TextIndicator>)textIndicator margin:(CGSize)margin

Weird mix of NS and CG geometry types here.

[Tim Horton]

Created attachment 241896 [details]
Patch

[WebKit Commit Bot]

Attachment 241896 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/mac/TextIndicatorWindow.h:51:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/mac/WKViewInternal.h:84:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/mac/WKView.mm:3072:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/mac/TextIndicatorWindow.mm:270:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 4 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

http://trac.webkit.org/changeset/176363

[Tim Horton]

Build fix http://trac.webkit.org/changeset/176370

[Tim Horton]

Build fix http://trac.webkit.org/changeset/176374

[Csaba Osztrogonác]

EFL buildfix - https://trac.webkit.org/changeset/176375

[Csaba Osztrogonác]

GTK buildfix - https://trac.webkit.org/changeset/176385