Bug 106539 – PDFPlugin: Tab between text annotations

• Component: PDF
• Status: RESOLVED FIXED

[Tim Horton]

<rdar://problem/12751789>

Should be able to use tab/shift-tab to navigate through text annotations.

[Tim Horton]

Created attachment 182089 [details]
patch

[Alexey Proskuryakov]

Comment on attachment 182089 [details]
patch

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

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.h:74
> +        PDFPluginTextAnnotation* m_annotation;

So the design is that the pointer can become stale, but it won't be used in that case?

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.mm:137
> +        if (keyboardEvent->keyCode() == VK_TAB) {
> +            if (keyboardEvent->getModifierState("Shift"))

This is not really the right way to check for key combinations. We should let Cocoa decide whether a key combination matches.

It's easy to observe the difference by adding a new combination for insertTab: in DefaultKeyBinding.dict. Also, Cocoa accurately checks all modifiers, so it doesn't steal e.g. Ctrl+Tab.

[Tim Horton]

(In reply to comment #2)
> This is not really the right way to check for key combinations. We should let Cocoa decide whether a key combination matches.
> 
> It's easy to observe the difference by adding a new combination for insertTab: in DefaultKeyBinding.dict. Also, Cocoa accurately checks all modifiers, so it doesn't steal e.g. Ctrl+Tab.

We took a look at this today; it seems like it's going to be a lot of work to let Cocoa figure this out for us, so I think we should file another bug about that. In the meantime, I'll make sure that ctrl/meta/'altgraph' aren't down.

[Tim Horton]

Comment on attachment 182089 [details]
patch

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

>> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.h:74
>> +        PDFPluginTextAnnotation* m_annotation;
> 
> So the design is that the pointer can become stale, but it won't be used in that case?

Hmm? PDFPluginTextAnnotationEventListener won't outlive its parent PDFPluginTextAnnotation.

[Tim Horton]

Created attachment 186973 [details]
patch

[Alexey Proskuryakov]

> Hmm? PDFPluginTextAnnotationEventListener won't outlive its parent PDFPluginTextAnnotation.

Why not? PDFPluginTextAnnotationEventListener is RefCounted.

[Darin Adler]

Comment on attachment 186973 [details]
patch

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

review+ but only if you add a check of isKeyboardEvent to fix the bad cast

> Source/WebCore/ChangeLog:9
> +        * WebCore.exp.in: Export KeyboardEvent::getModifierState.

Why? I don’t see it being used in this patch. Is it used indirectly?

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.h:63
> +        virtual bool operator==(const EventListener& listener) OVERRIDE { return this == &listener; }

Normally a virtual == operator is an anti-pattern. Not sure if there’s any real problem with it here.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.mm:131
> +void PDFPluginTextAnnotation::PDFPluginTextAnnotationEventListener::handleEvent(ScriptExecutionContext*, Event* event)

The code in EventHandler::defaultTabEventHandler checks Page::tabKeyCyclesThroughElements. Is that something we need to do too?

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.mm:134
> +    if (event->type() == eventNames().keydownEvent) {
> +        KeyboardEvent* keyboardEvent = static_cast<KeyboardEvent*>(event);

You need to check the type of the event before casting to KeyboardEvent by calling isKeyboardEvent. A script can create an event with a given event name that is not a “real” event of that type and if so this will be a bad cast.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.mm:136
> +        if (keyboardEvent->keyCode() == VK_TAB) {

The code in EventHandler::defaultKeyboardEventHandler uses keyIdentifier rather than keyCode. Any reason you chose differently?

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.mm:138
> +            if (keyboardEvent->ctrlKey() || keyboardEvent->metaKey() || keyboardEvent->altKey() || keyboardEvent->altGraphKey())
> +                return;

The code in EventHandler::defaultTabEventHandler does not check altKey. Any reason for the difference?

[Darin Adler]

(In reply to comment #2)
> This is not really the right way to check for key combinations. We should let Cocoa decide whether a key combination matches.

If we’re trying to make PDF handling match webpage tab key handling, we could use EventHandler as the model instead of trying to wire Cocoa up. I think there are arguments either way.

[Alexey Proskuryakov]

I think that in practice, it's extremely unlikely to have anyone remap Tab, and we have some level of '\t' hardcoding in normal key event handling anyway.

I'm still concerned about stale pointer issue.

[Darin Adler]

(In reply to comment #9)
> I'm still concerned about stale pointer issue.

When I reviewed, I did not see your comment about that from the previous review.

Tim, lets fix that stale pointer issue by having ~PDFPluginAnnotation call a function that zeroes the pointer, and make sure you null check. We should do it mechanically and make sure to not have pointer lifetime depend on high level understanding of the code.

[Tim Horton]

(In reply to comment #7)
> (From update of attachment 186973 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186973&action=review
> 
> review+ but only if you add a check of isKeyboardEvent to fix the bad cast
> 
> > Source/WebCore/ChangeLog:9
> > +        * WebCore.exp.in: Export KeyboardEvent::getModifierState.
> 
> Why? I don’t see it being used in this patch. Is it used indirectly?

Good call. Not being used anymore.

> > Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.h:63
> > +        virtual bool operator==(const EventListener& listener) OVERRIDE { return this == &listener; }
> 
> Normally a virtual == operator is an anti-pattern. Not sure if there’s any real problem with it here.

Interesting! I was just following all the other EventListener subclasses I saw.

> > Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.mm:131
> > +void PDFPluginTextAnnotation::PDFPluginTextAnnotationEventListener::handleEvent(ScriptExecutionContext*, Event* event)
> 
> The code in EventHandler::defaultTabEventHandler checks Page::tabKeyCyclesThroughElements. Is that something we need to do too?

Looking at the history of tabKeyCyclesThroughElements (r15093!), I don't really think we care in the PDFPlugin case. Even if it were embedded in another app, if a PDFPlugin form has focus, tab should tab to the next field, not insert a tab (IMO tabKeyCyclesThroughElements only makes sense for contenteditable areas and such, not PDF forms).

> > Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.mm:134
> > +    if (event->type() == eventNames().keydownEvent) {
> > +        KeyboardEvent* keyboardEvent = static_cast<KeyboardEvent*>(event);
> 
> You need to check the type of the event before casting to KeyboardEvent by calling isKeyboardEvent. A script can create an event with a given event name that is not a “real” event of that type and if so this will be a bad cast.

Ooh, that's unfortunate. Ok, will fix.

> > Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.mm:136
> > +        if (keyboardEvent->keyCode() == VK_TAB) {
> 
> The code in EventHandler::defaultKeyboardEventHandler uses keyIdentifier rather than keyCode. Any reason you chose differently?

Not in particular, I'll match them.

> > Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.mm:138
> > +            if (keyboardEvent->ctrlKey() || keyboardEvent->metaKey() || keyboardEvent->altKey() || keyboardEvent->altGraphKey())
> > +                return;
> 
> The code in EventHandler::defaultTabEventHandler does not check altKey. Any reason for the difference?

Nope, makes sense to match them. Will fix.

> Tim, lets fix that stale pointer issue by having ~PDFPluginAnnotation call a function that zeroes the pointer, and make sure you null check. We should do it mechanically and make sure to not have pointer lifetime depend on high level understanding of the code.

Okie.

[Tim Horton]

Created attachment 190111 [details]
addressing darin's comments

[Tim Horton]

Created attachment 190113 [details]
update changelog too

[Tim Horton]

http://trac.webkit.org/changeset/143962