72811 – [Gtk] No accessible caret-moved events found in certain content

Joanmarie Diggs

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

Joanmarie Diggs

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

Joanmarie Diggs

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

Joanmarie Diggs

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

Joanmarie Diggs

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

Joanmarie Diggs

Comment 5 2012-08-19 19:49:35 PDT

Created attachment 159321 [details] Proposed patch, includes updated unit test and new layout test

Joanmarie Diggs

Comment 6 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!

chris fleizach

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

Joanmarie Diggs

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

Joanmarie Diggs

Comment 9 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?

Joanmarie Diggs

Comment 10 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

Mario Sanchez Prada

Comment 11 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

Joanmarie Diggs

Comment 12 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

chris fleizach

Comment 13 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

Joanmarie Diggs

Comment 14 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!

Joanmarie Diggs

Comment 15 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!

chris fleizach

Comment 16 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

Joanmarie Diggs

Comment 17 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!)

chris fleizach

Comment 18 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

Joanmarie Diggs

Comment 19 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?

chris fleizach

Comment 20 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

Joanmarie Diggs

Comment 21 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!

WebKit Review Bot

Comment 22 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>

WebKit Review Bot

Comment 23 2012-08-21 18:36:44 PDT

All reviewed patches have been landed. Closing bug.

Joanmarie Diggs

Comment 24 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)

Joanmarie Diggs

Comment 25 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)

Joanmarie Diggs

Comment 26 2012-08-24 19:42:55 PDT

Created attachment 160543 [details] proposed fix - part 2 (master compatible)

Joanmarie Diggs

Comment 27 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

Joanmarie Diggs

Comment 28 2012-08-24 20:05:51 PDT

Created attachment 160549 [details] proposed fix - part 2 (master compatible AND with new tests git-added)

Joanmarie Diggs

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

Joanmarie Diggs

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

Joanmarie Diggs

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

chris fleizach

Comment 32 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

chris fleizach

Comment 33 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

Joanmarie Diggs

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

chris fleizach

Comment 35 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

Joanmarie Diggs

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

Joanmarie Diggs

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

chris fleizach

Comment 38 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

Joanmarie Diggs

Comment 39 2012-08-30 21:31:42 PDT

Comment on attachment 160878 [details] proposed fix - part 2 (addressed feedback from review) Thanks for the review!

chris fleizach

Comment 40 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)

Joanmarie Diggs

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

WebKit Review Bot

Comment 42 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

Zan Dobersek

Comment 43 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

Joanmarie Diggs

Comment 44 2012-09-01 02:16:12 PDT

Created attachment 161820 [details] Patch

Joanmarie Diggs

Comment 45 2012-09-01 02:45:25 PDT

Created attachment 161821 [details] Patch

WebKit Review Bot

Comment 46 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>

WebKit Review Bot

Comment 47 2012-09-01 03:51:07 PDT

All reviewed patches have been landed. Closing bug.

Joanmarie Diggs

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

Attachments
test script (483 bytes, text/plain) 2011-11-19 15:06 PST, Joanmarie Diggs	no flags	Details
Test case (115 bytes, text/plain) 2012-08-15 14:55 PDT, Joanmarie Diggs	no flags	Details
New test case (965 bytes, text/plain) 2012-08-16 13:26 PDT, Joanmarie Diggs	no flags	Details
New new test case (1.12 KB, text/plain) 2012-08-16 16:30 PDT, Joanmarie Diggs	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	no flags	Details Formatted Diff Diff
Just the layout only test (3.64 KB, text/plain) 2012-08-19 23:40 PDT, Joanmarie Diggs	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	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	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	no flags	Details Formatted Diff Diff
proposed fix - part 2 (11.25 KB, patch) 2012-08-22 22:36 PDT, Joanmarie Diggs	no flags	Details Formatted Diff Diff
proposed fix - part 2 (master compatible) (7.39 KB, patch) 2012-08-24 19:42 PDT, Joanmarie Diggs	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	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	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	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	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	no flags	Details Formatted Diff Diff
Patch (22.25 KB, patch) 2012-09-01 02:16 PDT, Joanmarie Diggs	no flags	Details Formatted Diff Diff
Patch (11.48 KB, patch) 2012-09-01 02:45 PDT, Joanmarie Diggs	no flags	Details Formatted Diff Diff
Show Obsolete (15) View All Add attachment proposed patch, testcase, etc.