Bug 45190

Summary: [Gtk] A list item's number/bullet should not be a child of that list item
Product: WebKit Reporter: Joanmarie Diggs <jdiggs>
Component: AccessibilityAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: apinheiro, cfleizach, commit-queue, mario, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 25531    
Attachments:
Description Flags
test case
none
Patch
none
Patch + unit test none

Joanmarie Diggs
Reported 2010-09-03 11:56:28 PDT
Created attachment 66529 [details] test case Steps to reproduce: 1. Open the attached test case. 2. Use Accerciser to examine the accessible hierarchy and text. Expected results: * Bullets and numbers would not be children of the list item * Bullets and numbers would be included in the accessible text for the list item. Actual results: (the inverse of the expected results)
Attachments
test case (291 bytes, text/html)
2010-09-03 11:56 PDT, Joanmarie Diggs 		no flags
Details
Patch (4.09 KB, patch)
2010-09-07 08:57 PDT, Mario Sanchez Prada 		no flags
DetailsFormatted DiffDiff
Patch + unit test (9.61 KB, patch)
2010-09-07 13:49 PDT, Mario Sanchez Prada 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Mario Sanchez Prada
Comment 1 2010-09-07 03:25:21 PDT
I guess this changes means to change WebCore::AccessibilityRenderObject::accessibilityIsIgnored() to return *true* for those objects with roleValue() == ListMarkerRole, thus affecting all the ports. My question then is, is this change that all the ports are willing to accept? IMHO, it doesn't make sense (neither in GTK nor in any other port) to have a a11y object just fro the number/bullet and, in case that info was important, setting it along with the acecssible text should be enough. But I don't know what other software (like VO, for instance) expects in this case so any light on this topic would be welcome. Joanmarie, Chris, what do you think about this?
chris fleizach
Comment 2 2010-09-07 05:17:43 PDT
(In reply to comment #1) > I guess this changes means to change WebCore::AccessibilityRenderObject::accessibilityIsIgnored() to return *true* for those objects with roleValue() == ListMarkerRole, thus affecting all the ports. > > My question then is, is this change that all the ports are willing to accept? IMHO, it doesn't make sense (neither in GTK nor in any other port) to have a a11y object just fro the number/bullet and, in case that info was important, setting it along with the acecssible text should be enough. But I don't know what other software (like VO, for instance) expects in this case so any light on this topic would be welcome. > > Joanmarie, Chris, what do you think about this? This is expected behavior on the Mac. Please do not break
Mario Sanchez Prada
Comment 3 2010-09-07 08:57:00 PDT
Created attachment 66728 [details] Patch (In reply to comment #2) > [...] > This is expected behavior on the Mac. Please do not break Thanks for the info Chris, but at the end there's no need (fortunately) to break anything in other ports, as all the changes can be carried out for the GTK port only. I'm now attaching a patch that would fix this stuff following Joanmarie's suggestion on IRC (thanks!). Hence, now pending for review.
Xan Lopez
Comment 4 2010-09-07 10:44:09 PDT
Comment on attachment 66728 [details] Patch I think this is OK. Joanie and you should work towards writing actual layout or unit tests for this stuff instead of manual testcases for each bug. Seems a waste not to do that since she's already writing one test per bug she opens most of the time.
Mario Sanchez Prada
Comment 5 2010-09-07 13:49:58 PDT
Created attachment 66761 [details] Patch + unit test (In reply to comment #4) > (From update of attachment 66728 [details]) > I think this is OK. Joanie and you should work towards writing actual layout or unit tests for this stuff > instead of manual testcases for each bug. Seems a waste not to do that since she's already writing > one test per bug she opens most of the time. Understood. Now replacing the former patch with another one including the same code *plus* a new unit test to check everything is working as expected (inspired in Joanie's test case). Btw, if you review+ this new patch don't forget to put the commit-queue+ flag as well, since I'm not a committer and I can't commit patches on my own. Thanks for reviewing and sorry for the hassle
Martin Robinson
Comment 6 2010-09-07 21:43:58 PDT
Comment on attachment 66761 [details] Patch + unit test View in context: https://bugs.webkit.org/attachment.cgi?id=66761&action=prettypatch Looks sane.
Mario Sanchez Prada
Comment 7 2010-09-08 00:43:52 PDT
(In reply to comment #6) > (From update of attachment 66761 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=66761&action=prettypatch > > Looks sane. Could please someone put the commit-queue+ flag on this bug? I'm not a committer, so I couldn't get it it on my own. Other than that, please let me know if I'm allowed to set that flag on my own. I'm not sure about that.
Philippe Normand
Comment 8 2010-09-08 01:12:02 PDT
Comment on attachment 66761 [details] Patch + unit test Validating commit request of the r+'d patch
Mario Sanchez Prada
Comment 9 2010-09-08 01:13:59 PDT
(In reply to comment #8) > (From update of attachment 66761 [details]) > Validating commit request of the r+'d patch Thanks!
Eric Seidel (no email)
Comment 10 2010-09-08 03:16:26 PDT
Comment on attachment 66728 [details] Patch Cleared Xan Lopez's review+ from obsolete attachment 66728 [details] so that this bug does not appear in http://webkit.org/pending-commit.
WebKit Commit Bot
Comment 11 2010-09-08 03:29:26 PDT
Comment on attachment 66761 [details] Patch + unit test Clearing flags on attachment: 66761 Committed r66964: <http://trac.webkit.org/changeset/66964>
WebKit Commit Bot
Comment 12 2010-09-08 03:29:31 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.