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

Description Joanmarie Diggs 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)
Comment 1 Mario Sanchez Prada 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?
Comment 2 chris fleizach 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
Comment 3 Mario Sanchez Prada 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.
Comment 4 Xan Lopez 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.
Comment 5 Mario Sanchez Prada 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
Comment 6 Martin Robinson 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.
Comment 7 Mario Sanchez Prada 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.
Comment 8 Philippe Normand 2010-09-08 01:12:02 PDT
Comment on attachment 66761 [details]
Patch + unit test

Validating commit request of the r+'d patch
Comment 9 Mario Sanchez Prada 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!
Comment 10 Eric Seidel (no email) 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.
Comment 11 WebKit Commit Bot 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>
Comment 12 WebKit Commit Bot 2010-09-08 03:29:31 PDT
All reviewed patches have been landed.  Closing bug.