Bug 27048

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!

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!

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!

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!

Created attachment 73270 [details] Patch proposal Yet another version of the patch, hopefully better than the previous one

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, :/)

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.

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?

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

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.

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

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