Bug 165122 - [iOS] Tapping on an HTML validation bubble should dismiss it
Summary: [iOS] Tapping on an HTML validation bubble should dismiss it
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 164382
  Show dependency tree / graph
 
Reported: 2016-11-28 16:07 PST by Chris Dumez
Modified: 2016-12-02 12:05 PST (History)
5 users (show)

See Also:

Attachments
WIP Patch (3.12 KB, patch)
2016-11-29 15:35 PST, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (4.26 KB, patch)
2016-11-30 12:20 PST, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (4.10 KB, patch)
2016-11-30 13:52 PST, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (4.20 KB, patch)
2016-12-01 14:01 PST, Chris Dumez 		no flags Details | Formatted Diff | Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2016-11-28 16:07:28 PST 
Tapping on an HTML validation bubble should dismiss it. It currently does nothing.
Comment 1 Chris Dumez 2016-11-29 15:35:05 PST 
Created attachment 295656 [details]
WIP Patch
Comment 2 Chris Dumez 2016-11-29 15:37:08 PST 
<rdar://problem/29429372>
Comment 3 Chris Dumez 2016-11-30 12:20:55 PST 
Created attachment 295741 [details]
Patch
Comment 4 Tim Horton 2016-11-30 13:06:00 PST 
Comment on attachment 295741 [details]
Patch

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

> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:52
> +- (WebValidationBubbleTapRecognizer *)initWithPopoverController:(UIViewController*)popoverController withPopoverView:(UIView *)popoverView

UIViewController star's on the wrong side. Also, do you really need to pass both? isn't popoverController.view popoverView?
Comment 5 Chris Dumez 2016-11-30 13:10:43 PST 
(In reply to comment #4)
> Comment on attachment 295741 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295741&action=review
> 
> > Source/WebCore/platform/ios/ValidationBubbleIOS.mm:52
> > +- (WebValidationBubbleTapRecognizer *)initWithPopoverController:(UIViewController*)popoverController withPopoverView:(UIView *)popoverView
> 
> UIViewController star's on the wrong side. Also, do you really need to pass
> both? isn't popoverController.view popoverView?

Good point. I'll fix.
Comment 6 Chris Dumez 2016-11-30 13:52:16 PST 
Created attachment 295761 [details]
Patch
Comment 7 Chris Dumez 2016-11-30 13:52:39 PST 
(In reply to comment #5)
> (In reply to comment #4)
> > Comment on attachment 295741 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=295741&action=review
> > 
> > > Source/WebCore/platform/ios/ValidationBubbleIOS.mm:52
> > > +- (WebValidationBubbleTapRecognizer *)initWithPopoverController:(UIViewController*)popoverController withPopoverView:(UIView *)popoverView
> > 
> > UIViewController star's on the wrong side. Also, do you really need to pass
> > both? isn't popoverController.view popoverView?
> 
> Good point. I'll fix.

Fixed.
Comment 8 Simon Fraser (smfr) 2016-11-30 13:58:46 PST 
Comment on attachment 295761 [details]
Patch

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

> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:58
> +    _tapGestureRecognizer = adoptNS([[getUITapGestureRecognizerClass() alloc] initWithTarget:self action:@selector(dismissPopover)]);

Are you sure this doesn't create a ref cycle?
Comment 9 Chris Dumez 2016-11-30 14:32:06 PST 
Comment on attachment 295761 [details]
Patch

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

>> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:58
>> +    _tapGestureRecognizer = adoptNS([[getUITapGestureRecognizerClass() alloc] initWithTarget:self action:@selector(dismissPopover)]);
> 
> Are you sure this doesn't create a ref cycle?

I see WebValidationBubbleTapRecognizer's dealloc getting called so it seems fine.
Comment 10 Chris Dumez 2016-12-01 14:01:28 PST 
Created attachment 295892 [details]
Patch
Comment 11 WebKit Commit Bot 2016-12-02 12:05:42 PST 
Comment on attachment 295892 [details]
Patch

Clearing flags on attachment: 295892

Committed r209253: <http://trac.webkit.org/changeset/209253>
Comment 12 WebKit Commit Bot 2016-12-02 12:05:46 PST 
All reviewed patches have been landed.  Closing bug.