Bug 224576

Summary:

[macOS] Make image extraction interactions work for elements inside links

Product:

WebKit

Reporter:

Wenson Hsieh <wenson_hsieh>

Component:

HTML Editing

Assignee:

Wenson Hsieh <wenson_hsieh>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bdakin, hi, megan_gardner, thorton, webkit-bug-importer, wenson_hsieh

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Address comments	none

Description Wenson Hsieh 2021-04-14 14:05:26 PDT

(Similar to #224517, but on macOS this time)

Comment 1 Radar WebKit Bug Importer 2021-04-14 14:12:26 PDT

<rdar://problem/76664721>

Comment 2 Wenson Hsieh 2021-04-14 14:15:13 PDT

Created attachment 426044 [details]
Patch

Comment 3 Devin Rousso 2021-04-14 14:26:48 PDT

Comment on attachment 426044 [details]
Patch

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

r=me

> Source/WebCore/page/EventHandler.cpp:960
> +    VisibleSelection newSelection { oldSelection };

NIT: Do we have a preference as to style of this vs something like `auto newSelection = oldSelection;`?

> Source/WebCore/page/EventHandler.cpp:1009
> +    auto isImageOverlayText = [&] (RefPtr<Node>&& node) {

NIT: Does this need to capture `&`?

NIT: It's a bit odd that this takes a `RefPtr<Node>&&` instead of just `RefPtr<Node>&` or something 🤔

Comment 4 Wenson Hsieh 2021-04-14 14:32:11 PDT

Comment on attachment 426044 [details]
Patch

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

Thanks for the review!

>> Source/WebCore/page/EventHandler.cpp:960
>> +    VisibleSelection newSelection { oldSelection };
> 
> NIT: Do we have a preference as to style of this vs something like `auto newSelection = oldSelection;`?

I don't think the style guidelines cover this, but I do agree that we generally use `auto/=` in new code, so I'll change it to that.

>> Source/WebCore/page/EventHandler.cpp:1009
>> +    auto isImageOverlayText = [&] (RefPtr<Node>&& node) {
> 
> NIT: Does this need to capture `&`?
> 
> NIT: It's a bit odd that this takes a `RefPtr<Node>&&` instead of just `RefPtr<Node>&` or something 🤔

Good point — removed the `&` capture, and changed to take `const RefPtr<Node>&`.

Comment 5 Wenson Hsieh 2021-04-14 14:38:34 PDT

(In reply to Wenson Hsieh from comment #4)
> Comment on attachment 426044 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=426044&action=review
> 
> Thanks for the review!
> 
> >> Source/WebCore/page/EventHandler.cpp:960
> >> +    VisibleSelection newSelection { oldSelection };
> > 
> > NIT: Do we have a preference as to style of this vs something like `auto newSelection = oldSelection;`?
> 
> I don't think the style guidelines cover this, but I do agree that we
> generally use `auto/=` in new code, so I'll change it to that.
> 
> >> Source/WebCore/page/EventHandler.cpp:1009
> >> +    auto isImageOverlayText = [&] (RefPtr<Node>&& node) {
> > 
> > NIT: Does this need to capture `&`?
> > 
> > NIT: It's a bit odd that this takes a `RefPtr<Node>&&` instead of just `RefPtr<Node>&` or something 🤔
> 
> Good point — removed the `&` capture, and changed to take `const
> RefPtr<Node>&`.

I'm going to add a `HTMLElement::isImageOverlayText(const Node*)` overload and use that instead, as you suggested over Slack.

Comment 6 Wenson Hsieh 2021-04-14 14:52:05 PDT

Created attachment 426050 [details]
Address comments

Comment 7 EWS 2021-04-14 19:04:27 PDT

Committed r275988 (236540@main): <https://commits.webkit.org/236540@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 426050 [details].