Bug 27048 – [Gtk] Implement STATE_FOCUSED, STATE_FOCUSABLE, and corresponding events for text objects

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Joanmarie Diggs]

According to: http://library.gnome.org/devel/atk/stable/atk-AtkState.html:

~~~
ATK_STATE_FOCUSABLE
	Indicates this object can accept keyboard focus, which means all events resulting from typing on the keyboard will normally be passed to it when it has focus

ATK_STATE_FOCUSED
	Indicates this object currently has the keyboard focus 
~~~

While I typically think of "focusable" being akin to "I can Tab to it," it seems to be used to mean "the active object." Thus we expect objects which implement the accessible text interface to have STATE_FOCUSABLE if the user can interact with the object via keyboard and STATE_FOCUSED if the user is interacting with the object. The corresponding events (focus: and object:state-changed:focused) are also expected.

WebKit objects of ROLE_TEXT lack these states and events.

While I can work around the absence of these things in some cases, there are other cases in which one needs to know if an object has focus. Example: distinguishing caret navigation related object:text-caret-moved and object:text-selection-changed events from the same events seen when using Epiphany's Find toolbar.

[Mario Sanchez Prada]

Hi Joanie, 

It would be wonderful if you could provide one of those manual test cases, that would probably help a lot both to properly understand the problem and to check further patches.

Not sure if that's possible (I know you're quite busy these days), but you know... hope is always there and I don't think there's anything bad in just checking :-). Thanks!

[Joanmarie Diggs]

Created attachment 72440 [details]
requested manual test case

Mario, I am more than happy to provide test cases in exchange for your awesome work. :-)

1. Open the attached in Epiphany and enable caret browsing.

2. Arrow Up and Down amongst the lines.

3. Click to move the caret from one paragraph to another.

Expected results: After steps 2 and 3, when each paragraph* gets the caret:

1. The AtkObject associated with the paragraph would emit an object:state-changed:focused event with detail1 == 1. A focus: event would also be emitted. And the AtkObject's state would change to include ATK_STATE_FOCUSED.

2. If the cursor was in some other paragraph, the AtkObject associated with the paragraph which was just exited would emit an object:state-changed:focused event with detail1 == 0. Its state would change to NOT include ATK_STATE_FOCUSED.

On a related note, any object which might at some point have ATK_STATE_FOCUSED should also have ATK_STATE_FOCUSABLE at all times.

* You'll notice that there's a forced line break in the first paragraph which of course results in the first paragraph consisting of two WebKit text render objects. We do NOT want the events or state changes to occur if the user moves from render object to render object *within the same paragraph*. We only want them when the user navigates from one exposed-to-ATs object to another exposed-to-ATs object.

Hope this helps. Let me know if you have questions. Thanks!

[Mario Sanchez Prada]

Created attachment 72774 [details]
Patch proposal

(In reply to comment #2)
> [...]
> Hope this helps. Let me know if you have questions. Thanks!

Really, really helpful... so much helpful that I'm now attaching a patch and asking for review over it :-). At the end I had to write a GTK-specific solution because it seems WebCore does not support focusing on text objects, so it's not as much a general solution as I'd have liked... but it works great with Accerciser, so I think it's worth it to propose it for review at this point.

Thanks Joanie!

[Mario Sanchez Prada]

Created attachment 72807 [details]
Patch proposal

Ooops! This is the right patch (tm) for review. The one I attached yesterday contained some "experimental" lines that shouldn't have ever seen the sunlight :-)

Sorry!

[Mario Sanchez Prada]

Created attachment 73270 [details]
Patch proposal

Yet another version of the patch, hopefully better than the previous one

[Mario Sanchez Prada]

Created attachment 74223 [details]
Patch proposal + Layout Test

Yet^2 another version more, this time featuring an useful Layout Test (that I should have written before, :/)

[Mario Sanchez Prada]

Created attachment 74224 [details]
Patch proposal + Layout Test

(In reply to comment #6)
> Created an attachment (id=74223) [details]
> Patch proposal + Layout Test
> 
> Yet^2 another version more, this time featuring an useful Layout Test (that I should have written before, :/)

Stupid me, I forgot to add the -expected.txt file in the previous patch. This is the good-really-good-I-swear patch to be reviewed.

Sorry for the noise.

[Xan Lopez]

Comment on attachment 74224 [details]
Patch proposal + Layout Test

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

Mostly looks good to me, just have some doubts/comments, see below.

> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:506
> +    Settings* settings = coreObject->document()->frame()->settings();

Some of those can be in some situations NULL. At the very least frame(). If this function can never be called in those cases then you should ASSERT.

> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:521
> +    return !selectionBelongsToObject(coreObject, selection) || selection.isNone() ? 0 : 1;

How is this grouping exactly? It's a quite confusing statement.

> WebCore/editing/gtk/SelectionControllerGtk.cpp:48
> +    static RefPtr<AccessibilityObject> oldObject = 0;

Mmm, I think you want to use DEFINE_STATIC_LOCAL here?

> WebCore/editing/gtk/SelectionControllerGtk.cpp:82
> +    // Reset lastFocuseNode and return for no valid selections.

I don't see that reset happening?

[Mario Sanchez Prada]

Created attachment 76411 [details]
Patch proposal + Layout Test

Attaching patch addressing all those issues. See details below...

(In reply to comment #8)
> (From update of attachment 74224 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74224&action=review
> 
> Mostly looks good to me, just have some doubts/comments, see below.
> 
> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:506
> > +    Settings* settings = coreObject->document()->frame()->settings();
> 
> Some of those can be in some situations NULL. At the very least frame(). If this function can never be called in those cases then you should ASSERT.

Fixed, by adding extra checks to the function (better to be safe!)

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:521
> > +    return !selectionBelongsToObject(coreObject, selection) || selection.isNone() ? 0 : 1;
> 
> How is this grouping exactly? It's a quite confusing statement.

Fixed by replacing it for equivalent code.

> > WebCore/editing/gtk/SelectionControllerGtk.cpp:48
> > +    static RefPtr<AccessibilityObject> oldObject = 0;
> 
> Mmm, I think you want to use DEFINE_STATIC_LOCAL here?

Fixed.

> > WebCore/editing/gtk/SelectionControllerGtk.cpp:82
> > +    // Reset lastFocuseNode and return for no valid selections.
> 
> I don't see that reset happening?

I meant resetting it to 'zero'. Fixed comment

Now asking for review again then...

[Xan Lopez]

Comment on attachment 76411 [details]
Patch proposal + Layout Test

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

> WebCore/editing/gtk/SelectionControllerGtk.cpp:43
> +static void maybeEmitTextFocusChange(PassRefPtr<AccessibilityObject> prpObject)

Can you use a full name instead of 'prpObject' when landing? Thanks.

[Mario Sanchez Prada]

(In reply to comment #10)
> (From update of attachment 76411 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76411&action=review
> 
> > WebCore/editing/gtk/SelectionControllerGtk.cpp:43
> > +static void maybeEmitTextFocusChange(PassRefPtr<AccessibilityObject> prpObject)
> 
> Can you use a full name instead of 'prpObject' when landing? Thanks.

I used that prefix according to the Guidelines written in here:

http://webkit.org/coding/RefPtr.html > Guidelines > Function arguments

Agree it's a weird prefix, although not that weird once you know it means "PassRefPtr" :-). Other than that, it's widely used in WebCore:

dom/Node.cpp:2572:bool Node::dispatchEvent(PassRefPtr<Event> prpEvent)
dom/Node.cpp:2596:bool Node::dispatchGenericEvent(PassRefPtr<Event> prpEvent)
dom/NamedNodeMap.cpp:248:void NamedNodeMap::addAttribute(PassRefPtr<Attribute> prpAttribute)
dom/Range.cpp:919:void Range::insertNode(PassRefPtr<Node> prpNewNode, ExceptionCode& ec)
editing/CompositeEditCommand.cpp:273:void CompositeEditCommand::mergeIdenticalElements(PassRefPtr<Element> prpFirst, PassRefPtr<Element> prpSecond)
[...]

Hence, as agreed via IRC with Xan, committing the patch as it is right now...

[Mario Sanchez Prada]

Committed r74025: <http://trac.webkit.org/changeset/74025>