posinset and setsize for input type, radio, exposed in AX tree

posinset and setsize for input type, radio, exposed in AX tree

posinset and setsize for radio input should be index and size of group
and exposed as object attributes.
This patch introduces AXRadioInput class to handle input element with
radio type.

BUG=557041
Committed: https://crrev.com/c52244a6cfc56abce6411a2d82b31eaa06167ec3
Cr-Commit-Position: refs/heads/master@{#379445}

[je_julie(Not used), 2016-01-25 15:12:43]

je_julie.kim@samsung.com changed reviewers:
+ dmazzoni@chromium.org, keishi@chromium.org

[je_julie(Not used), 2016-01-25 15:12:44]

Dominic and keishi,

This CL adds new APIs at RadioGroup in order to expose index and size of radio
group for AX tree.
PTAL.

[dmazzoni, 2016-01-25 18:21:09]

This looks great, but could you include some test cases that use aria-posinset
and aria-setsize on the radio buttons?

The existing tests don't really exercise ARIA much - i.e. there's a listbox with
4 items, but aria-setsize=4. It'd be more interesting if it had aria-setsize=10
and the first item had aria-posinset=3, for example.

[keishi, 2016-01-26 01:32:24]

https://codereview.chromium.org/1628283002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/forms/RadioButtonGroupScope.cpp
(right):

https://codereview.chromium.org/1628283002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/forms/RadioButtonGroupScope.cpp:41: unsigned
posInMembers(HTMLInputElement*) const;
nit: Blink style avoid abbreviations so positionInMembers might be better.

https://codereview.chromium.org/1628283002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/forms/RadioButtonGroupScope.cpp:52:
WillBeHeapVector<RawPtrWillBeMember<HTMLInputElement>> m_members;
This change will conflict with https://codereview.chromium.org/1632493002/

[je_julie(Not used), 2016-02-01 15:25:24]

Patchset #2 (id:20001) has been deleted

[je_julie(Not used), 2016-02-01 15:37:39]

Description was changed from

==========
posinset and setsize for input type, radio, exposed in AX tree

posinset and setsize for radio input should be index and size of group
and exposed as object attributes.
This patch makes html input have posInRadioGroup() and sizeOfRadioGroup()
and updates m_members from RadioButtonGroup with Vector as Hashset can't
keep order.

BUG=557041
==========

to

==========
posinset and setsize for input type, radio, exposed in AX tree

posinset and setsize for radio input should be index and size of group
and exposed as object attributes.
This patch makes html input have posInRadioGroup() and sizeOfRadioGroup()
and adds RadioButtonGroup::updatePosition() to get position.

BUG=557041
==========

[je_julie(Not used), 2016-02-02 00:36:17]

Dominic and keishi,

I updated TC and rebased code.
As Radio Group doesn't have information about ordered list,
I added m_needToUpdateIndex at RadioButtonGroup and it is set as true whenever
radio buttons are added and removed.
If it is true, it updates position from html input element before getting
HTMLInputElement::positionInRadioGroup() by AX.
PTAL.

https://codereview.chromium.org/1628283002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/forms/RadioButtonGroupScope.cpp
(right):

https://codereview.chromium.org/1628283002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/forms/RadioButtonGroupScope.cpp:41: unsigned
posInMembers(HTMLInputElement*) const;
On 2016/01/26 01:32:23, keishi wrote:
> nit: Blink style avoid abbreviations so positionInMembers might be better.

Done

https://codereview.chromium.org/1628283002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/forms/RadioButtonGroupScope.cpp:52:
WillBeHeapVector<RawPtrWillBeMember<HTMLInputElement>> m_members;
On 2016/01/26 01:32:23, keishi wrote:
> This change will conflict with https://codereview.chromium.org/1632493002/

I rebased code on that CL.

[dmazzoni, 2016-02-03 16:41:44]

lgtm

[je_julie(Not used), 2016-02-05 00:38:18]

On 2016/02/03 16:41:44, dmazzoni wrote:
> lgtm

keishi, could you take a look?

[keishi, 2016-02-05 03:05:26]

https://codereview.chromium.org/1628283002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/forms/RadioButtonGroupScope.cpp
(right):

https://codereview.chromium.org/1628283002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/RadioButtonGroupScope.cpp:52: bool
isNext(HTMLInputElement* list, HTMLInputElement* current) const;
nit: Could you make the method name more specific so its easier to tell that
this is comparing the layout position, not dom order.

https://codereview.chromium.org/1628283002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/RadioButtonGroupScope.cpp:211: if
(!m_needToUpdateIndex)
List order seems to rely on layout.
Does m_needToUpdateIndex need to be true whenever CSS updates the position of
the radio group?

https://codereview.chromium.org/1628283002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/RadioButtonGroupScope.cpp:247: if
(listRect.y() == currentRect.y() && listRect.x() <= currentRect.x())
Just curious. Is this ok for rtl?

[je_julie(Not used), 2016-02-12 16:03:54]

Patchset #4 (id:80001) has been deleted

[je_julie(Not used), 2016-02-12 16:04:37]

Patchset #3 (id:60001) has been deleted

[je_julie(Not used), 2016-02-13 02:04:54]

Patchset #3 (id:100001) has been deleted

[je_julie(Not used), 2016-02-13 03:19:00]

keishi,

Thanks for your comment, I checked RTL case and additional layout-ing case.
I considered several ways to handle updating index by additional layout.
1) Adding AXRadioButton class and keeping radio button information to avoid
giving any effect to other components.
=> Because AX module can know whenever occurring layout, Adding AXRadioButton
class is reasonable but I concluded that it causes redundant code and requires
addition storage because it should know radio group information as well.
As RadioButtonGroup is local class, it should be updated with public class to be
used in AX module. If it’s not exposed, RadioButtonGroup should has code for
handling AX.
So, I thought RadioButtonGroup doesn’t need to be public and it’s not good to
have AX code in RadioButtonGroup class.
2) Adds updating radio button index at layout.
=> At first, I tried not to add updating index at layout code because it doesn’t
seem natural but just HTMLInputElement has RadioGroup information, we need to
update index according to lay outing and I also found HTMLDialogElement is
updated at layout. So, I thought it could be natural and I chose it. 
WDYT?

https://codereview.chromium.org/1628283002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/forms/RadioButtonGroupScope.cpp
(right):

https://codereview.chromium.org/1628283002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/RadioButtonGroupScope.cpp:52: bool
isNext(HTMLInputElement* list, HTMLInputElement* current) const;
On 2016/02/05 03:05:26, keishi wrote:
> nit: Could you make the method name more specific so its easier to tell that
> this is comparing the layout position, not dom order.

I updated it with isNextPosition().

https://codereview.chromium.org/1628283002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/RadioButtonGroupScope.cpp:211: if
(!m_needToUpdateIndex)
On 2016/02/05 03:05:26, keishi wrote:
> List order seems to rely on layout.
> Does m_needToUpdateIndex need to be true whenever CSS updates the position of
> the radio group?

I also set this flag in LayoutBlockFlow::layoutBlock().

https://codereview.chromium.org/1628283002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/RadioButtonGroupScope.cpp:247: if
(listRect.y() == currentRect.y() && listRect.x() <= currentRect.x())
On 2016/02/05 03:05:26, keishi wrote:
> Just curious. Is this ok for rtl?

Right. We have to consider RTL case as well. I updated it.

[je_julie(Not used), 2016-02-16 00:45:09]

keishi, could you take another look?

[keishi, 2016-02-16 02:22:47]

Sorry, I'm not familiar with aria, but does posinset really need to return the
index based on visual position? We don't do that for <li> or the other roles,
right?

https://codereview.chromium.org/1628283002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/1628283002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLInputElement.cpp:1768:
scope->needUpdatePositionGroup(this);
Only update if accessibilty is enabled?

https://codereview.chromium.org/1628283002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLInputElement.h (right):

https://codereview.chromium.org/1628283002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLInputElement.h:385: unsigned
m_positonInRadio;
I think its ok to have code/API to support accedssibility features in core/. But
could we store the data in the accessibility module (ie  somewhere like
AXRadioButton). Maybe through something like
AXObjectCache::handlePositionInSetChange. This is only used for accessibility.

https://codereview.chromium.org/1628283002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/1628283002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:324: if
(isHTMLInputElement(node()) && toHTMLInputElement(*node()).type() ==
InputTypeNames::radio)
We try to avoid the "if (input.type==foo) input.bar()" pattern using the
InputType class.

[je_julie(Not used), 2016-02-16 05:19:03]

Hi keishi,

Thanks for your comment.
I was still hesitating between 2 ways, adding AXRadiobutton class or using
current code.
As getting your feedback, I'm convinced that I'd better adding AXRadioButton.
I'll try to make it more clear on next patch. 

"Sorry, I'm not familiar with aria, but does posinset really need to return the
index based on visual position? We don't do that for <li> or the other roles,
right?"
=> https://www.w3.org/TR/wai-aria-1.1/#aria-posinset
According to the spec above, it's based on position. Generally we can say that
it's same as node orders but node order doesn't match with position when node is
created by JS dynamically. If we think it from a point of blind people, I
believe they expect that screen reader reads it according to visual position.

I'll upload new patch soon.
Thanks,

https://codereview.chromium.org/1628283002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/1628283002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLInputElement.cpp:1768:
scope->needUpdatePositionGroup(this);
On 2016/02/16 02:22:46, keishi wrote:
> Only update if accessibilty is enabled?

So far, there is no module which needs radio button index except Accessibility
because normal people don't need radio button index on current spec and web
behavior. So, I'll try to update code more specific to accessibility with
AXRadioButton class on next patch.

https://codereview.chromium.org/1628283002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLInputElement.h (right):

https://codereview.chromium.org/1628283002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLInputElement.h:385: unsigned
m_positonInRadio;
On 2016/02/16 02:22:46, keishi wrote:
> I think its ok to have code/API to support accedssibility features in core/.
But
> could we store the data in the accessibility module (ie  somewhere like
> AXRadioButton). Maybe through something like
> AXObjectCache::handlePositionInSetChange. This is only used for accessibility.

Right. It'd better adding new AXRadioButton class to handle it.

https://codereview.chromium.org/1628283002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/1628283002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:324: if
(isHTMLInputElement(node()) && toHTMLInputElement(*node()).type() ==
InputTypeNames::radio)
On 2016/02/16 02:22:47, keishi wrote:
> We try to avoid the "if (input.type==foo) input.bar()" pattern using the
> InputType class.

OK. I'll update it on next patch.

[keishi, 2016-02-16 05:45:33]

On 2016/02/16 05:19:03, je_julie wrote:
> Hi keishi,
> 
> Thanks for your comment.
> I was still hesitating between 2 ways, adding AXRadiobutton class or using
> current code.
> As getting your feedback, I'm convinced that I'd better adding AXRadioButton.
> I'll try to make it more clear on next patch. 
> 
> "Sorry, I'm not familiar with aria, but does posinset really need to return
the
> index based on visual position? We don't do that for <li> or the other roles,
> right?"
> => https://www.w3.org/TR/wai-aria-1.1/#aria-posinset
> According to the spec above, it's based on position. Generally we can say that
> it's same as node orders but node order doesn't match with position when node
is
> created by JS dynamically. If we think it from a point of blind people, I
> believe they expect that screen reader reads it according to visual position.
> 
> I'll upload new patch soon.
> Thanks,
The spec says "position in the current set" so doesn't that "position" means an
index number, not a 2d coordinate. It doesn't say what the order should be.
I understand that sorting based on visual position is the ideal behavior, but we
often compromise and use dom order, for example spatial navigation.
Visual order is hard. What about vertical text using CSS writing-mode? Shouldn't
we be doing this for all elements that support posinset?

+dmazzoni could you confirm that this is indeed what we want?

[je_julie(Not used), 2016-02-17 02:14:34]

On 2016/02/16 05:45:33, keishi wrote:
> On 2016/02/16 05:19:03, je_julie wrote:
> > => https://www.w3.org/TR/wai-aria-1.1/#aria-posinset
> > According to the spec above, it's based on position. Generally we can say
that
> > it's same as node orders but node order doesn't match with position when
node
> is
> > created by JS dynamically. If we think it from a point of blind people, I
> > believe they expect that screen reader reads it according to visual
position.
> > 
> The spec says "position in the current set" so doesn't that "position" means
an
> index number, not a 2d coordinate. It doesn't say what the order should be.
> I understand that sorting based on visual position is the ideal behavior, but
we
> often compromise and use dom order, for example spatial navigation.
Could you describe more detail how to compromise and use dom order at spatial
navigation?

> Visual order is hard. What about vertical text using CSS writing-mode?
Shouldn't
I agree it's hard to handle with visual order.

> we be doing this for all elements that support posinset?
There are some other elements except radio button but they are already handled
with AX tree order.
Currently Radiobuttons are also handled with AX tree but it's not working
correctly
because AX tree doesn't care it with RadioGroup Information.
> 
> +dmazzoni could you confirm that this is indeed what we want?
Dominic, WDYT?

[keishi, 2016-02-17 02:25:10]

On 2016/02/17 02:14:34, je_julie wrote:
> On 2016/02/16 05:45:33, keishi wrote:
> > On 2016/02/16 05:19:03, je_julie wrote:
> > > => https://www.w3.org/TR/wai-aria-1.1/#aria-posinset
> > > According to the spec above, it's based on position. Generally we can say
> that
> > > it's same as node orders but node order doesn't match with position when
> node
> > is
> > > created by JS dynamically. If we think it from a point of blind people, I
> > > believe they expect that screen reader reads it according to visual
> position.
> > > 
> > The spec says "position in the current set" so doesn't that "position" means
> an
> > index number, not a 2d coordinate. It doesn't say what the order should be.
> > I understand that sorting based on visual position is the ideal behavior,
but
> we
> > often compromise and use dom order, for example spatial navigation.
> Could you describe more detail how to compromise and use dom order at spatial
> navigation?
Spatial navigation is a feature of Blink where you can control the focus using
arrow keys (if I remember correctly used for browsers on TV?)
I know that at least for radio button groups and <select> options we use dom
order.
> > Visual order is hard. What about vertical text using CSS writing-mode?
> Shouldn't
> I agree it's hard to handle with visual order.
> 
> > we be doing this for all elements that support posinset?
> There are some other elements except radio button but they are already handled
> with AX tree order.
> Currently Radiobuttons are also handled with AX tree but it's not working
> correctly
> because AX tree doesn't care it with RadioGroup Information.
AX tree order, isn't that roughly dom order with extra nodes?
If we need to do visual order I'm hoping there's a more generic, official way to
do it.

[je_julie(Not used), 2016-02-17 02:52:35]

On 2016/02/17 02:25:10, keishi wrote:
> On 2016/02/17 02:14:34, je_julie wrote:
> > On 2016/02/16 05:45:33, keishi wrote:
> > > On 2016/02/16 05:19:03, je_julie wrote:
> > > > => https://www.w3.org/TR/wai-aria-1.1/#aria-posinset
> > > > According to the spec above, it's based on position. Generally we can
say
> > that
> > > > it's same as node orders but node order doesn't match with position when
> > node
> > > is
> > > > created by JS dynamically. If we think it from a point of blind people,
I
> > > > believe they expect that screen reader reads it according to visual
> > position.
> > > > 
> > > The spec says "position in the current set" so doesn't that "position"
means
> > an
> > > index number, not a 2d coordinate. It doesn't say what the order should
be.
> > > I understand that sorting based on visual position is the ideal behavior,
> but
> > we
> > > often compromise and use dom order, for example spatial navigation.
> > Could you describe more detail how to compromise and use dom order at
spatial
> > navigation?
> Spatial navigation is a feature of Blink where you can control the focus using
> arrow keys (if I remember correctly used for browsers on TV?)
> I know that at least for radio button groups and <select> options we use dom
> order.
> > > Visual order is hard. What about vertical text using CSS writing-mode?
> > Shouldn't
> > I agree it's hard to handle with visual order.
> > 
> > > we be doing this for all elements that support posinset?
> > There are some other elements except radio button but they are already
handled
> > with AX tree order.
> > Currently Radiobuttons are also handled with AX tree but it's not working
> > correctly
> > because AX tree doesn't care it with RadioGroup Information.
> AX tree order, isn't that roughly dom order with extra nodes?
> If we need to do visual order I'm hoping there's a more generic, official way
to
> do it.

I think I tried to handle it more complicated as I thought visual order is more
generic because of dynamic updating with js. After discussion with keishi, I
think it's OK to handle it with DOM order.
Thanks a lot to keishi for helping me find solution. I'll update code with DOM
order.
Dominic, if you have any opinion about that, please let me know.
Thanks,

[dmazzoni, 2016-02-17 07:04:56]

On 2016/02/17 02:52:35, je_julie wrote:
> I think I tried to handle it more complicated as I thought visual order is
more
> generic because of dynamic updating with js. After discussion with keishi, I
> think it's OK to handle it with DOM order.
> Thanks a lot to keishi for helping me find solution. I'll update code with DOM
> order.
> Dominic, if you have any opinion about that, please let me know.

I agree, we should use the DOM order.

I suppose we could try to do slightly better and use the order in the AX tree,
for example if the AX tree is reordered by aria-owns we could use that order.
But that's a pretty unusual case, I'm fine with just using the DOM order.

[je_julie(Not used), 2016-02-22 14:04:40]

Patchset #4 (id:140001) has been deleted

[je_julie(Not used), 2016-02-22 15:29:48]

Dominic and keishi,
I updated patchset with several changes below.
1) added AXRadioInput.h/cpp.
It’s created when node is input and radio type.
I named it as AXRadioInput rather than AXRadioButton because it could have
MenuItemRadioRole or RadioButtonRole.
It overrides posInSet() and setSize() from AXObject and has m_posInSet to mark
position in radio group.
When it’s created, it set ‘setNeedToUpdateAXPosition()’ as AX tree could be
created after RadioButtons are added or removed and already updated.
2) AXObjectCache has radiobuttonPositionChanged.
When RadioButtonGroup updates AX position, it’s called with position
information.
3) InputType has virtual API to test Radio Input type to avoid type checking.
4) Here is working flow.
Case 1) AX tree is created and radiobutton is added and removed.
->RadioButtons are added or removed
->Dirty flag is set.
->AX accesses AXRadioInput Attribute, posInSet.
->AXRadioInput::posInSet() updates AXPosition when dirty flag is set.
Case 2) Radiobuttons are added and AX tree is updated. And then AX is disabled
and enabled again.
RadioButtons are added and removed, dirty flag is set.
->AXPosition is updated when AX tree is updated and dirty flag is reset.
->AX is disabled and it destroys AX tree.
->AX is enabled again and AXRadioInput is created and set dirty flag.
->when posInSet is accessed, AXPositon is updated.
PTAL.

[keishi, 2016-02-23 09:43:00]

https://codereview.chromium.org/1628283002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/forms/RadioButtonGroupScope.cpp
(right):

https://codereview.chromium.org/1628283002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/forms/RadioButtonGroupScope.cpp:242: if
(!isNextPosition(item, current))
This reordering is like O(N^3). Could we use
RadioInputType::findNextFocusableRadioButtonInGroup to make it O(N).

https://codereview.chromium.org/1628283002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/forms/RadioButtonGroupScope.cpp:375:
group->setNeedToUpdate(set);
If we do something like RadioInputType::findNextFocusableRadioButtonInGroup to
find all the radio buttons I think you won't need access to the
RadioButtonGroupScope. RadioButtonGroup::add/remove could call something like
AXObjectCache::handleRadioButtonGroupChange and it could invalidate the cached
posinset/setsize values. The posinset/setsize can be recalculated from the
accessibility module, and we won't need to add all of these *AXPosition* APIs.

[je_julie, 2016-02-26 15:14:38]

Patchset #5 (id:180001) has been deleted

[je_julie(Not used), 2016-02-27 14:01:08]

keishi and Dominic,

I moved the main code to handle posInSet and setSize AX-attributes to
AXRadioInput using element traversal like findNextFocusableRadioButtonInGroup.
PTAL.

https://codereview.chromium.org/1628283002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/forms/RadioButtonGroupScope.cpp
(right):

https://codereview.chromium.org/1628283002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/forms/RadioButtonGroupScope.cpp:242: if
(!isNextPosition(item, current))
On 2016/02/23 09:43:00, keishi wrote:
> This reordering is like O(N^3). Could we use
> RadioInputType::findNextFocusableRadioButtonInGroup to make it O(N).

I moved handling position for AX to AXRadioInput and it checked with the way
like findNextFocusableRadioButtonInGroup.

https://codereview.chromium.org/1628283002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/forms/RadioButtonGroupScope.cpp:375:
group->setNeedToUpdate(set);
On 2016/02/23 09:43:00, keishi wrote:
> If we do something like RadioInputType::findNextFocusableRadioButtonInGroup to
> find all the radio buttons I think you won't need access to the
> RadioButtonGroupScope. RadioButtonGroup::add/remove could call something like
> AXObjectCache::handleRadioButtonGroupChange and it could invalidate the cached
> posinset/setsize values. The posinset/setsize can be recalculated from the
> accessibility module, and we won't need to add all of these *AXPosition* APIs.

I removed almost all handling to AXRadioInput as you recommended.

[keishi, 2016-02-29 05:19:19]

I am uncertain about the timing of AX notifications, but the forms side looks
good.

https://codereview.chromium.org/1628283002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXRadioInput.cpp (right):

https://codereview.chromium.org/1628283002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXRadioInput.cpp:63:
HTMLInputElement*
AXRadioInput::findFristFocusableRadioButtonInGroup(HTMLInputElement* current)
const
s/Frist/First/

https://codereview.chromium.org/1628283002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXRadioInput.cpp:115:
HTMLInputElement*
AXRadioInput::findNextFocusableRadioButtonInGroup(HTMLInputElement* current,
bool forward) const
nit: This function isn't checking focusable.

[je_julie(Not used), 2016-02-29 15:11:45]

keishi, Thanks for your time to review this patch.
I fixed typo and added comment why I removed focusable checking.

I'll wait for Dominic's review after he's back.
Dominic, PTAL.

https://codereview.chromium.org/1628283002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXRadioInput.cpp (right):

https://codereview.chromium.org/1628283002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXRadioInput.cpp:63:
HTMLInputElement*
AXRadioInput::findFristFocusableRadioButtonInGroup(HTMLInputElement* current)
const
On 2016/02/29 05:19:19, keishi wrote:
> s/Frist/First/

Done.

https://codereview.chromium.org/1628283002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXRadioInput.cpp:115:
HTMLInputElement*
AXRadioInput::findNextFocusableRadioButtonInGroup(HTMLInputElement* current,
bool forward) const
On 2016/02/29 05:19:19, keishi wrote:
> nit: This function isn't checking focusable.

According to spec, there is no mention about focusable checking. I also checked
TC on FireFox and It doesn't check focusable either.

[dmazzoni, 2016-02-29 17:16:19]

lgtm

https://codereview.chromium.org/1628283002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXRadioInput.cpp (right):

https://codereview.chromium.org/1628283002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXRadioInput.cpp:115:
HTMLInputElement*
AXRadioInput::findNextFocusableRadioButtonInGroup(HTMLInputElement* current,
bool forward) const
On 2016/02/29 15:11:44, je_julie wrote:
> On 2016/02/29 05:19:19, keishi wrote:
> > nit: This function isn't checking focusable.
> 
> According to spec, there is no mention about focusable checking. I also
checked
> TC on FireFox and It doesn't check focusable either.

That's fine, but change the name of the function from
findNextFocusableRadioButtonInGroup to findNextRadioButtonInGroup

https://codereview.chromium.org/1628283002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXRadioInput.cpp (right):

https://codereview.chromium.org/1628283002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXRadioInput.cpp:18: return
forward ? Traversal<HTMLElement>::next(element, (Node* )stayWithin) :
Traversal<HTMLElement>::previous(element, (Node* )stayWithin);
Is it necessary to cast |stayWithin| to Node*? If it compiles without it, do
that, otherwise please use the proper C++ cast like static_cast<>, etc.

https://codereview.chromium.org/1628283002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXRadioInput.cpp:54: if
(forward)
What if |forward| is false, should position be posInSet() - 1?

[keishi, 2016-03-01 01:22:32]

keishi@chromium.org changed reviewers:
+ tkent@chromium.org

[keishi, 2016-03-01 01:22:33]

LGTM

+tkent FYI

[tkent, 2016-03-01 01:43:01]

https://codereview.chromium.org/1628283002/diff/220001/content/test/data/acce...
File content/test/data/accessibility/aria/aria-posinset.html (right):

https://codereview.chromium.org/1628283002/diff/220001/content/test/data/acce...
content/test/data/accessibility/aria/aria-posinset.html:45: child.type =
"radio";
We need a test to change input type from "radio" to another type after creating
AXRadioInput.

https://codereview.chromium.org/1628283002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLInputElement.h (right):

https://codereview.chromium.org/1628283002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLInputElement.h:98: bool isRadioButton()
const;
Please do not add this.
Use |input->formControlType() == InputTypeNames::radio| outside
HTMLInputElement.

https://codereview.chromium.org/1628283002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/forms/RadioButtonGroupScope.cpp
(right):

https://codereview.chromium.org/1628283002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/forms/RadioButtonGroupScope.cpp:41: unsigned
sizeOfMembers() const;
I'd like name it "size" simply.

https://codereview.chromium.org/1628283002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/forms/RadioButtonGroupScope.h (right):

https://codereview.chromium.org/1628283002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/forms/RadioButtonGroupScope.h:47: unsigned
sizeOfGroup(const HTMLInputElement*) const;
I'd like to name it "groupSizeFor"

https://codereview.chromium.org/1628283002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXRadioInput.cpp (right):

https://codereview.chromium.org/1628283002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXRadioInput.cpp:147: if (!size)
Does this actually happen?

[tkent, 2016-03-01 02:17:49]

https://codereview.chromium.org/1628283002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLInputElement.h (right):

https://codereview.chromium.org/1628283002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLInputElement.h:98: bool isRadioButton()
const;
On 2016/03/01 at 01:43:00, tkent wrote:
> Please do not add this.
> Use |input->formControlType() == InputTypeNames::radio| outside
HTMLInputElement.

oops, formControlType() -> type()

[je_julie(Not used), 2016-03-01 13:46:07]

tkent and keishi,
I'd like to check one thing before I upload new patchset.
Please my comment below.

Dominic,
I replied to your questions.
PTAL.

https://codereview.chromium.org/1628283002/diff/220001/content/test/data/acce...
File content/test/data/accessibility/aria/aria-posinset.html (right):

https://codereview.chromium.org/1628283002/diff/220001/content/test/data/acce...
content/test/data/accessibility/aria/aria-posinset.html:45: child.type =
"radio";
On 2016/03/01 01:43:00, tkent wrote:
> We need a test to change input type from "radio" to another type after
creating
> AXRadioInput.

I'll add it.

https://codereview.chromium.org/1628283002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLInputElement.h (right):

https://codereview.chromium.org/1628283002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLInputElement.h:98: bool isRadioButton()
const;
On 2016/03/01 02:17:49, tkent wrote:
> On 2016/03/01 at 01:43:00, tkent wrote:
> > Please do not add this.
> > Use |input->formControlType() == InputTypeNames::radio| outside
> HTMLInputElement.
> 
> oops, formControlType() -> type()

tkent and keishi,
keishi left comment
"third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:324: if
(isHTMLInputElement(node()) && toHTMLInputElement(*node()).type() ==
InputTypeNames::radio)
We try to avoid the "if (input.type==foo) input.bar()" pattern using the
InputType class."at #16 review comment and I chose the way to add that API.
I'd better checking radio type with type() API at this patch for now?
WDYT?

https://codereview.chromium.org/1628283002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/forms/RadioButtonGroupScope.cpp
(right):

https://codereview.chromium.org/1628283002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/forms/RadioButtonGroupScope.cpp:41: unsigned
sizeOfMembers() const;
On 2016/03/01 01:43:00, tkent wrote:
> I'd like name it "size" simply.

I'll change it at next patch.

https://codereview.chromium.org/1628283002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/forms/RadioButtonGroupScope.h (right):

https://codereview.chromium.org/1628283002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/forms/RadioButtonGroupScope.h:47: unsigned
sizeOfGroup(const HTMLInputElement*) const;
On 2016/03/01 01:43:00, tkent wrote:
> I'd like to name it "groupSizeFor"

I'll change it at next patch.

https://codereview.chromium.org/1628283002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXRadioInput.cpp (right):

https://codereview.chromium.org/1628283002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXRadioInput.cpp:18: return
forward ? Traversal<HTMLElement>::next(element, (Node* )stayWithin) :
Traversal<HTMLElement>::previous(element, (Node* )stayWithin);
On 2016/02/29 17:16:19, dmazzoni wrote:
> Is it necessary to cast |stayWithin| to Node*? If it compiles without it, do
> that, otherwise please use the proper C++ cast like static_cast<>, etc.
> 

I'll update it at next patch.

https://codereview.chromium.org/1628283002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXRadioInput.cpp:54: if
(forward)
On 2016/02/29 17:16:19, dmazzoni wrote:
> What if |forward| is false, should position be posInSet() - 1?

'requestUpdateToNextNode(false)' is only called in ctor and previous objects
will update only size, not position, as new objects are added.
If we call updatePosAndSetSize with 0, it keeps position.
I'll leave comment here at next patchset.

https://codereview.chromium.org/1628283002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXRadioInput.cpp:147: if (!size)
On 2016/03/01 01:43:00, tkent wrote:
> Does this actually happen?

It could happen when radio input element doesn't have name attribute and is not
grouped.

[tkent, 2016-03-02 00:40:02]

https://codereview.chromium.org/1628283002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLInputElement.h (right):

https://codereview.chromium.org/1628283002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLInputElement.h:98: bool isRadioButton()
const;
On 2016/03/01 at 13:46:07, je_julie wrote:
> On 2016/03/01 02:17:49, tkent wrote:
> > On 2016/03/01 at 01:43:00, tkent wrote:
> > > Please do not add this.
> > > Use |input->formControlType() == InputTypeNames::radio| outside
> > HTMLInputElement.
> > 
> > oops, formControlType() -> type()
> 
> tkent and keishi,
> keishi left comment
"third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:324: if
> (isHTMLInputElement(node()) && toHTMLInputElement(*node()).type() ==
> InputTypeNames::radio)
> We try to avoid the "if (input.type==foo) input.bar()" pattern using the
> InputType class."at #16 review comment and I chose the way to add that API.
> I'd better checking radio type with type() API at this patch for now?
> WDYT?

Please follow my comment.


Keishi's comment meant we should have done:
  if (isHTMLInputElement(node()))
     toHTMLInputElement(*node()).needUpdatePositionOfRadioGroup();
and should have applied virtual dispatch by InputType if necessary.

In the latest Patch Set:

if (isHTMLInputElement(node) && toHTMLInputElement(node)->isRadioButton())
    return AXRadioInput::create(layoutObject, *this);

Our normal way is to introduce HTMLInputElement::createAXObject(), and
InputType::createAXObject.  However HTMLInputElement should not have knowledge
of AX objects in modules/accessibility/.  So type checking by type() is
reasonable.

Introducing isRadioButton() improves nothing.

[je_julie(Not used), 2016-03-02 15:26:24]

Description was changed from

==========
posinset and setsize for input type, radio, exposed in AX tree

posinset and setsize for radio input should be index and size of group
and exposed as object attributes.
This patch makes html input have posInRadioGroup() and sizeOfRadioGroup()
and adds RadioButtonGroup::updatePosition() to get position.

BUG=557041
==========

to

==========
posinset and setsize for input type, radio, exposed in AX tree

posinset and setsize for radio input should be index and size of group
and exposed as object attributes.
This patch introduces AXRadioInput class to handle input element with radio
type.

BUG=557041
==========

[je_julie(Not used), 2016-03-02 15:29:34]

Description was changed from

==========
posinset and setsize for input type, radio, exposed in AX tree

posinset and setsize for radio input should be index and size of group
and exposed as object attributes.
This patch introduces AXRadioInput class to handle input element with radio
type.

BUG=557041
==========

to

==========
posinset and setsize for input type, radio, exposed in AX tree

posinset and setsize for radio input should be index and size of group
and exposed as object attributes.
This patch introduces AXRadioInput class to handle input element with
radio type.

BUG=557041
==========

[je_julie(Not used), 2016-03-03 04:04:23]

Patchset #7 (id:240001) has been deleted

[je_julie(Not used), 2016-03-03 07:00:19]

On 2016/03/02 00:40:02, tkent wrote:
> Please follow my comment.
> 
> 
> Keishi's comment meant we should have done:
>   if (isHTMLInputElement(node()))
>      toHTMLInputElement(*node()).needUpdatePositionOfRadioGroup();
> and should have applied virtual dispatch by InputType if necessary.
> 
> In the latest Patch Set:
> 
> if (isHTMLInputElement(node) && toHTMLInputElement(node)->isRadioButton())
>     return AXRadioInput::create(layoutObject, *this);
> 
> Our normal way is to introduce HTMLInputElement::createAXObject(), and
> InputType::createAXObject.  However HTMLInputElement should not have knowledge
> of AX objects in modules/accessibility/.  So type checking by type() is
> reasonable.
> 
> Introducing isRadioButton() improves nothing.

tkent, I removed isRadioButton() and changed APIs' names. PTAL.

[tkent, 2016-03-03 12:49:31]

lgtm

[je_julie(Not used), 2016-03-04 00:23:12]

On 2016/03/03 12:49:31, tkent wrote:
> lgtm

Reviewers,
Thanks for your time to review this patch.
I'll try to land it within today if there is no further comment.
Thanks,

[je_julie(Not used), 2016-03-05 01:19:42]

The CQ bit was checked by je_julie.kim@samsung.com

[je_julie(Not used), 2016-03-05 01:19:43]

The patchset sent to the CQ was uploaded after l-g-t-m from keishi@chromium.org,
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/1628283002/#ps260001
(title: "removed isRadiobutton()")

[commit-bot: I haz the power, 2016-03-05 01:20:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1628283002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1628283002/260001

[commit-bot: I haz the power, 2016-03-05 02:51:11]

Description was changed from

==========
posinset and setsize for input type, radio, exposed in AX tree

posinset and setsize for radio input should be index and size of group
and exposed as object attributes.
This patch introduces AXRadioInput class to handle input element with
radio type.

BUG=557041
==========

to

==========
posinset and setsize for input type, radio, exposed in AX tree

posinset and setsize for radio input should be index and size of group
and exposed as object attributes.
This patch introduces AXRadioInput class to handle input element with
radio type.

BUG=557041
==========

[commit-bot: I haz the power, 2016-03-05 02:51:12]

Committed patchset #7 (id:260001)

[commit-bot: I haz the power, 2016-03-05 02:52:41]

Description was changed from

==========
posinset and setsize for input type, radio, exposed in AX tree

posinset and setsize for radio input should be index and size of group
and exposed as object attributes.
This patch introduces AXRadioInput class to handle input element with
radio type.

BUG=557041
==========

to

==========
posinset and setsize for input type, radio, exposed in AX tree

posinset and setsize for radio input should be index and size of group
and exposed as object attributes.
This patch introduces AXRadioInput class to handle input element with
radio type.

BUG=557041

Committed: https://crrev.com/c52244a6cfc56abce6411a2d82b31eaa06167ec3
Cr-Commit-Position: refs/heads/master@{#379445}
==========

[commit-bot: I haz the power, 2016-03-05 02:52:43]

Patchset 7 (id:??) landed as
https://crrev.com/c52244a6cfc56abce6411a2d82b31eaa06167ec3
Cr-Commit-Position: refs/heads/master@{#379445}