Bug 26991

Making a depends on 30883 because in that bug I'm flattening the hierarchy w.r.t. text objects. Those changes may just fix this bug completely, but if they do not, they will still most certainly impact this bug as the offending text objects in this bug will no longer exist. :-)

I'm working on this and I found that currently one of the big issues to properly implement this is to reliably determine when a VisibleSelection belongs to the selected AccessibleObject, since such a selection is got right away from the frame (in a global fashion) and hence there's no API to actually do something like "get the node for that selection". Hence, I was playing with some possibilities but all of them failed so far. Basically they were: - Get the containerNode for the selection (what current code does): doesn't work, as the returned node has nothing to do with DOM nodes (it's a node which parent is the #document node). - Check the absolute coords for the selection and check them against the extents of the selected node: I don't think that's a good way to do it since it could leave to false positives when different nodes are rendered in the same places (overlay text with CSS, for instance). - Trying to get the AccessibleObject from the renderer() associated to the base/extent/start/end of the VisibleSelection, and match it against the node associated to the selected object: in progress (still need to figure out what exactly those base/extent/start/end things mean and how to work with them) So this is just a temporary braindump while I'm working on this. Perhaps someone wants to share something that shed some light on the topic :-)

Created attachment 64105 [details] Patch to fix this issue [Disclaimer: this is WIP, not a patch to be reviewed at this moment] After working on this stuff I think I finally got a reliable way to check whether a global selection belongs (even partially only) to a accessible object inside the Atk wrapper's code, thus allowing to return the right values when executing getNSelections() in the kind of scenarios as those described by Joanmarie, and even in other cases I've tested (such as selecting text accross diferent paragraphs at the same level, with more and more nested elements inside of it and so on). However, or the implementation of getSelection, I need to add some more code (but based on the new code in selectionBelongsToObject()) to allow getting the exact values for the start and end offsets of a selection *for a given object* (so if you have a full paragraph selected and just ask a link inside of it for the offsets they are relative to such link), and I have to say it works pretty fine in all the cases I've tested. Hence, I guess now the next step here would be to write some unit tests (or even layout tests?) for this new code, so I'll focus on that. However, as I was chatting to Joanmarie on IRC, I couldn't find an obvious way to select and unselect text so I could write those unit tests, so perhaps it could be worth it to try to fix bug 25673 first and then write unit tests for both the setSelection and getSelection stuff. Now attaching a patch with the code I've working now, in case you want to test it or comment on top of it.

Created attachment 64234 [details] Patch proposal Now attaching a true patch and asking for review over it. The current new patch contains basically the same thing than the previous one but slightly cleaned up and got some fixes about some things that were not 100% correct in the previous version. (In reply to comment #3) > [...] > Hence, I guess now the next step here would be to write > some unit tests (or even layout tests?) for this new code, > so I'll focus on that. At last, I did this but in a separate (new filed) bug (bug 43919), set as dependent on bug 26991 and bug 25673, as I thought it was the best way considering that both bugs needed to be fixed first before being able to actually implement good unit tests in testatk.c for both of them (see more reasons in https://bugs.webkit.org/show_bug.cgi?id=43919#c0) > However, as I was chatting to Joanmarie on IRC, I couldn't > find an obvious way to select and unselect text so I could > write those unit tests, so perhaps it could be worth it to > try to fix bug 25673 first and then write unit tests for > both the setSelection and getSelection stuff. Done that way, which is great since we get rid of two bugs for the prize of one (See bug 25673 and 43919 for more details)

Comment on attachment 64234 [details] Patch proposal > [Gtk] get_n_selections and get_selection fail when selecting text across object boundaries > https://bugs.webkit.org/show_bug.cgi?id=26991 > > Fix AtkText getNSelections() and getSelection() to work properly > > Changed the implementation of the selectionBelongsToObject() > function in the Atk wrapper to return a reliable value by > carefully checking that the selection belongs to the selected > object or some of its descendants. > > Implemented a new function to get the start and end offsets of a > selection for a given accessible object. > > Made it so webkit_accessible_text_get_selection returns zero when > both start and end offsets are equal, following GAIL's lead. > > * accessibility/gtk/AccessibilityObjectWrapperAtk.cpp: > (selectionBelongsToObject): > (getSelectionOffsetsForObject): New. > (webkit_accessible_text_get_selection): You should put your descriptions of the functions inline with the ChangeLog file and method names. It makes the result more readable. Take a look at the existing ChangeLog entries in WebCore. > + * accessibility/gtk/AccessibilityObjectWrapperAtk.cpp: > + (selectionBelongsToObject): > + (getSelectionOffsetsForObject): New. > + (webkit_accessible_text_get_selection): Same here. > + // Make range for the given selection > + RefPtr<Range> range = makeRange(selection.visibleStart(), selection.visibleEnd()); > + if (!range) > + return false; Comments like this really don't add anything. Your comments should aways answer 'Why?' and not just 'What?' > + // Check whether selected node is the selection's owner For instance here, I'm unsure why a node owns a selection if it is inside the selection range. > + Node* node = coreObject->node(); > + for (TextIterator it(range.get()); !it.atEnd(); it.advance()) { > + Node* nodeInRange = it.range()->startContainer(); > + if (nodeInRange && nodeInRange->isDescendantOf(node)) > + return true; > + } > + > + // Default > + return false; > +} > + // Get the offsets of the selection for the selected object > + AccessibilityObject* coreObject = core(text); > + VisibleSelection selection = coreObject->selection(); > + getSelectionOffsetsForObject(coreObject, selection, *start_offset, *end_offset); Perhaps I simply do not understand this section of code, but couldn't you simply call core(text)->doAXStringForRange(coreObject->selectedTextRange()) and return that? That would allow you to get rid of most of the code above. > + // Return the text selected between the offsets if different or zero otherwise > + if (*start_offset != *end_offset) > + return webkit_accessible_text_get_text(text, *start_offset, *end_offset); > - return webkit_accessible_text_get_text(text, *start_offset, *end_offset); > + return 0; It is my preference that the failure case "return 0" is the early return. Also, the comment doesn't add much and doesn't explain why NULL is returned instead of "".

Created attachment 66035 [details] Patch proposal Attaching new patch addressing the issues pointed out in comment #5. See my comments below: (In reply to comment #5) > [...] > You should put your descriptions of the functions inline with the > ChangeLog file and method names. It makes the result more readable. > > Take a look at the existing ChangeLog entries in WebCore. Done. > > + // Make range for the given selection > > + RefPtr<Range> range = makeRange(selection.visibleStart(), selection.visibleEnd()); > > + if (!range) > > + return false; > > Comments like this really don't add anything. Your comments should aways > answer 'Why?' and not just 'What?' > > > + // Check whether selected node is the selection's owner > > For instance here, I'm unsure why a node owns a selection if it is > inside the selection range. I agree with you, sorry for the confusion. Changed :-) > > + Node* node = coreObject->node(); > > + for (TextIterator it(range.get()); !it.atEnd(); it.advance()) { > > + Node* nodeInRange = it.range()->startContainer(); > > + if (nodeInRange && nodeInRange->isDescendantOf(node)) > > + return true; > > + } > > + > > + // Default > > + return false; > > +} > > > + // Get the offsets of the selection for the selected object > > + AccessibilityObject* coreObject = core(text); > > + VisibleSelection selection = coreObject->selection(); > > + getSelectionOffsetsForObject(coreObject, selection, *start_offset, *end_offset); > > Perhaps I simply do not understand this section of code, but couldn't > you simply call core(text)->doAXStringForRange(coreObject->selectedTextRange()) and > return that? That would allow you to get rid of most of the code above. I know what you mean, but in this case I think that wouldn't be enough, since we need both to return the selected text and the offsets *relative to the AtkObject* where we are asking for a text selection. That's the reason behind the getSelectionOffsetsForObject() method. > > + // Return the text selected between the offsets if different or zero otherwise > > + if (*start_offset != *end_offset) > > + return webkit_accessible_text_get_text(text, *start_offset, *end_offset); > > - return webkit_accessible_text_get_text(text, *start_offset, *end_offset); > > + return 0; > > It is my preference that the failure case "return 0" is the early return. > Also, the comment doesn't add much and doesn't explain why NULL is returned > instead of "". Changed.

Created attachment 66039 [details] Patch proposal (In reply to comment #6) > Created an attachment (id=66035) [details] > Patch proposal > > Attaching new patch addressing the issues pointed out in comment #5. Sorry, I messed things up with git (forgot to squash patches). Now attaching the right one.

Attachment 66039 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1405: Use 0 instead of NULL. [readability/null] [4] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 66734 [details] Patch proposal Attaching new up-to-date version now we (Martin and I) decided to bundle unit tests for this stuff along with patch for bug 256773 (instead of using a new bug just for that, like bug 43919)

Comment on attachment 66734 [details] Patch proposal View in context: https://bugs.webkit.org/attachment.cgi?id=66734&action=prettypatch Looks good, but I have a few concerns. > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1336 > + return false; Any reason you aren't using selection.firstRange() or selection.toNormalizedRange() here? > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1341 > + for (TextIterator it(range.get()); !it.atEnd(); it.advance()) { Would it be possible to replace this loop with Range::intersectsNode? > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1360 > + RefPtr<Range> range = makeRange(selection.visibleStart(), selection.visibleEnd()); Again it seems like this should be selection.firstRange() or selection.toNormalizedRange(). > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1376 > + break; Tricky loop logic. :/ I admit I'm not sure exactly what the range return value is supposed to represent, but could you take a non-loop approach, such as the one used in AccessibilityRenderObject::ariaSelectedTextDOMRange in WebCore/accessibility/AccessibilityRenderObject.cpp? > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1397 > + if (selection_num) There are lots of non-WebKit style variable names in this file, but we should be converting them gradually to WebKit style. Please do this for all other arguments of this method as well.

Created attachment 67204 [details] Patch proposal (In reply to comment #10) > (From update of attachment 66734 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=66734&action=prettypatch > > Looks good, but I have a few concerns. Thanks for the review, I've been working on this addressing these issues and I have to say I learnt quite a lot of interesting things in the way, so here you have a new patch, hopefully better :-) > > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1336 > > + return false; > Any reason you aren't using selection.firstRange() or selection.toNormalizedRange() here? I didn't know of those :-). selection.toNormalizedRange() works fine in this case so I used that one in this new patch > > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1341 > > + for (TextIterator it(range.get()); !it.atEnd(); it.advance()) { > Would it be possible to replace this loop with Range::intersectsNode? In this case intersectsNode() *almost* works ok but it fails when the selection is right touching one of the boundaries of the selected node so that wouldn't be enough. Anyway, I still could replace the loop with a combination of intersectsNode() + 2 extra checks to make sure we don't return TRUE if the situation with the boundaries happened. Thanks for the tip. > > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1360 > > + RefPtr<Range> range = makeRange(selection.visibleStart(), selection.visibleEnd()); > Again it seems like this should be selection.firstRange() or selection.toNormalizedRange(). Changed to selection.toNormalizedRange(). > > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1376 > > + break; > Tricky loop logic. :/ I admit I'm not sure exactly what the range return value is supposed to represent, but could you take a non-loop approach, such as the one used in AccessibilityRenderObject::ariaSelectedTextDOMRange in WebCore/accessibility/AccessibilityRenderObject.cpp? It is not exactly the same thing because we need to return the offsets of the selection relative to the selected object, so some manual calculation, at least for the endoffset is still needed. However, I was able to improve the code (I think) making it more readable (easier to understand) and efficient (still a loop but with far less iterations), allowing to determine starOffset in a much more logical fashion, and keeping a small loop just to compute the length of the intermediate nodes to build the "effective" value for endOffset > > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1397 > > + if (selection_num) > There are lots of non-WebKit style variable names in this file, but we should be converting them gradually to WebKit style. Please do this for all other arguments of this method as well. I fixed those as well in this new patch, compiled and tested against the very vest version of trunk :-)

Attachment 67204 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1433: webkit_accessible_text_get_selection is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 67213 [details] Patch proposal (In reply to comment #11) > Thanks for the review, I've been working on this addressing these issues and I have to say I learnt quite a lot of interesting things in the way, so here you have a new patch, hopefully better :-) This is *the right patch*, sorry for the noise

Attachment 67213 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1434: webkit_accessible_text_get_selection is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 67213 [details] Patch proposal View in context: https://bugs.webkit.org/attachment.cgi?id=67213&action=prettypatch Okay!

Comment on attachment 67213 [details] Patch proposal Have to set cq+...

Comment on attachment 67213 [details] Patch proposal Clearing flags on attachment: 67213 Committed r67320: <http://trac.webkit.org/changeset/67320>

All reviewed patches have been landed. Closing bug.