Add an optional entry transition (from selection highlight) to TextIndicator
Created attachment 241840 [details] Patch
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.
<rdar://problem/18840128>
Need to turn on the transition for the DDChangedUI case, too.
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.
Created attachment 241896 [details] Patch
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.
http://trac.webkit.org/changeset/176363
Build fix http://trac.webkit.org/changeset/176370
Build fix http://trac.webkit.org/changeset/176374
EFL buildfix - https://trac.webkit.org/changeset/176375
GTK buildfix - https://trac.webkit.org/changeset/176385