REGRESSION (r38629): Cannot scroll a WebHTMLView using Home/End/Page up/Page down

• Component: HTML Editing
• Status: RESOLVED FIXED

[Cameron Zwarich (cpst)]

After r38629, all keyboard events get sent by Editor to the EditorClient, even if the selection is not editable. If the event's command is unsupported by WebHTMLView, WebHTMLView mistakingly thinks that the event was handled when it was not. When using the page up / page down keys, the events generated are of the form scrollPageUp rather than movePageUp, so they are unsupported by WebHTMLView and cause this bug to occur.

[Cameron Zwarich (cpst)]

<rdar://problem/6467830>

[Cameron Zwarich (cpst)]

Created attachment 26339 [details]
Proposed patch

Here's a patch that fixes the problem. This is probably testable in DRT, but I'd rather get the fix out of the way before deciding how to test it.

[Darin Adler]

Comment on attachment 26339 [details]
Proposed patch

Typo "mistakenly" rather than "mistakingly".

> -        bool eventWasHandled = true;
> +        bool eventWasHandled = false;

This changes the value when the delegate returns YES and when coreFrame is 0. Did you really mean to change it in those cases? Why?

> -            else {
> +            else if ([self _canEdit]) {
>                  _private->selectorForDoCommandBySelector = selector;
>                  [super doCommandBySelector:selector];
>                  _private->selectorForDoCommandBySelector = 0;
> +                eventWasHandled = true;
>              }

What this really amounts to is:

   if _canEdit returns false, then set eventWasHandled to false and don't call super's doCommandBySelector

So for one thing, you can write it that way:

    else if (![self_canEdit])
        eventWasHandled = false;
    else
        ...

Without changing the surrounding code, if you like.

But for another, I don't understand the rationale here. Maybe we should never treat calling [super doCommandBySelector:] as "eventWasHandled true" -- I don't see what _canEdit has to do with it.

[Cameron Zwarich (cpst)]

(In reply to comment #3)
> (From update of attachment 26339 [details] [review])
> Typo "mistakenly" rather than "mistakingly".
> 
> > -        bool eventWasHandled = true;
> > +        bool eventWasHandled = false;
> 
> This changes the value when the delegate returns YES and when coreFrame is 0.
> Did you really mean to change it in those cases? Why?
> 
> > -            else {
> > +            else if ([self _canEdit]) {
> >                  _private->selectorForDoCommandBySelector = selector;
> >                  [super doCommandBySelector:selector];
> >                  _private->selectorForDoCommandBySelector = 0;
> > +                eventWasHandled = true;
> >              }
> 
> What this really amounts to is:
> 
>    if _canEdit returns false, then set eventWasHandled to false and don't call
> super's doCommandBySelector
> 
> So for one thing, you can write it that way:
> 
>     else if (![self_canEdit])
>         eventWasHandled = false;
>     else
>         ...
> 
> Without changing the surrounding code, if you like.

Yeah, I accidentally changed the delegate case. I originally had it like you said, but thought it would be better to make the default false rather than true. I should never doubt my instincts. ;-)

> But for another, I don't understand the rationale here. Maybe we should never
> treat calling [super doCommandBySelector:] as "eventWasHandled true" -- I don't
> see what _canEdit has to do with it.

I would have thought about doing that, but I couldn't answer this question: when does [super doCommandBySelector:] ever handle an event? Presumedly there is some case if the code was added long ago.

[Cameron Zwarich (cpst)]

Comment on attachment 26339 [details]
Proposed patch

Clearing review flag pending a new patch.

[Darin Adler]

(In reply to comment #4)
> I would have thought about doing that, but I couldn't answer this question:
> when does [super doCommandBySelector:] ever handle an event? Presumedly there
> is some case if the code was added long ago.

I think the case has something to do with input methods, but I really don't know.

[Cameron Zwarich (cpst)]

Created attachment 26340 [details]
Proposed patch

Here is the revised patch. I don't want to change the default in the editable case if you don't know exactly why the message is passed to super. Also, 'mistakingly' is valid English, just a bit out of use. I changed it though. ;-)

The same comment stands about the test.

[Darin Adler]

Comment on attachment 26340 [details]
Proposed patch

r=me

I think this would be slightly better if it had a comment explaining what's going on.

[Cameron Zwarich (cpst)]

Fixed in r39549, with a comment added like Darin suggested.