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

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

[Wenson Hsieh]

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

[Radar WebKit Bug Importer]

<rdar://problem/76664721>

[Wenson Hsieh]

Created attachment 426044 [details]
Patch

[Devin Rousso]

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 🤔

[Wenson Hsieh]

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>&`.

[Wenson Hsieh]

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

[Wenson Hsieh]

Created attachment 426050 [details]
Address comments

[EWS]

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