Bug 72811 - [Gtk] No accessible caret-moved events found in certain content
Summary: [Gtk] No accessible caret-moved events found in certain content
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL: http://www.bbc.co.uk/radio4/programme...
Keywords: Gtk
Depends on:
Blocks: 25531
  Show dependency treegraph
 
Reported: 2011-11-19 15:06 PST by Joanmarie Diggs (irc: joanie)
Modified: 2012-09-01 06:32 PDT (History)
6 users (show)

See Also:


Attachments
test script (483 bytes, text/plain)
2011-11-19 15:06 PST, Joanmarie Diggs (irc: joanie)
no flags Details
Test case (115 bytes, text/plain)
2012-08-15 14:55 PDT, Joanmarie Diggs (irc: joanie)
no flags Details
New test case (965 bytes, text/plain)
2012-08-16 13:26 PDT, Joanmarie Diggs (irc: joanie)
no flags Details
New new test case (1.12 KB, text/plain)
2012-08-16 16:30 PDT, Joanmarie Diggs (irc: joanie)
no flags Details
Proposed patch, includes updated unit test and new layout test (16.50 KB, patch)
2012-08-19 19:49 PDT, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
Just the layout only test (3.64 KB, text/plain)
2012-08-19 23:40 PDT, Joanmarie Diggs (irc: joanie)
no flags Details
Fix for the failure to implement AtkText along with updated unit test (10.67 KB, patch)
2012-08-21 15:18 PDT, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
Fix for the failure to implement AtkText along with updated unit test (11.04 KB, patch)
2012-08-21 16:17 PDT, Joanmarie Diggs (irc: joanie)
cfleizach: review+
Details | Formatted Diff | Diff
Fix for the failure to implement AtkText along with updated unit test (11.04 KB, patch)
2012-08-21 16:33 PDT, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
proposed fix - part 2 (11.25 KB, patch)
2012-08-22 22:36 PDT, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
proposed fix - part 2 (master compatible) (7.39 KB, patch)
2012-08-24 19:42 PDT, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
proposed fix - part 2 (master compatible AND with new tests git-added) (11.24 KB, patch)
2012-08-24 20:05 PDT, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
proposed fix - part 3 (includes new test) (11.59 KB, patch)
2012-08-24 20:18 PDT, Joanmarie Diggs (irc: joanie)
cfleizach: review+
Details | Formatted Diff | Diff
proposed fix - part 2 (addressed feedback from review) (11.32 KB, patch)
2012-08-27 02:21 PDT, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
proposed fix - part 2 (addressed feedback from review) (12.72 KB, patch)
2012-08-27 18:23 PDT, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
proposed fix - part 3 (addressed feedback from review) (11.89 KB, patch)
2012-08-31 00:57 PDT, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
Patch (22.25 KB, patch)
2012-09-01 02:16 PDT, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
Patch (11.48 KB, patch)
2012-09-01 02:45 PDT, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joanmarie Diggs (irc: joanie) 2011-11-19 15:06:32 PST
Created attachment 115961 [details]
test script

Steps to reproduce:

1. Launch the attached test script (or use Accerciser's event monitor)
2. View http://www.bbc.co.uk/radio4/programmes/genres/drama in epiphany
3. Enable caret navigation if it is not already enabled
4. Position the caret at "AVAILABLE NOW ON BBC IPLAYER"
5. Down Arrow

Expected results: A caret-moved event would be seen each time the caret moves.

Actual results: For every program listed, a caret-moved event is seen for all lines BUT the subtitle line (the line immediately beneath the program title).
Comment 1 Joanmarie Diggs (irc: joanie) 2012-04-09 11:18:12 PDT
Another place I'm seeing this is in GNOME's bugzilla.

1. View a bug
2. Arrow from the top of the page to the 'Collapse All Comments' link
3. Arrow left and right within that link

In steps 2 and 3, the caret moves but no corresponding accessible events are seen.
Comment 2 Joanmarie Diggs (irc: joanie) 2012-08-15 14:55:41 PDT
Created attachment 158641 [details]
Test case

Very simple test case. Note that we:

* Get no caret-moved events from the middle line.
* Get bogus offsets for the caret-moved events we are getting.
Comment 3 Joanmarie Diggs (irc: joanie) 2012-08-16 13:26:35 PDT
Created attachment 158885 [details]
New test case

Found the trigger. Finally. :-/

It seems that spans are exposed as having GroupRole (translates to ATK_ROLE_PANEL). If the span is displayed as a block element, we get caret moved events; otherwise we do not.
Comment 4 Joanmarie Diggs (irc: joanie) 2012-08-16 16:30:58 PDT
Created attachment 158937 [details]
New new test case

Found another point of failure: Wrap the breakage in a link and we get a node (unlike the other failures). However ATK_IS_TEXT fails so we bail on the signal emission.
Comment 5 Joanmarie Diggs (irc: joanie) 2012-08-19 19:49:35 PDT
Created attachment 159321 [details]
Proposed patch, includes updated unit test and new layout test
Comment 6 Joanmarie Diggs (irc: joanie) 2012-08-19 20:09:51 PDT
Mario: An informal review would be awesome since this patch is related to hierachy and the text interface. (But don't let that scare you, the bulk of the patch is test-related.)

Chris: A formal review would be awesome since this $%^#! bug boils down to Orca users navigating and Orca presenting nothing when conditions are JustRight(tm). And it turns out it is far easier than I had initially realized to cause those conditions to occur. :(

Thanks in advance to you both!
Comment 7 chris fleizach 2012-08-19 21:03:07 PDT
Comment on attachment 159321 [details]
Proposed patch, includes updated unit test and new layout test

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

> Source/WebCore/accessibility/gtk/AccessibilityObjectAtk.cpp:85
> +    if (ariaRoleAttribute() != UnknownRole || parent->ariaRoleAttribute() != UnknownRole)

It seems that just asking the parent if it's not an unknown wouldn't account for
<div><div><span>element</span></div></div>, or anything with nested unknowns. I don't see what's special about a parent having unknown role, but not a grandparent.

> Source/WebCore/accessibility/gtk/AccessibilityObjectAtk.cpp:91
> +    if ((node() && node()->hasTagName(spanTag)) || (renderObject && renderObject->isAnonymousBlock())) {

It seems weird to special case spanTag here. That seems like a level of detail that platform wrapper shouldn't have to know. 
It also seems weird that these isBody checks are inside this block. Seems like you could just check for that outside this block.

The logic change here appears to be to cause <spans> and anonymous render blocks to be ignored, but doesn't WebCore do that already?

> Source/WebCore/accessibility/gtk/WebKitAccessibleWrapperAtk.cpp:860
> +                    || (!coreObject->isControl() && !coreObject->textUnderElement().isEmpty()))

presumably this would make all kinds of things have the text interface. Tables, Lists, <body> element. Is that intended?
Also if you're checking textUnderElement(), does the renderer->childrenInline() check matter? It seems that anything that has textUnder it would be a candidate
Comment 8 Joanmarie Diggs (irc: joanie) 2012-08-19 21:22:41 PDT
(In reply to comment #7)

Thanks for the review Chris.

The short version of the story is that I wasn't checking for spans or isBody() and thought I had a ready-to-go patch, but it broke a bunch of layout tests so those changes are reactions. But I was also getting tired. So I'll make the changes you suggest and try a fresh look at the problem.

If you (and/or Mario) have suggestions about how to ensure everything that should have AtkText actually does, it would be great. Otherwise, I'll give that some more thought as well.
Comment 9 Joanmarie Diggs (irc: joanie) 2012-08-19 21:34:32 PDT
Sorry for the spam, but just noticed this:

(In reply to comment #7)
 
> The logic change here appears to be to cause <spans> and anonymous render blocks to be ignored, but doesn't WebCore do that already?

Not if you have them mixed in a single element to be included. That's the problem. I've seen cases where:

* The inline span gets ignored, the block span gets included.
* The inline span becomes a child of GroupRole, because of the block span.
* The GroupRole gets included and the parent Paragraph gets ignored.

I have a Mac Mini and should probably add build WebKit for the Mac to my TODO list. In the meantime, what do you see hierarchy-wize for the test cases?
Comment 10 Joanmarie Diggs (irc: joanie) 2012-08-19 23:40:32 PDT
Created attachment 159342 [details]
Just the layout only test

(In reply to comment #7)
 
> The logic change here appears to be to cause <spans> and anonymous render blocks to be ignored, but doesn't WebCore do that already?

The attached patch is just the original (new) layout test. I applied it to WebKit from master and ran it. What I get in the WebKitGtk port is below.

Chris: If you could do what I did and run just the new layout test, but on the Mac port, and tell me what you get and if that jives with what you would expect, that would be quite helpful. Thanks in advance!

--- /home/jd/checkout/WebKit/WebKitBuild/Release/layout-test-results/platform/gtk/accessibility/spans-expected.txt
+++ /home/jd/checkout/WebKit/WebKitBuild/Release/layout-test-results/platform/gtk/accessibility/spans-actual.txt
@@ -17,19 +17,19 @@
 
 
 PASS element.role is 'AXRole: heading'
-PASS element.childrenCount is 0
+FAIL element.childrenCount should be 0. Was 3.
 PASS element.role is 'AXRole: heading'
 PASS element.childrenCount is 1
 PASS link.role is 'AXRole: link'
-PASS link.childrenCount is 0
+FAIL link.childrenCount should be 0. Was 2.
 PASS element.role is 'AXRole: list'
 PASS element.childrenCount is 2
 PASS item.role is 'AXRole: list item'
-PASS item.childrenCount is 0
+FAIL item.childrenCount should be 0. Was 3.
 PASS item.role is 'AXRole: list item'
 PASS item.childrenCount is 1
 PASS link.role is 'AXRole: link'
-PASS link.childrenCount is 0
+FAIL link.childrenCount should be 0. Was 2.
 PASS successfullyParsed is true
 
 TEST COMPLETE
Comment 11 Mario Sanchez Prada 2012-08-20 01:07:08 PDT
Comment on attachment 159321 [details]
Proposed patch, includes updated unit test and new layout test

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

>> Source/WebCore/accessibility/gtk/AccessibilityObjectAtk.cpp:85
>> +    if (ariaRoleAttribute() != UnknownRole || parent->ariaRoleAttribute() != UnknownRole)
> 
> It seems that just asking the parent if it's not an unknown wouldn't account for
> <div><div><span>element</span></div></div>, or anything with nested unknowns. I don't see what's special about a parent having unknown role, but not a grandparent.

What about using parentObjectUnignored()? I think that would cover the issue with "nested unknowns" Chris mentioned. Not sure if it would be good enough, though

>> Source/WebCore/accessibility/gtk/AccessibilityObjectAtk.cpp:91
>> +    if ((node() && node()->hasTagName(spanTag)) || (renderObject && renderObject->isAnonymousBlock())) {
> 
> It seems weird to special case spanTag here. That seems like a level of detail that platform wrapper shouldn't have to know. 
> It also seems weird that these isBody checks are inside this block. Seems like you could just check for that outside this block.
> 
> The logic change here appears to be to cause <spans> and anonymous render blocks to be ignored, but doesn't WebCore do that already?

I agree with Chris's concerns here. Also, I wonder whether this would work as expected for those cases where divs are displayed inline (<div style="display:inline;">), which I agree are not the regular use case of HTML, but it happens some time.

Maybe there's a better way to check this (and get the same results) by checking the computed style for the render object?

> Source/WebKit/gtk/tests/testatk.c:413
> +    g_assert_cmpstr(text, ==, "Block span in a link\nInline span in a link");

You're leaking memory for text here. As atk_text_get_text returns newly allocated memory, you should ensure you call g_free before reusing that variable.

It seems the same issues was present in this function prior to this patch, though. This patch could be a good moment to fix them (I'd put them in lines 375 and 386)

> Source/WebKit/gtk/tests/testatk.c:426
> +    g_assert_cmpstr(text, ==, "Block span in a heading\nInline span in a heading");

Same leak here
Comment 12 Joanmarie Diggs (irc: joanie) 2012-08-20 17:41:34 PDT
Well, I went ahead and set up a development environment on my Mac. Here are the mac results for the same test. Ignoring the rolename diffs, the hierarchy and child count looks the same.

Chris: Please let me know if the child count is what you expect (i.e. it's a platform bug) or not what you expect (it is a WebCore bug).

Thanks!

=====================
@@ -16,20 +16,20 @@
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-PASS element.role is 'AXRole: heading'
-PASS element.childrenCount is 0
-PASS element.role is 'AXRole: heading'
+FAIL element.role should be AXRole: heading. Was AXRole: AXHeading.
+FAIL element.childrenCount should be 0. Was 3.
+FAIL element.role should be AXRole: heading. Was AXRole: AXHeading.
 PASS element.childrenCount is 1
-PASS link.role is 'AXRole: link'
-PASS link.childrenCount is 0
-PASS element.role is 'AXRole: list'
+FAIL link.role should be AXRole: link. Was AXRole: AXLink.
+FAIL link.childrenCount should be 0. Was 3.
+FAIL element.role should be AXRole: list. Was AXRole: AXList.
 PASS element.childrenCount is 2
-PASS item.role is 'AXRole: list item'
-PASS item.childrenCount is 0
-PASS item.role is 'AXRole: list item'
+FAIL item.role should be AXRole: list item. Was AXRole: AXGroup.
+FAIL item.childrenCount should be 0. Was 3.
+FAIL item.role should be AXRole: list item. Was AXRole: AXGroup.
 PASS item.childrenCount is 1
-PASS link.role is 'AXRole: link'
-PASS link.childrenCount is 0
+FAIL link.role should be AXRole: link. Was AXRole: AXLink.
+FAIL link.childrenCount should be 0. Was 3.
 PASS successfullyParsed is true
 
 TEST COMPLETE
Comment 13 chris fleizach 2012-08-20 18:44:58 PDT
It looks like the children count is different

> +FAIL element.childrenCount should be 0. Was 3.

It looks like this patch makes every <span> into an accessible element. that's sort of a big change. maybe it doesn't matter, but it will add a whole lot more elements to the tree

(In reply to comment #12)
> Well, I went ahead and set up a development environment on my Mac. Here are the mac results for the same test. Ignoring the rolename diffs, the hierarchy and child count looks the same.
> 
> Chris: Please let me know if the child count is what you expect (i.e. it's a platform bug) or not what you expect (it is a WebCore bug).
> 
> Thanks!
> 
> =====================
> @@ -16,20 +16,20 @@
>  On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> 
> 
> -PASS element.role is 'AXRole: heading'
> -PASS element.childrenCount is 0
> -PASS element.role is 'AXRole: heading'
> +FAIL element.role should be AXRole: heading. Was AXRole: AXHeading.
> +FAIL element.childrenCount should be 0. Was 3.
> +FAIL element.role should be AXRole: heading. Was AXRole: AXHeading.
>  PASS element.childrenCount is 1
> -PASS link.role is 'AXRole: link'
> -PASS link.childrenCount is 0
> -PASS element.role is 'AXRole: list'
> +FAIL link.role should be AXRole: link. Was AXRole: AXLink.
> +FAIL link.childrenCount should be 0. Was 3.
> +FAIL element.role should be AXRole: list. Was AXRole: AXList.
>  PASS element.childrenCount is 2
> -PASS item.role is 'AXRole: list item'
> -PASS item.childrenCount is 0
> -PASS item.role is 'AXRole: list item'
> +FAIL item.role should be AXRole: list item. Was AXRole: AXGroup.
> +FAIL item.childrenCount should be 0. Was 3.
> +FAIL item.role should be AXRole: list item. Was AXRole: AXGroup.
>  PASS item.childrenCount is 1
> -PASS link.role is 'AXRole: link'
> -PASS link.childrenCount is 0
> +FAIL link.role should be AXRole: link. Was AXRole: AXLink.
> +FAIL link.childrenCount should be 0. Was 3.
>  PASS successfullyParsed is true
> 
>  TEST COMPLETE
Comment 14 Joanmarie Diggs (irc: joanie) 2012-08-20 19:01:09 PDT
Chris, I think you may not be understanding me. Which is my fault for writing when tired. Lemme try again.

You said in comment #7:
> The logic change here appears to be to cause <spans> and anonymous render blocks to be ignored, but doesn't WebCore do that already?

The answer is no.

* Comment 10 shows what happens if I run the layout test from
  the proposed patch WITHOUT the code changes from the proposed
  patch. Without the code changes, I see spans and anonymous
  render blocks. I do NOT want to see spans and anonymous render
  blocks

(In reply to comment #13)
> It looks like the children count is different
> 
> > +FAIL element.childrenCount should be 0. Was 3.
> 
> It looks like this patch makes every <span> into an accessible element. that's sort of a big change. maybe it doesn't matter, but it will add a whole lot more elements to the tree

Nope. The patch with the code changes results in their being 0 children.
To get 3 children, just use the current WebKit.

* Comment 12 shows what happens if I run that same layout test in
  my shiny new Mac development environment. Using WebKit from master
  and NO code changes. And the answer is: I see the same spans. You
  seem to have essentially the same thing.

Based on your response in Comment #13:

> It looks like this patch makes every <span> into an accessible element.
> that's sort of a big change. maybe it doesn't matter, but it will add
> a whole lot more elements to the tree

1. My patch doesn't do that. The current code does.
2. I'm guessing that is something you didn't realize.
3. I'm guessing my next attempt should filter out those spans for us all.

Please confirm.

Thanks!
Comment 15 Joanmarie Diggs (irc: joanie) 2012-08-21 15:18:18 PDT
Created attachment 159777 [details]
Fix for the failure to implement AtkText along with updated unit test

This patch addresses the AtkText issue.

Given this some further thought this situation is as follows:
  * It the object is not ignored, AND
  * If the object is a paragraph, heading, div, table cell, or list item THEN
  * It should claim to implement AtkText -- regardless of whether or not it has text
  * If it doesn't actually have text, it should claim to have 0 chars and an empty string

FWIW: This is what we see from LibreOffice and it is what we see from Gecko. (Not that either can be counted upon for always doing what is right, but in this case I do think it is right.)

Therefore this patch just does a simple role check for paragraph, heading, div, and cell. The reason I did not do it for list item is listitem is that role already has role-specific code that causes it to do the right thing.

Please review. Thanks!
Comment 16 chris fleizach 2012-08-21 15:21:07 PDT
Comment on attachment 159777 [details]
Fix for the failure to implement AtkText along with updated unit test

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

> Source/WebCore/accessibility/gtk/WebKitAccessibleWrapperAtk.cpp:860
> +                    || role == ParagraphRole || role == HeadingRole || role == DivRole || role == CellRole)

having an && and then an || without parenthesis makes it ambiguous which will be interpreted first (so we should add parens around the renderer && render->childrenInline)

I also think you should have a static method like roleIsTextType(AXRole role) instead of listing out each role here
Comment 17 Joanmarie Diggs (irc: joanie) 2012-08-21 16:17:50 PDT
Created attachment 159791 [details]
Fix for the failure to implement AtkText along with updated unit test

Makes the changes specified by Chris. (Thanks!)
Comment 18 chris fleizach 2012-08-21 16:22:04 PDT
Comment on attachment 159791 [details]
Fix for the failure to implement AtkText along with updated unit test

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

> Source/WebCore/accessibility/gtk/WebKitAccessibleWrapperAtk.cpp:820
> +    return (role == ParagraphRole || role == HeadingRole || role == DivRole || role == CellRole);

i think the parens around the statement are not necessary. i'm not sure if there is a style guide entry about this
Comment 19 Joanmarie Diggs (irc: joanie) 2012-08-21 16:25:36 PDT
(In reply to comment #18)
> (From update of attachment 159791 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159791&action=review
> 
> > Source/WebCore/accessibility/gtk/WebKitAccessibleWrapperAtk.cpp:820
> > +    return (role == ParagraphRole || role == HeadingRole || role == DivRole || role == CellRole);
> 
> i think the parens around the statement are not necessary. i'm not sure if there is a style guide entry about this

  $ Tools/Scripts/check-webkit-style 
  WARNING: File exempt from style guide. Skipping:   
  "Source/WebKit/gtk/tests/testatk.c"
  Total errors found: 0 in 4 files

Would you like a new patch with the parens removed?
Comment 20 chris fleizach 2012-08-21 16:28:43 PDT
(In reply to comment #19)
> (In reply to comment #18)
> > (From update of attachment 159791 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=159791&action=review
> > 
> > > Source/WebCore/accessibility/gtk/WebKitAccessibleWrapperAtk.cpp:820
> > > +    return (role == ParagraphRole || role == HeadingRole || role == DivRole || role == CellRole);
> > 
> > i think the parens around the statement are not necessary. i'm not sure if there is a style guide entry about this
> 
>   $ Tools/Scripts/check-webkit-style 
>   WARNING: File exempt from style guide. Skipping:   
>   "Source/WebKit/gtk/tests/testatk.c"
>   Total errors found: 0 in 4 files
> 
> Would you like a new patch with the parens removed?

Yes. Thank you
Comment 21 Joanmarie Diggs (irc: joanie) 2012-08-21 16:33:27 PDT
Created attachment 159799 [details]
Fix for the failure to implement AtkText along with updated unit test

(In reply to comment #20)

> > Would you like a new patch with the parens removed?
> 
> Yes. Thank you

Here you go. Thanks again!
Comment 22 WebKit Review Bot 2012-08-21 18:36:40 PDT
Comment on attachment 159799 [details]
Fix for the failure to implement AtkText along with updated unit test

Clearing flags on attachment: 159799

Committed r126243: <http://trac.webkit.org/changeset/126243>
Comment 23 WebKit Review Bot 2012-08-21 18:36:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Joanmarie Diggs (irc: joanie) 2012-08-21 18:49:10 PDT
(Reopening because I separated the problems. The AtkText part is solved. What remains is the extra children in the hierarchy for WebCore a11y)
Comment 25 Joanmarie Diggs (irc: joanie) 2012-08-22 22:36:46 PDT
Created attachment 160086 [details]
proposed fix - part 2

* Ignore inline spans in WebCore a11y
* Ignore anonymous blocks in Gtk platform a11y
* Treat list items as text role
* New layout test
* Corrected layout test (mismatched tags discovered as a result of this fix)
* Updated layout test (remove extraneous and now-ignored GroupRole object)
Comment 26 Joanmarie Diggs (irc: joanie) 2012-08-24 19:42:55 PDT
Created attachment 160543 [details]
proposed fix - part 2 (master compatible)
Comment 27 Joanmarie Diggs (irc: joanie) 2012-08-24 19:48:48 PDT
Comment on attachment 160543 [details]
proposed fix - part 2 (master compatible)

*sobs* forgot to re-git-add the new tests
Comment 28 Joanmarie Diggs (irc: joanie) 2012-08-24 20:05:51 PDT
Created attachment 160549 [details]
proposed fix - part 2 (master compatible AND with new tests git-added)
Comment 29 Joanmarie Diggs (irc: joanie) 2012-08-24 20:18:08 PDT
Created attachment 160550 [details]
proposed fix - part 3 (includes new test)

Yet another aspect of this bug is expected accessible objects of DivRole and ParagraphRole being ignored, in favor of including child blocks.

N.B. this patch's test assume/rely in part upon the Part 2 fix.
Comment 30 Joanmarie Diggs (irc: joanie) 2012-08-24 21:45:08 PDT
Comment on attachment 160549 [details]
proposed fix - part 2 (master compatible AND with new tests git-added)

EWS is green, so....

Chris, when you get a chance could you please review this? Thanks in advance.
Comment 31 Joanmarie Diggs (irc: joanie) 2012-08-24 21:48:14 PDT
Comment on attachment 160550 [details]
proposed fix - part 3 (includes new test)

EWS is NOT green, but.... This patch assumes the part 2 patch and the EWS failure is due to this patch not applying without the previous one having been first applied. So...

Chris, if you have a little extra time after reviewing part 2, could you please also review this one? Thanks in advance.
Comment 32 chris fleizach 2012-08-26 21:15:35 PDT
Comment on attachment 159799 [details]
Fix for the failure to implement AtkText along with updated unit test

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

> Source/WebKit/gtk/tests/testatk.c:351
> +    g_free (text);

extra space after g_free

> Source/WebKit/gtk/tests/testatk.c:357
> +    g_free (text);

extra space after g_free
Comment 33 chris fleizach 2012-08-26 21:16:50 PDT
Comment on attachment 160549 [details]
proposed fix - part 2 (master compatible AND with new tests git-added)

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1956
> +    if (node && node->hasTagName(spanTag))

we should have a comment here as to why all span tags need to be ignored
Comment 34 Joanmarie Diggs (irc: joanie) 2012-08-27 02:21:28 PDT
Created attachment 160678 [details]
proposed fix - part 2 (addressed feedback from review)

(In reply to comment #33)
> (From update of attachment 160549 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160549&action=review
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1956
> > +    if (node && node->hasTagName(spanTag))
> 
> we should have a comment here as to why all span tags need to be ignored

Done.
Comment 35 chris fleizach 2012-08-27 16:41:27 PDT
Comment on attachment 160678 [details]
proposed fix - part 2 (addressed feedback from review)

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1956
> +    // Content inside span tags gets included in the parent text object.

I don't feel like this comment is adequate to explain why they're ignored at this point. 

If I had to guess it's because 

"<span> tags are inline tags and not meant to convey information if they have no other aria information on them. If we don't ignore them, then they can cause other elements to be ignored because...."

> Source/WebCore/accessibility/gtk/AccessibilityObjectAtk.cpp:83
> +        && parent->ariaRoleAttribute() == UnknownRole)

this seems like very specific logic. Generally we don't make inclusion decisions based on what the parent is doing. can you add a better comment here why an anonymous block, who's parent is not the body, and who's parent is not an unknown role, is ignored?

also, why does it only matter for the parent's aria role but not it's normal role?

> Source/WebCore/accessibility/gtk/WebKitAccessibleWrapperAtk.cpp:822
> +    return role == ParagraphRole || role == HeadingRole || role == DivRole || role == CellRole || role == ListItemRole;

this patch here seems like a separate issue from the rest of this patch
Comment 36 Joanmarie Diggs (irc: joanie) 2012-08-27 18:23:27 PDT
Created attachment 160878 [details]
proposed fix - part 2 (addressed feedback from review)

(In reply to comment #35)

Thanks Chris! I fear this time I may have gone too far in the other direction (too long a comment), but I hope not.
 
> If I had to guess it's because 
> 
> "<span> tags are inline tags and not meant to convey information if they have no other aria information on them. If we don't ignore them, then they can cause other elements to be ignored because...."

(They also cause events to be emitted from the wrong object.)

Both explained.

> > Source/WebCore/accessibility/gtk/AccessibilityObjectAtk.cpp:83
> > +        && parent->ariaRoleAttribute() == UnknownRole)
> 
> this seems like very specific logic. Generally we don't make inclusion decisions based on what the parent is doing. can you add a better comment here why an anonymous block, who's parent is not the body, and who's parent is not an unknown role, is ignored?
> 
> also, why does it only matter for the parent's aria role but not it's normal role?

Bottom line is being conservative and going with the default behavior for now. But I detailed the areas.
 
> > Source/WebCore/accessibility/gtk/WebKitAccessibleWrapperAtk.cpp:822
> > +    return role == ParagraphRole || role == HeadingRole || role == DivRole || role == CellRole || role == ListItemRole;
> 
> this patch here seems like a separate issue from the rest of this patch

It does seem so on the surface. But it turns out that without that change, this patch introduces a regression in one of the unit tests. As I mentioned in comment 15, while list item is an always-expect-AtkText role I had not explicitly included it because we already had role-specific code handling that situation. Turned out the handling was based on an unwanted, unexpected GroupRole object. That is my mistake, for which I apologize.
Comment 37 Joanmarie Diggs (irc: joanie) 2012-08-30 13:57:48 PDT
Chris: Ping?

Are you crazy-busy, or do you want things done differently on the part 2 and/or part 3 patches that I still need to do? If the former, I understand completely and am sorry. If the latter, please let me know what they are and/or if there are bits you are fine with so that I can do a new patch with just those bits.

This particular bug results has significant impact for Orca users and I am really hoping it can be fixed in time for the GNOME 3.6 release. (Next webkitgtk release is Monday and we're nearly at code freeze.)

Sorry for being a noodge.
Comment 38 chris fleizach 2012-08-30 21:30:13 PDT
Comment on attachment 160878 [details]
proposed fix - part 2 (addressed feedback from review)

thanks for updates to comments
Comment 39 Joanmarie Diggs (irc: joanie) 2012-08-30 21:31:42 PDT
Comment on attachment 160878 [details]
proposed fix - part 2 (addressed feedback from review)

Thanks for the review!
Comment 40 chris fleizach 2012-08-30 21:33:18 PDT
Comment on attachment 160550 [details]
proposed fix - part 3 (includes new test)

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

looks ok otherwise

> Source/WebCore/accessibility/gtk/AccessibilityObjectAtk.cpp:82
> +    // ignores the paragraph or div and includes the block. We want the opposite.

it might be worth noting why GTK wants to do the opposite (since there's no link back to which bug this is fixing)
Comment 41 Joanmarie Diggs (irc: joanie) 2012-08-31 00:57:39 PDT
Created attachment 161630 [details]
proposed fix - part 3 (addressed feedback from review)

(In reply to comment #40)
> (From update of attachment 160550 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160550&action=review
> 
> looks ok otherwise
> 
> > Source/WebCore/accessibility/gtk/AccessibilityObjectAtk.cpp:82
> > +    // ignores the paragraph or div and includes the block. We want the opposite.
> 
> it might be worth noting why GTK wants to do the opposite (since there's no link back to which bug this is fixing)

Done. Thanks for the review!

r?ing and cq?ing. But note that this patch expects/depends upon the "Part 2" patch, so I fully expect the EWS to spit up on this.
Comment 42 WebKit Review Bot 2012-09-01 01:30:18 PDT
Comment on attachment 160878 [details]
proposed fix - part 2 (addressed feedback from review)

Rejecting attachment 160878 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
.cpp
Hunk #1 FAILED at 1953.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/accessibility/AccessibilityRenderObject.cpp.rej
patching file Source/WebCore/accessibility/gtk/AccessibilityObjectAtk.cpp
patching file Source/WebCore/accessibility/gtk/WebKitAccessibleWrapperAtk.cpp
Hunk #1 succeeded at 820 (offset 1 line).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Chris Flei..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/13724481
Comment 43 Zan Dobersek 2012-09-01 01:53:58 PDT
(In reply to comment #42)
> (From update of attachment 160878 [details])
> Rejecting attachment 160878 [details] from commit-queue.
> 
> Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2
> 
> Last 500 characters of output:
> .cpp
> Hunk #1 FAILED at 1953.
> 1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/accessibility/AccessibilityRenderObject.cpp.rej
> patching file Source/WebCore/accessibility/gtk/AccessibilityObjectAtk.cpp
> patching file Source/WebCore/accessibility/gtk/WebKitAccessibleWrapperAtk.cpp
> Hunk #1 succeeded at 820 (offset 1 line).
> 
> Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Chris Flei..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue
> 
> Full output: http://queues.webkit.org/results/13724481

Rebased on ToT and landed in r127368:
http://trac.webkit.org/changeset/127368
Comment 44 Joanmarie Diggs (irc: joanie) 2012-09-01 02:16:12 PDT
Created attachment 161820 [details]
Patch
Comment 45 Joanmarie Diggs (irc: joanie) 2012-09-01 02:45:25 PDT
Created attachment 161821 [details]
Patch
Comment 46 WebKit Review Bot 2012-09-01 03:51:01 PDT
Comment on attachment 161821 [details]
Patch

Clearing flags on attachment: 161821

Committed r127370: <http://trac.webkit.org/changeset/127370>
Comment 47 WebKit Review Bot 2012-09-01 03:51:07 PDT
All reviewed patches have been landed.  Closing bug.
Comment 48 Joanmarie Diggs (irc: joanie) 2012-09-01 06:32:14 PDT
(For reference in case gardeners come looking here)

I'm seeing the need for just... one... more.... tweak to the replaced-objects-in-anonymous-blocks.html layout test. I opened bug 95643.