Bug 122164 – AX: Add paramAttrs to fetch start and end text markers in a given rect.

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Samuel White]

We have several cases where it is necessary to fetch only the text markers that fall within a given rect. To support this, we need to add two parameterizedAttributes. I believe that:

AXStartTextMarkerParameterizedAttribute
AXEndTextMarkerParameterizedAttribute

will do the trick. The parameter in this case would be an NSValue wrapped rect.

[Radar WebKit Bug Importer]

<rdar://problem/15122165>

[Samuel White]

Created attachment 214791 [details]
Patch.

[Build Bot]

Comment on attachment 214791 [details]
Patch.

Attachment 214791 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/8908129

New failing tests:
platform/mac/accessibility/bounds-for-range.html

[Build Bot]

Created attachment 214799 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[EFL EWS Bot]

Comment on attachment 214791 [details]
Patch.

Attachment 214791 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/8888186

[Samuel White]

Created attachment 214803 [details]
Updated patch.

This should fix the mac platform errors. Still looking into others.

[Samuel White]

Created attachment 214804 [details]
Updated patch.

Found my mistake. Should fix remaining failures.

[EFL EWS Bot]

Comment on attachment 214804 [details]
Updated patch.

Attachment 214804 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/8898142

[Samuel White]

Created attachment 214809 [details]
Updated patch.

Missed ATK method stubs.

[Samuel White]

Is it normal to see a bot add "commit-queue-" when a failure is hit even when I haven't set any flags on a patch? Seems a bit strange.

[chris fleizach]

Comment on attachment 214809 [details]
Updated patch.

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1674
> +    return &frame->mainFrame();

seems like this should be a reference rather than pointer, so you maintain the same benefits as the reference.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1953
> +    MainFrame* mainFrame = this->mainFrame();

might want to check that rect.size() is not empty or negative in case we got a bad rect

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1954
> +    if (!mainFrame)

e.g. here you don't need to check for nil, since it's a reference only

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1957
> +    IntPoint corner = (visiblePositionForBounds == FirstVisiblePositionForBounds) ? rect.minXMinYCorner() : rect.maxXMaxYCorner();

should probably add a FIXME to handle rtl languages

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1963
> +    while (!rect.contains(position.absoluteCaretBounds().center()) && (nextPosition.isNotNull() || previousPosition.isNotNull())) {

i think this snippet can be written more efficiently

before getting next/previous, check 
   if (rect.contains(position))
        return pos;

then get next/prev and do
while (nextPosition.isNotNull() || previousPosition.isNotNull()) {
     if (rect.contains(nextPos) return position;
     if (rect.contains(prevPos) return position;


}

> Source/WebCore/accessibility/AccessibilityRenderObject.h:135
> +    

this empty line has extraneous whitespace

> Source/WebCore/accessibility/AccessibilityRenderObject.h:193
> +    virtual VisiblePosition visiblePositionForBounds(const IntRect&, AccessibilityVisiblePositionForBounds) const OVERRIDE;

this method looks like it can just go in AcccessibilityObject, and then it won't have to be virtual

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:454
> +- (IntRect)screenToContents:(const IntRect &)rect

& is on the wrong side

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:456
> +    RenderObject* renderer = m_object->renderer();

can you ask for Document* document = this->document() and get the frameView from the document?
there are a number of elements that may not have renderers, but could still get their right conversion

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:460
> +    IntPoint startPoint = renderer->view().frameView().screenToContents(rect.minXMaxYCorner());

you should cache FrameView instead of calling twice

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3274
> +        return rectSet ? [self textMarkerForVisiblePosition:m_object->visiblePositionForBounds(webCoreRect, LastVisiblePositionForBounds)] : nil;

seems like we shouldn't need to check if rectSet == anything here. we can just rely on textMarkerForVisible... to return nil or an empty text marker when we pass in an empty rect.

> Source/WebCore/ChangeLog:8
> +        Added ability to fetch end/start text marker inside a given bounds. This allows us to avoid

Your "this allows" comment seems to explain some of your marker expansion finding code rather than what we gain from fetching markers from a rect.

I think you want to say "This allows VoiceOver to retrieve the text markers for a given column" or something along that lines... that is, why are we adding this...

[Samuel White]

Created attachment 214874 [details]
Updated patch.

[Samuel White]

(In reply to comment #11)
> (From update of attachment 214809 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=214809&action=review
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1674
> > +    return &frame->mainFrame();
> 
> seems like this should be a reference rather than pointer, so you maintain the same benefits as the reference.

The issue with this approach is that some of the other objects that are necessary to fetch mainFrame may be NULL. It seems preferable to simply return a pointer that may be NULL rather than creating a sentinel value to represent an invalid mainFrame ref.

> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1953
> > +    MainFrame* mainFrame = this->mainFrame();
> 
> might want to check that rect.size() is not empty or negative in case we got a bad rect

Good point. Added.

> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1954
> > +    if (!mainFrame)
> 
> e.g. here you don't need to check for nil, since it's a reference only
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1957
> > +    IntPoint corner = (visiblePositionForBounds == FirstVisiblePositionForBounds) ? rect.minXMinYCorner() : rect.maxXMaxYCorner();
> 
> should probably add a FIXME to handle rtl languages

Added.

> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1963
> > +    while (!rect.contains(position.absoluteCaretBounds().center()) && (nextPosition.isNotNull() || previousPosition.isNotNull())) {
> 
> i think this snippet can be written more efficiently

I assume your intention was to avoid the unnecessary contains check during each loop. I've updated the stack to eliminate this check (in a slightly different way than you implied because your example isn't actually interchangeable, I assume this was more pesdo code to make the point anyways).

> 
> before getting next/previous, check 
>    if (rect.contains(position))
>         return pos;
> 
> then get next/prev and do
> while (nextPosition.isNotNull() || previousPosition.isNotNull()) {
>      if (rect.contains(nextPos) return position;
>      if (rect.contains(prevPos) return position;
> 
> 
> }
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.h:135
> > +    
> 
> this empty line has extraneous whitespace

I've been trying to leave these in hoping that over time I could slowly clean up all the legacy weird spacing issues. That is, some empty lines are indented to match their siblings, and some are not.

However, I see that it makes review tougher so I'll stop this practice and create separate bugs to do any cleanup that becomes needed down the road. Sorry about that.

> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.h:193
> > +    virtual VisiblePosition visiblePositionForBounds(const IntRect&, AccessibilityVisiblePositionForBounds) const OVERRIDE;
> 
> this method looks like it can just go in AcccessibilityObject, and then it won't have to be virtual
>

Moved. Also moved topDocument to make this possible without casts.
 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:454
> > +- (IntRect)screenToContents:(const IntRect &)rect
> 
> & is on the wrong side

Not sure what you mean here. I've pushed the ref and the type together to match the style in IntRect.h. But as far as which side the ampersand should be on, it is on the correct side now unless I'm losing my mind. I'm parsing this as "A reference to a constant thing", and since refs themselves are immutable, I'm not seeing what is wrong. Could you elaborate?

> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:456
> > +    RenderObject* renderer = m_object->renderer();
> 
> can you ask for Document* document = this->document() and get the frameView from the document?
> there are a number of elements that may not have renderers, but could still get their right conversion

Done.

> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:460
> > +    IntPoint startPoint = renderer->view().frameView().screenToContents(rect.minXMaxYCorner());
> 
> you should cache FrameView instead of calling twice

Done.

> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3274
> > +        return rectSet ? [self textMarkerForVisiblePosition:m_object->visiblePositionForBounds(webCoreRect, LastVisiblePositionForBounds)] : nil;
> 
> seems like we shouldn't need to check if rectSet == anything here. we can just rely on textMarkerForVisible... to return nil or an empty text marker when we pass in an empty rect.

Done.

> 
> > Source/WebCore/ChangeLog:8
> > +        Added ability to fetch end/start text marker inside a given bounds. This allows us to avoid
> 
> Your "this allows" comment seems to explain some of your marker expansion finding code rather than what we gain from fetching markers from a rect.
> 
> I think you want to say "This allows VoiceOver to retrieve the text markers for a given column" or something along that lines... that is, why are we adding this...

Updated.

Thanks for the feedback Chris.

[chris fleizach]

(In reply to comment #13)
> (In reply to comment #11)
> > (From update of attachment 214809 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=214809&action=review
> > 
> > > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1674
> > > +    return &frame->mainFrame();
> > 
> > seems like this should be a reference rather than pointer, so you maintain the same benefits as the reference.
> 
> The issue with this approach is that some of the other objects that are necessary to fetch mainFrame may be NULL. It seems preferable to simply return a pointer that may be NULL rather than creating a sentinel value to represent an invalid mainFrame ref.

good point

> 
> > 
> > > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1953
> > > +    MainFrame* mainFrame = this->mainFrame();
> > 
> > might want to check that rect.size() is not empty or negative in case we got a bad rect
> 
> Good point. Added.
> 
> > 
> > > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1954
> > > +    if (!mainFrame)
> > 
> > e.g. here you don't need to check for nil, since it's a reference only
> > 
> > > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1957
> > > +    IntPoint corner = (visiblePositionForBounds == FirstVisiblePositionForBounds) ? rect.minXMinYCorner() : rect.maxXMaxYCorner();
> > 
> > should probably add a FIXME to handle rtl languages
> 
> Added.
> 
> > 
> > > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1963
> > > +    while (!rect.contains(position.absoluteCaretBounds().center()) && (nextPosition.isNotNull() || previousPosition.isNotNull())) {
> > 
> > i think this snippet can be written more efficiently
> 
> I assume your intention was to avoid the unnecessary contains check during each loop. I've updated the stack to eliminate this check (in a slightly different way than you implied because your example isn't actually interchangeable, I assume this was more pesdo code to make the point anyways).
> 
> > 
> > before getting next/previous, check 
> >    if (rect.contains(position))
> >         return pos;
> > 
> > then get next/prev and do
> > while (nextPosition.isNotNull() || previousPosition.isNotNull()) {
> >      if (rect.contains(nextPos) return position;
> >      if (rect.contains(prevPos) return position;
> > 
> > 
> > }
> > 
> > > Source/WebCore/accessibility/AccessibilityRenderObject.h:135
> > > +    
> > 
> > this empty line has extraneous whitespace
> 
> I've been trying to leave these in hoping that over time I could slowly clean up all the legacy weird spacing issues. That is, some empty lines are indented to match their siblings, and some are not.
> 
> However, I see that it makes review tougher so I'll stop this practice and create separate bugs to do any cleanup that becomes needed down the road. Sorry about that.
> 
> > 
> > > Source/WebCore/accessibility/AccessibilityRenderObject.h:193
> > > +    virtual VisiblePosition visiblePositionForBounds(const IntRect&, AccessibilityVisiblePositionForBounds) const OVERRIDE;
> > 
> > this method looks like it can just go in AcccessibilityObject, and then it won't have to be virtual
> >
> 
> Moved. Also moved topDocument to make this possible without casts.
> 
> > > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:454
> > > +- (IntRect)screenToContents:(const IntRect &)rect
> > 
> > & is on the wrong side
> 
> Not sure what you mean here. I've pushed the ref and the type together to match the style in IntRect.h. But as far as which side the ampersand should be on, it is on the correct side now unless I'm losing my mind. I'm parsing this as "A reference to a constant thing", and since refs themselves are immutable, I'm not seeing what is wrong. Could you elaborate?

pretty sure it should be screenToContents:(const IntRect&)rect

> 
> > 
> > > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:456
> > > +    RenderObject* renderer = m_object->renderer();
> > 
> > can you ask for Document* document = this->document() and get the frameView from the document?
> > there are a number of elements that may not have renderers, but could still get their right conversion
> 
> Done.
> 
> > 
> > > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:460
> > > +    IntPoint startPoint = renderer->view().frameView().screenToContents(rect.minXMaxYCorner());
> > 
> > you should cache FrameView instead of calling twice
> 
> Done.
> 
> > 
> > > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3274
> > > +        return rectSet ? [self textMarkerForVisiblePosition:m_object->visiblePositionForBounds(webCoreRect, LastVisiblePositionForBounds)] : nil;
> > 
> > seems like we shouldn't need to check if rectSet == anything here. we can just rely on textMarkerForVisible... to return nil or an empty text marker when we pass in an empty rect.
> 
> Done.
> 
> > 
> > > Source/WebCore/ChangeLog:8
> > > +        Added ability to fetch end/start text marker inside a given bounds. This allows us to avoid
> > 
> > Your "this allows" comment seems to explain some of your marker expansion finding code rather than what we gain from fetching markers from a rect.
> > 
> > I think you want to say "This allows VoiceOver to retrieve the text markers for a given column" or something along that lines... that is, why are we adding this...
> 
> Updated.
> 
> Thanks for the feedback Chris.

[chris fleizach]

Comment on attachment 214874 [details]
Updated patch.

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

one minor nit

> Source/WebCore/accessibility/AccessibilityObject.cpp:955
> +    if (!rect.contains(position.absoluteCaretBounds().center())) {

can we do an early return if, to flip the !

if (rect.contains())
   return position;

then the rest of the code doesn't have to be indented.

[Samuel White]

Created attachment 214880 [details]
Updated patch.

[Samuel White]

(In reply to comment #15)
> (From update of attachment 214874 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=214874&action=review
> 
> one minor nit
> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:955
> > +    if (!rect.contains(position.absoluteCaretBounds().center())) {
> 
> can we do an early return if, to flip the !
> 
> if (rect.contains())
>    return position;
> 
> then the rest of the code doesn't have to be indented.

Done. Thanks.

[Samuel White]

I made one additional change while making the change you suggested Chris. That is, if none of the early returns are hit we return VisiblePosition() rather than position.

If the early returns don't hit, we shouldn't return our closest approximation of the end/start element (whatever position was initially set to) because it is guaranteed to NOT be inside the bounds and thus isn't valid.

[WebKit Commit Bot]

Comment on attachment 214880 [details]
Updated patch.

Rejecting attachment 214880 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Samuel White]

(In reply to comment #19)
> (From update of attachment 214880 [details])
> Rejecting attachment 214880 [details] from commit-queue.
> 
> [e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.
> 
> - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.
> 
> - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

Setting c? for this one. Will update file as directed.

[WebKit Commit Bot]

Comment on attachment 214880 [details]
Updated patch.

Clearing flags on attachment: 214880

Committed r157821: <http://trac.webkit.org/changeset/157821>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.