Bug 45381 – [Gtk] Adjust atk_text_get_text_at_offset to account for bullets/numbers in list items

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

[Joanmarie Diggs]

As a result of the fix for bug #45190, the bullet/number of a list item was made part of the text. (Thanks!) It appears that a corresponding change will need to be made in atk_text_get_text_{at,before,after}_offset.

Using the original test case (https://bug-45190-attachments.webkit.org/attachment.cgi?id=66529)

Steps to reproduce:
1. Open the test case.
2. In Accerciser, select the first bulleted item.
3. In the iPython console:

  In [1]: text = acc.queryText()
  In [2]: text.getText(0, -1)
  Out[2]: '\xe2\x80\xa2 text only'
  In [3]: text.getTextAtOffset(0, TEXT_BOUNDARY_LINE_START)
  Out[3]: ('\xe2\x80\xa2 text onl', 0, 10)

[Mario Sanchez Prada]

Created attachment 67807 [details]
Patch proposal

Attaching a patch that would fix the issue.

It adds some -pretty simple yet useful- new API to WebCore::RenderListMarker and WebCore::RenderListItem to provide a way to retrieve the actual full text involved in a list item, that is, the item marker (like "1", from "1. This is an item") and the item marker's suffix (like ". " in "1. This is an item"). So far, it was possible to retrieve the marker but not the suffix, which was dinamically calculated and drawn in the paint() method of WebCore::RenderListMarker. However, as we need the suffix to be as much as accurate as possible when exposing list items to AT's through the ATK implementation (GTK), we need a way to get that suffix as well, and I thought of this one as the best way to do it.

The other part of the patch is just in AccessibilityObjectWrapperAtk.cpp (GTK port) and fixes some problems considering list markers in the AtkText interface implementation, now doing it in a more consistent way, IMHO.

Hence, now asking for review.

[WebKit Review Bot]

Attachment 67807 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1209:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mario Sanchez Prada]

Created attachment 67808 [details]
Patch proposal

Damm it... always remember to run the check-webkit-style script after attaching the patch. Sorry!

This is the patch to review then :-P

[chris fleizach]

Comment on attachment 67808 [details]
Patch proposal

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

i don't know how other people feel about adding a variable to RenderListMarker. sometimes they are quite picky about that. 

need a layout test for this however

> WebCore/ChangeLog:39
> +

indent comments when they are underneath the a filename. it makes it easier to read

> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1218
> +    RenderObject* renderer = static_cast<const AccessibilityRenderObject*>(object)->renderer();

check if renderer is nil as well

> WebCore/rendering/RenderListItem.cpp:325
> +    const String& markerTxt = m_marker->text();

var name should be markerText

[Mario Sanchez Prada]

(In reply to comment #4)
> (From update of attachment 67808 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67808&action=review
> 
> i don't know how other people feel about adding a variable to RenderListMarker. sometimes they are quite picky about that. 

Well, that's the best way I found to allow returning the marker's suffix without having to recalculate it each time. But if there's a better way to do it, I'm more than happy to change it.

> need a layout test for this however

Wouldn't it be enough with an unit test to check the marker is what it's supposed to be?

> > WebCore/ChangeLog:39
> > +
> 
> indent comments when they are underneath the a filename. it makes it easier to read

Ok

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1218
> > +    RenderObject* renderer = static_cast<const AccessibilityRenderObject*>(object)->renderer();
> 
> check if renderer is nil as well

Ok.

> > WebCore/rendering/RenderListItem.cpp:325
> > +    const String& markerTxt = m_marker->text();
> 
> var name should be markerText

Ok.

[Mario Sanchez Prada]

Created attachment 68509 [details]
Patch proposal + unit tes updated

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 67808 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=67808&action=review
> > 
> > i don't know how other people feel about adding a variable to RenderListMarker. sometimes they are quite picky about that. 
> 
> Well, that's the best way I found to allow returning the marker's suffix without having to recalculate it each time. But if there's a better way to do it, I'm more than happy to change it.

I finally found a simple way to expose the marker's suffix through a public function without having to add that variable to RenderListMarker, and it was just delegating on the internal static function listMarkerSuffix(), and then doing some small changes in the new funcion RenderListItem::markerTextWithSuffix(). And to be honest, I recognize I like it more this way, which hast the plus of not relying in the painting process to get the right value of the marker suffix.

> > need a layout test for this however
> 
> Wouldn't it be enough with an unit test to check the marker is what it's supposed to be?

I updated the unit test for ATK to ensure that the exposition of the marker is the right one. Guess this should be enough, but you (Chris) will better tell
 
> > > WebCore/ChangeLog:39
> > > +
> > 
> > indent comments when they are underneath the a filename. it makes it easier to read
> 
> Ok

Done.
 
> > > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1218
> > > +    RenderObject* renderer = static_cast<const AccessibilityRenderObject*>(object)->renderer();
> > 
> > check if renderer is nil as well
> 
> Ok.

Done.

> > > WebCore/rendering/RenderListItem.cpp:325
> > > +    const String& markerTxt = m_marker->text();
> > 
> > var name should be markerText
> 
> Ok.

Done.

[chris fleizach]

Comment on attachment 68509 [details]
Patch proposal + unit tes updated

when you get the text of an item with a list marker, should there be a space between the suffix and the actual text?
atk_text_get_text(ATK_TEXT(item3), 0, -1), ==, "3.text and a link"

otherwise this patch is looking pretty good

[Mario Sanchez Prada]

(In reply to comment #7)
> (From update of attachment 68509 [details])
> when you get the text of an item with a list marker, should there be a space between the suffix and the actual text?
> atk_text_get_text(ATK_TEXT(item3), 0, -1), ==, "3.text and a link"
> 
> otherwise this patch is looking pretty good

Yeah, actually the previous version of the patch would need that extra space in the test, as I was also returning the space that sometimes comes along with the suffix in the RenderListMarker...

But then I thought again when redoing the patch without using the suffix, and I realized that the aditional space that sometimes is added to the suffix seemed to be too dependant on the painting process (where it's decided whether to put or not that extra space), and so to return that space along with the suffix would be too much hassle, would complicate this patch too much just because of it and, after all, if I wanted to be coherent, a function RenderListMarker::suffix() should return just the suffix, not the suffix and that -sometimes used while painting- extra space.

On top of that, from the ATK world POV, I don't think this is a big deal anyway, because at least a 1-character suffix will always be there separating the marker and the item's content (if it's not a character like '.', it will be at least ' '), so I don't think it was worth messing with that kind of things.

Of course, I can be wrong... but I honestly think this is a better, cleaner and good enough implementation. But in case I was wrong, just point it out and I'd look for a reliable way to put that extra space when needed, just the same way it would be done during the execution of the paint() method.

[Mario Sanchez Prada]

(In reply to comment #8)
> (In reply to comment #7)
> [...]
> Of course, I can be wrong... but I honestly think this is a better, cleaner and good enough 
> implementation. But in case I was wrong, just point it out and I'd look for a reliable way to 
> put that extra space when needed, just the same way it would be done during the execution 
> of the paint() method.

I forgot to explicitly say that before, but I'd like to hear as well Joanie's opinion on this topic, as she probably knows better what it would be acceptable/the right thing to do in this case.

So wrapping up, Joanmarie... would it be ok to expose a list item like "1.item content here" or would it be needed to do it like "1. item content here", instead?

[Joanmarie Diggs]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > [...]
> > Of course, I can be wrong... but I honestly think this is a better, cleaner and good enough 
> > implementation. But in case I was wrong, just point it out and I'd look for a reliable way to 
> > put that extra space when needed, just the same way it would be done during the execution 
> > of the paint() method.
> 
> I forgot to explicitly say that before, but I'd like to hear as well Joanie's opinion on this topic, as she probably knows better what it would be acceptable/the right thing to do in this case.
> 
> So wrapping up, Joanmarie... would it be ok to expose a list item like "1.item content here" or would it be needed to do it like "1. item content here", instead?

Well, the goal is (of course) to present to the user what's on the screen. And what's visually on the screen looks like "1. item content here". The fact that this appearance was achieved by rendering magic other than the addition of an actual space character is, as far as the end user is concerned, irrelevant. Thus I'm afraid we do need the space added.

[Mario Sanchez Prada]

(In reply to comment #10)
> [...]
> Well, the goal is (of course) to present to the user what's on the screen. And what's visually
> on the screen looks like "1. item content here". The fact that this appearance was achieved 
> by rendering magic other than the addition of an actual space character is, as far as the end 
> user is concerned, irrelevant. Thus I'm afraid we do need the space added.

Ok. Then I'll try to come up with a patch dealing with that extra space. I've just checked what firefox does in thit case and it seems they also add that extra space, so let's follow the ff lead.

Thanks for the feedback.

[Mario Sanchez Prada]

Created attachment 68677 [details]
Patch proposal + unit test updated

(In reply to comment #11)
> (In reply to comment #10)
> > [...]
> > Well, the goal is (of course) to present to the user what's on the screen. And what's visually
> > on the screen looks like "1. item content here". The fact that this appearance was achieved 
> > by rendering magic other than the addition of an actual space character is, as far as the end 
> > user is concerned, irrelevant. Thus I'm afraid we do need the space added.
> 
> Ok. Then I'll try to come up with a patch dealing with that extra space. I've just checked what firefox does in thit case and it seems they also add that extra space, so let's follow the ff lead.
> 
> Thanks for the feedback.

Attaching a new patch that also returns the extra space, when needed, along with the marker's suffix, so calling to RenderListMarker::suffix() returns the same string that is going to be rendered when painting between the list item and the item's content.

Hence, asking for review over this new patch.

[chris fleizach]

Comment on attachment 68677 [details]
Patch proposal + unit test updated

looks ok. thanks
r=me

[Mario Sanchez Prada]

(In reply to comment #13)
> (From update of attachment 68677 [details])
> looks ok. thanks
> r=me

Thanks for the review. Could you (or some other reviewer/committer) set the cq+ flag? I forgot to set the cq? in advance.

Thanks!

[WebKit Commit Bot]

Comment on attachment 68677 [details]
Patch proposal + unit test updated

Rejecting patch 68677 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 68677]" exit_code: 2
Cleaning working directory
Updating working directory
Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2

Full output: http://queues.webkit.org/results/4002175

[Mario Sanchez Prada]

(In reply to comment #15)
> (From update of attachment 68677 [details])
> Rejecting patch 68677 from commit-queue.
> 
> Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 68677]" exit_code: 2
> Cleaning working directory
> Updating working directory
> Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
> 
> Full output: http://queues.webkit.org/results/4002175

I don't know what could have happened here, since the patch applies in my local machine perfectly on top of the very last commit.

Also it looks this issue also happened to other recent patches in the commit queue, so perhaps this is not an isolated issues, but something more global. See bugs 46621, 46538 and 46545, for instance, at the commit queue:

http://webkit-commit-queue.appspot.com/queue-status/commit-queue

If there's anything I can do from my side, don't hesitate to let me know.

[WebKit Commit Bot]

Comment on attachment 68677 [details]
Patch proposal + unit test updated

Rejecting patch 68677 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']" exit_code: 2
Last 500 characters of output:
k
All tests successful.
Files=14, Tests=304,  2 wallclock secs ( 1.13 cusr +  0.23 csys =  1.36 CPU)
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/abarth/git/webkit-queue/LayoutTests
Testing 21379 test cases.
media/invalid-media-url-crash.html -> crashed

Exiting early after 1 failures. 17591 tests run.
597.37s total testing time

17590 test cases (99%) succeeded
1 test case (<1%) crashed
29 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/4058163

[Eric Seidel (no email)]

Comment on attachment 68677 [details]
Patch proposal + unit test updated

Looks like a flaky test.

[WebKit Commit Bot]

Comment on attachment 68677 [details]
Patch proposal + unit test updated

Clearing flags on attachment: 68677

Committed r68415: <http://trac.webkit.org/changeset/68415>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/68415 might have broken Leopard Intel Debug (Tests)
The following changes are on the blame list:
http://trac.webkit.org/changeset/68416
http://trac.webkit.org/changeset/68417
http://trac.webkit.org/changeset/68415