[Webkit-unassigned] [Bug 153085] AX: Add a boundary value to AXTextStateChangeType

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=153085

--- Comment #20 from Doug Russell <d_russell at apple.com> ---
(In reply to comment #19)
> Comment on attachment 269010 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269010&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        an editable elements boundaries.
> 
> elements -> element's

will do

> 
> > Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:385
> > +                [userInfo setObject:[NSNumber numberWithBool:intent.selection.focusChange] forKey:NSAccessibilityTextSelectionChangedFocus];
> 
> @(intent.selectin.focusChange)

will do

> 
> > Source/WebCore/editing/FrameSelection.cpp:1110
> > +        // FIXME: BIDI
> 
> do you have a WK bug to reference

I'll file one

> 
> > Source/WebCore/editing/FrameSelection.cpp:1164
> > +    return (AXTextSelection) { intentDirection, intentGranularity, false };
> 
> is the cast necessary (it may very well be, but i'm curious)

This syntax was suggested by Darin in past reviews, I'll see if the cast can be dropped

> 
> > Source/WebCore/editing/VisiblePosition.h:57
> > +        , m_isBoundary(false)
> 
> m_isBoundary should be false by default. you may not need to initialize

That is how I had it, trying this way to see if that's what was tripping up the windows build (it's one of the bigger things I changed between patches when windows stopped building)

> 
> > Source/WebCore/editing/VisiblePosition.h:60
> > +    VisiblePosition(bool boundary) : m_affinity(VP_DEFAULT_AFFINITY), m_isBoundary(boundary) { };
> 
> should this be "bool boundary = false" so you get the default behavior
> without changing code to be no?
> or should we make a static boundaryVisiblePosition() class method that
> returns the right object

If you want boundary to be false you can just call VisiblePosition(). I can do a static if you prefer but it feels odd to not use a constructor.

> 
> i know people don't generally like passing around bools where the value is
> not clear what its for

And we'd solve that with the static method?

> 
> > LayoutTests/accessibility/mac/selection-boundary-userinfo.html:8
> > +<div role="textbox" tabindex=0 id="textbox" contenteditable=true>
> 
> can you also add a test with a input type="text"

Yup. As is this patch doesn't post a boundary notification when arrowing up in a input (since they're single line controls), but I can file an enhancement for that.

> 
> > LayoutTests/accessibility/mac/selection-boundary-userinfo.html:30
> > +            	shouldBe("results[0]", "AXTextStateChangeTypeSelectionMove");
> 
> looks like this file has some tabs in it

I'll fix that.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160115/0d9ea37e/attachment.html>