Bug 207866

Created attachment 390999 [details]
Patch

Patch built on top of rename done in patch for bug #207865

Created attachment 391057 [details]
Patch

<rdar://problem/59553028>

Created attachment 391059 [details]
Patch

Created attachment 391060 [details]
Patch

Yikes! I was wavering whether to make SetShouldRevealCurrentSelectionAfterInsertion cross-platform or not. In the end I decided that is required setting aside some space for storage and that I only need this for iOS at the moment. So, I only made the message SetShouldRevealCurrentSelectionAfterInsertion iOS-specific. But I forgot to update WebPageProxy.{h, cpp} of this decision. Will update....

Created attachment 391064 [details]
Patch

Comment on attachment 391064 [details]
Patch

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

> Source/WebCore/editing/Editor.cpp:1304
> +                if (auto* editedFrame = document->frame()) {
> +                    if (auto* page = editedFrame->page())
> +                        page->focusController().focusedOrMainFrame().selection().revealSelection(SelectionRevealMode::Reveal, ScrollAlignment::alignCenterIfNeeded);
>                  }

Looking at this again, should I extract this into a common function, WEBCORE_EXPORT Editor::revealCurrentSelection(), then call from here and WebPage::setShouldRevealCurrentSelectionAfterInsertion()? Thoughts?

Comment on attachment 391064 [details]
Patch

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

(Just some quick comments for now — I'll continue to look)

> Source/WebCore/ChangeLog:9
> +        the client wants the enginer to reveal the current selection after insertion. The default implementation

Nit - “enginer"

> Source/WebKit/ChangeLog:17
> +        Add forward declarations for some functions. I am not using these now, but I will make use
> +        of them to fix <rdar://problem/57608794>.

Nit - can we just add these when we need them?

> Source/WebKit/ChangeLog:34
> +        was still on and hence the next layer tree update or editor state change would cause -_didUpdateEditorState
> +        to be called, which could zoom or scroll the UIScrollView by code inspection.

This refactoring seems fine, but this statement in particular doesn’t sound right. -_didReceiveEditorStateUpdateAfterFocus will only zoom if _focusedElementInformation.elementType is not None, which it is in the case where the delegate prevents an input session from starting.

>> Source/WebCore/editing/Editor.cpp:1304
>>                  }
> 
> Looking at this again, should I extract this into a common function, WEBCORE_EXPORT Editor::revealCurrentSelection(), then call from here and WebPage::setShouldRevealCurrentSelectionAfterInsertion()? Thoughts?

Sure.

Comment on attachment 391064 [details]
Patch

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

Thanks Wenson!

>> Source/WebKit/ChangeLog:17
>> +        of them to fix <rdar://problem/57608794>.
> 
> Nit - can we just add these when we need them?

I need them now, but the code that uses them will be in Apple Internal. That radar link is for the corresponding Apple Internal change that uses them.

>> Source/WebKit/ChangeLog:34
>> +        to be called, which could zoom or scroll the UIScrollView by code inspection.
> 
> This refactoring seems fine, but this statement in particular doesn’t sound right. -_didReceiveEditorStateUpdateAfterFocus will only zoom if _focusedElementInformation.elementType is not None, which it is in the case where the delegate prevents an input session from starting.

I'm just going to delete the "Doing the former also fixes a correctness issue in the code" bits.

Comment on attachment 391064 [details]
Patch

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

Seems reasonable to me overall; just a quick question.

>>> Source/WebKit/ChangeLog:17
>>> +        of them to fix <rdar://problem/57608794>.
>> 
>> Nit - can we just add these when we need them?
> 
> I need them now, but the code that uses them will be in Apple Internal. That radar link is for the corresponding Apple Internal change that uses them.

Ah, I see — sounds good to me then.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:4210
> +    m_shouldRevealCurrentSelectionAfterInsertion = shouldRevealCurrentSelectionAfterInsertion;

Please double check that we don’t end up leaking `m_shouldRevealCurrentSelectionAfterInsertion` state, e.g. across top-level navigation.

(In reply to Wenson Hsieh from comment #12)
> Comment on attachment 391064 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=391064&action=review
> 
> Seems reasonable to me overall; just a quick question.
> 
> >>> Source/WebKit/ChangeLog:17
> >>> +        of them to fix <rdar://problem/57608794>.
> >> 
> >> Nit - can we just add these when we need them?
> > 
> > I need them now, but the code that uses them will be in Apple Internal. That radar link is for the corresponding Apple Internal change that uses them.
> 
> Ah, I see — sounds good to me then.
> 
> > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:4210
> > +    m_shouldRevealCurrentSelectionAfterInsertion = shouldRevealCurrentSelectionAfterInsertion;
> 
> Please double check that we don’t end up leaking
> `m_shouldRevealCurrentSelectionAfterInsertion` state, e.g. across top-level
> navigation.

Yeah, we should clear it...

Created attachment 391084 [details]
Patch

Comment on attachment 391084 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5868
> +    m_shouldRevealCurrentSelectionAfterInsertion = false;

Shouldn’t this be true by default?

(In reply to Wenson Hsieh from comment #15)
> Comment on attachment 391084 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=391084&action=review
> 
> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:5868
> > +    m_shouldRevealCurrentSelectionAfterInsertion = false;
> 
> Shouldn’t this be true by default?

It should.

Created attachment 391089 [details]
Patch

Comment on attachment 391089 [details]
Patch

r=mews

Comment on attachment 391089 [details]
Patch

Clearing flags on attachment: 391089

Committed r256864: <https://trac.webkit.org/changeset/256864>

All reviewed patches have been landed.  Closing bug.

Comment on attachment 391089 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:591
> +    if (!editor.ignoreSelectionChanges() && editor.client()->shouldRevealCurrentSelectionAfterInsertion() && (editor.hasComposition() || !frame.selection().selection().isNone()))

Looks like the’s a missing client() null check here too, in addition to the one fixed in bug 207925.

(In reply to Darin Adler from comment #21)
> Comment on attachment 391089 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=391089&action=review
> 
> > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:591
> > +    if (!editor.ignoreSelectionChanges() && editor.client()->shouldRevealCurrentSelectionAfterInsertion() && (editor.hasComposition() || !frame.selection().selection().isNone()))
> 
> Looks like the’s a missing client() null check here too, in addition to the
> one fixed in bug 207925.

Yeah, I know, but I am pretty sure it doesn't matter because iOS always assigns an editor client and the client is not nullable once set. I could be misremembering. I will look once more tomorrow.

The missing null in WebPage.cpp mattered because it was cross platform. I didn't make that mistake in the other cross platform code in Editor.cpp....

(In reply to Daniel Bates from comment #22)
> (In reply to Darin Adler from comment #21)
> > Comment on attachment 391089 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=391089&action=review
> > 
> > > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:591
> > > +    if (!editor.ignoreSelectionChanges() && editor.client()->shouldRevealCurrentSelectionAfterInsertion() && (editor.hasComposition() || !frame.selection().selection().isNone()))
> > 
> > Looks like the’s a missing client() null check here too, in addition to the
> > one fixed in bug 207925.
> 
> Yeah, I know, but I am pretty sure it doesn't matter because iOS always
> assigns an editor client and the client is not nullable once set. I could be
> misremembering. I will look once more tomorrow.

Thanks for pointing this out by the way.

I don't like my phrasing of "Doesn't matter". That sounds flippant and I didn't mean for it to be. Here's a better way to say things: I thought about this code and as far as I can remember I ruled out the need for an extra check because iOS always assigns a client (at least that's what I remember). I could be misremembering and I will double check tomorrow.

Comment on attachment 391089 [details]
Patch

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

>>>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:591
>>>> +    if (!editor.ignoreSelectionChanges() && editor.client()->shouldRevealCurrentSelectionAfterInsertion() && (editor.hasComposition() || !frame.selection().selection().isNone()))
>>> 
>>> Looks like the’s a missing client() null check here too, in addition to the one fixed in bug 207925.
>> 
>> Yeah, I know, but I am pretty sure it doesn't matter because iOS always assigns an editor client and the client is not nullable once set. I could be misremembering. I will look once more tomorrow.
> 
> Thanks for pointing this out by the way.
> 
> I don't like my phrasing of "Doesn't matter". That sounds flippant and I didn't mean for it to be. Here's a better way to say things: I thought about this code and as far as I can remember I ruled out the need for an extra check because iOS always assigns a client (at least that's what I remember). I could be misremembering and I will double check tomorrow.

So, Editor::client() will always return non-nullptr here because 1) we have a page and 2) A WebCore::Page always is initialized with an editor client on iOS. If you find this too subtle then I can add a null check.

(In reply to Daniel Bates from comment #25)
> Comment on attachment 391089 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=391089&action=review
> 
> >>>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:591
> >>>> +    if (!editor.ignoreSelectionChanges() && editor.client()->shouldRevealCurrentSelectionAfterInsertion() && (editor.hasComposition() || !frame.selection().selection().isNone()))
> >>> 
> >>> Looks like the’s a missing client() null check here too, in addition to the one fixed in bug 207925.
> >> 
> >> Yeah, I know, but I am pretty sure it doesn't matter because iOS always assigns an editor client and the client is not nullable once set. I could be misremembering. I will look once more tomorrow.
> > 
> > Thanks for pointing this out by the way.
> > 
> > I don't like my phrasing of "Doesn't matter". That sounds flippant and I didn't mean for it to be. Here's a better way to say things: I thought about this code and as far as I can remember I ruled out the need for an extra check because iOS always assigns a client (at least that's what I remember). I could be misremembering and I will double check tomorrow.
> 
> So, Editor::client() will always return non-nullptr here because 1) we have
> a page and 2) A WebCore::Page always is initialized with an editor client on
> iOS. If you find this too subtle then I can add a null check.

Turns out (1) is wrong. Fixed in bug 207939