Bug 113817

Summary: AX: Bounding paths should be made available through accessibility
Product: WebKit Reporter: chris fleizach <cfleizach>
Component: AccessibilityAssignee: chris fleizach <cfleizach>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, buildbot, ddkilzer, dmazzoni, jdiggs, rakuco, rniwa, thorton, webkit-ews, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
webkit-ews: commit-queue-
patch
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
none
patch ddkilzer: review+

Description chris fleizach 2013-04-02 12:17:08 PDT
See

http://examples.simplyaccessible.com/svg-maps/

The <g> elements in the SVG example are not in the AX hierarchy and the <title> field is not being exposed
Comment 1 chris fleizach 2013-04-02 12:17:23 PDT
rdar://13468267
Comment 2 chris fleizach 2013-04-05 17:43:51 PDT
Created attachment 196708 [details]
patch
Comment 3 Early Warning System Bot 2013-04-05 17:54:14 PDT
Comment on attachment 196708 [details]
patch

Attachment 196708 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17526199
Comment 4 Early Warning System Bot 2013-04-05 17:55:50 PDT
Comment on attachment 196708 [details]
patch

Attachment 196708 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17524250
Comment 5 chris fleizach 2013-04-05 17:57:04 PDT
Created attachment 196710 [details]
patch
Comment 6 Build Bot 2013-04-05 20:04:14 PDT
Comment on attachment 196710 [details]
patch

Attachment 196710 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17528276

New failing tests:
platform/mac/accessibility/element-paths.html
Comment 7 Build Bot 2013-04-05 20:04:16 PDT
Created attachment 196714 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2
Comment 8 Build Bot 2013-04-05 22:07:32 PDT
Comment on attachment 196710 [details]
patch

Attachment 196710 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17529333

New failing tests:
platform/mac/accessibility/element-paths.html
Comment 9 Build Bot 2013-04-05 22:07:35 PDT
Created attachment 196717 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.2
Comment 10 chris fleizach 2013-04-06 00:51:58 PDT
Created attachment 196718 [details]
patch
Comment 11 David Kilzer (:ddkilzer) 2013-04-09 10:01:25 PDT
Comment on attachment 196718 [details]
patch

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

r=me, but please investigate why there is a "Move to point" after "Close" on the mac/accessibility/element-paths-expected.txt test results, and future-proof the #if in AccessibilityUIElement.cpp

> Tools/DumpRenderTree/AccessibilityUIElement.cpp:1094
> +#if !PLATFORM(MAC)
> +JSStringRef AccessibilityUIElement::pathDescription() const { return 0; }
> +#endif

For future-proofing, this should be:

#if !PLATFORM(IOS) && !PLATFORM(MAC)

> LayoutTests/platform/mac/accessibility/element-paths-expected.txt:14
> +PASS svg.isAttributeSupported('AXPath') is true
> +SVG path description
> +Start Path
> +	Move to point
> +	Line to
> +	Line to
> +	Close
> +	Move to point

Would it be better to print out the point and line data here?  Or will that change from Mac-to-Mac?

Why is there a "Move to point" after "Close" here and on the two tests below?

> LayoutTests/platform/iphone-simulator/accessibility/element-paths-expected.txt:12
> +SVG path description
> +Start Path
> +	Move to point
> +	Line to
> +	Line to
> +	Close

Would it be best to have specific point and line information here?
Comment 12 chris fleizach 2013-04-09 10:56:04 PDT
(In reply to comment #11)
> (From update of attachment 196718 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=196718&action=review
> 
> r=me, but please investigate why there is a "Move to point" after "Close" on the mac/accessibility/element-paths-expected.txt test results, and future-proof the #if in AccessibilityUIElement.cpp
> 
> > Tools/DumpRenderTree/AccessibilityUIElement.cpp:1094
> > +#if !PLATFORM(MAC)
> > +JSStringRef AccessibilityUIElement::pathDescription() const { return 0; }
> > +#endif
> 
> For future-proofing, this should be:
> 
> #if !PLATFORM(IOS) && !PLATFORM(MAC)
> 
> > LayoutTests/platform/mac/accessibility/element-paths-expected.txt:14
> > +PASS svg.isAttributeSupported('AXPath') is true
> > +SVG path description
> > +Start Path
> > +	Move to point
> > +	Line to
> > +	Line to
> > +	Close
> > +	Move to point
> 
> Would it be better to print out the point and line data here?  Or will that change from Mac-to-Mac?
> 

I had that in my first go through but the EWS buildbot failed with the values I had uploaded, so it seems like it might not work out

> Why is there a "Move to point" after "Close" here and on the two tests below?
> 

I don't know. i'll look into that

> > LayoutTests/platform/iphone-simulator/accessibility/element-paths-expected.txt:12
> > +SVG path description
> > +Start Path
> > +	Move to point
> > +	Line to
> > +	Line to
> > +	Close
> 
> Would it be best to have specific point and line information here?

It's possible we can do this for the simulator. I had the same results every time I ran it, but i'm not sure if we'd get different answers on different machines
Comment 13 chris fleizach 2013-04-09 11:36:14 PDT
(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 196718 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=196718&action=review
> > 
> > r=me, but please investigate why there is a "Move to point" after "Close" on the mac/accessibility/element-paths-expected.txt test results, and future-proof the #if in AccessibilityUIElement.cpp
> > 
> > > Tools/DumpRenderTree/AccessibilityUIElement.cpp:1094
> > > +#if !PLATFORM(MAC)
> > > +JSStringRef AccessibilityUIElement::pathDescription() const { return 0; }
> > > +#endif
> > 
> > For future-proofing, this should be:
> > 
> > #if !PLATFORM(IOS) && !PLATFORM(MAC)
> > 
> > > LayoutTests/platform/mac/accessibility/element-paths-expected.txt:14
> > > +PASS svg.isAttributeSupported('AXPath') is true
> > > +SVG path description
> > > +Start Path
> > > +	Move to point
> > > +	Line to
> > > +	Line to
> > > +	Close
> > > +	Move to point
> > 
> > Would it be better to print out the point and line data here?  Or will that change from Mac-to-Mac?
> > 
> 
> I had that in my first go through but the EWS buildbot failed with the values I had uploaded, so it seems like it might not work out
> 
> > Why is there a "Move to point" after "Close" here and on the two tests below?
> > 

I looked into it. It looks like NSBezierPath appends another move to point (all the time) when you do a close subpath

> 
> I don't know. i'll look into that
> 
> > > LayoutTests/platform/iphone-simulator/accessibility/element-paths-expected.txt:12
> > > +SVG path description
> > > +Start Path
> > > +	Move to point
> > > +	Line to
> > > +	Line to
> > > +	Close
> > 
> > Would it be best to have specific point and line information here?
> 
> It's possible we can do this for the simulator. I had the same results every time I ran it, but i'm not sure if we'd get different answers on different machines
Comment 14 chris fleizach 2013-04-09 11:54:24 PDT
http://trac.webkit.org/changeset/148033
Comment 15 Tim Horton 2013-04-09 12:16:01 PDT
Comment on attachment 196718 [details]
patch

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

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1503
> +        IntPoint intPoint = (IntPoint)point;

This isn't the right way to cast to IntPoint, please use the constructor (and, you broke the build :D)
Comment 16 chris fleizach 2013-04-09 12:19:45 PDT
(In reply to comment #15)
> (From update of attachment 196718 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=196718&action=review
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1503
> > +        IntPoint intPoint = (IntPoint)point;
> 
> This isn't the right way to cast to IntPoint, please use the constructor 

Will fix.

> (and, you broke the build :D)

Which platform is broken? When i look at http://build.webkit.org/waterfall 
I don't see any failures
Comment 17 chris fleizach 2013-04-09 12:53:45 PDT
(In reply to comment #16)
> (In reply to comment #15)
> > (From update of attachment 196718 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=196718&action=review
> > 
> > > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1503
> > > +        IntPoint intPoint = (IntPoint)point;
> > 
> > This isn't the right way to cast to IntPoint, please use the constructor 
> 
> Will fix.

Addressed this feedback in

http://trac.webkit.org/changeset/148039

> 
> > (and, you broke the build :D)
> 
> Which platform is broken? When i look at http://build.webkit.org/waterfall 
> I don't see any failures
Comment 18 Tim Horton 2013-04-09 12:57:25 PDT
(In reply to comment #17)
> > 
> > > (and, you broke the build :D)
> > 
> > Which platform is broken? When i look at http://build.webkit.org/waterfall 
> > I don't see any failures

Sent you an email.
Comment 19 chris fleizach 2013-04-09 13:17:41 PDT
(In reply to comment #18)
> (In reply to comment #17)
> > > 
> > > > (and, you broke the build :D)
> > > 
> > > Which platform is broken? When i look at http://build.webkit.org/waterfall 
> > > I don't see any failures
> 
> Sent you an email.

Addressed in
http://trac.webkit.org/changeset/148042
Comment 20 Raphael Kubo da Costa (:rakuco) 2013-04-10 02:18:20 PDT
You also broke WebKitTestRunner for all non-Mac platforms :(

Fixed in <http://trac.webkit.org/changeset/148086>.