Fire AX notifications on list box options. (issue 1034493005 by dmazzoni@chromium.org)

[dmaz...@chromium.org]

Reviewers: je_julie, Mike West,

Description:
Fire AX notifications on list box options.

Previously we just sent a single notification on the
list box when the selection changed, now we send
separate notifications on each option that changes
state too.

BUG=323462

Please review this at https://codereview.chromium.org/1034493005/

Base URL:
https://chromium.googlesource.com/chromium/blink.git@menu_list_more

Affected files (+141, -20 lines):
A LayoutTests/accessibility/listbox-focus.html
M Source/core/dom/AXObjectCache.h
M Source/core/html/HTMLOptionElement.cpp
M Source/core/html/HTMLSelectElement.cpp
M Source/modules/accessibility/AXListBox.h
M Source/modules/accessibility/AXListBox.cpp
M Source/modules/accessibility/AXListBoxOption.h
M Source/modules/accessibility/AXObject.h
M Source/modules/accessibility/AXObjectCacheImpl.h
M Source/modules/accessibility/AXObjectCacheImpl.cpp

[mk...@chromium.org]

The bots are on fire. :(

https://codereview.chromium.org/1034493005/

[dmaz...@chromium.org]

I just jumped the gun on "git cl try", it's based on top of the previous
patch.
Rebasing now...


https://codereview.chromium.org/1034493005/

[je_jul...@samsung.com]

Nice change!
There is no expected result.


https://codereview.chromium.org/1034493005/diff/40001/Source/core/html/HTMLOptionElement.cpp
File Source/core/html/HTMLOptionElement.cpp (right):

https://codereview.chromium.org/1034493005/diff/40001/Source/core/html/HTMLOptionElement.cpp#newcode272
Source/core/html/HTMLOptionElement.cpp:272: if (!select->layoutObject()
|| select->layoutObject()->isListBox()) {
((there is no layoutObject) or (layoutObject is ListBox))
Do you intent to check whether it has no layoutobject and the
layoutobject is listbox if it has layoutobject?

https://codereview.chromium.org/1034493005/diff/40001/Source/modules/accessibility/AXListBox.cpp
File Source/modules/accessibility/AXListBox.cpp (right):

https://codereview.chromium.org/1034493005/diff/40001/Source/modules/accessibility/AXListBox.cpp#newcode68
Source/modules/accessibility/AXListBox.cpp:68: if (!node() ||
!isHTMLSelectElement(node()))
When we use isHTMLXXXElement(const Node* node), we don't need to check
null for node because it checks null inside it.
So, I think we can remove '!node()' condition.

https://codereview.chromium.org/1034493005/diff/40001/Source/modules/accessibility/AXListBox.cpp#newcode83
Source/modules/accessibility/AXListBox.cpp:83: if (!node() ||
!isHTMLSelectElement(node()))
Ditto.

https://codereview.chromium.org/1034493005/

[mk...@chromium.org]

The rebase seems to have failed. Now _all_ the bots are red. ;)

https://codereview.chromium.org/1034493005/

[dmaz...@chromium.org]

https://codereview.chromium.org/1034493005/diff/40001/Source/core/html/HTMLOptionElement.cpp
File Source/core/html/HTMLOptionElement.cpp (right):

https://codereview.chromium.org/1034493005/diff/40001/Source/core/html/HTMLOptionElement.cpp#newcode272
Source/core/html/HTMLOptionElement.cpp:272: if (!select->layoutObject()
|| select->layoutObject()->isListBox()) {

On 2015/03/31 03:51:04, je_julie wrote:
> ((there is no layoutObject) or (layoutObject is ListBox))
> Do you intent to check whether it has no layoutobject and the
layoutobject is
> listbox if it has layoutobject?

I want to fire these notifications under two circumstances:
* If it does have a layout object (the most common case), I only want to
fire these notifications if it's a list box (and not a menu list).
* If it does not have a layout object (rare, maybe only inside of a
<canvas> subtree), I want to fire these notifications anyway, because it
won't be rendered on screen but will act to the user kind of like a
listbox.

I added a comment to clarify this.

https://codereview.chromium.org/1034493005/diff/40001/Source/modules/accessibility/AXListBox.cpp
File Source/modules/accessibility/AXListBox.cpp (right):

https://codereview.chromium.org/1034493005/diff/40001/Source/modules/accessibility/AXListBox.cpp#newcode68
Source/modules/accessibility/AXListBox.cpp:68: if (!node() ||
!isHTMLSelectElement(node()))

On 2015/03/31 03:51:04, je_julie wrote:
> When we use isHTMLXXXElement(const Node* node), we don't need to check
null for
> node because it checks null inside it.
> So, I think we can remove '!node()' condition.

Thanks!

https://codereview.chromium.org/1034493005/diff/40001/Source/modules/accessibility/AXListBox.cpp#newcode83
Source/modules/accessibility/AXListBox.cpp:83: if (!node() ||
!isHTMLSelectElement(node()))

On 2015/03/31 03:51:04, je_julie wrote:
> Ditto.

Done.

https://codereview.chromium.org/1034493005/

[je_jul...@samsung.com]

On 2015/03/31 06:57:37, dmazzoni wrote:
> I want to fire these notifications under two circumstances:
> * If it does have a layout object (the most common case), I only want to
> fire
> these notifications if it's a list box (and not a menu list).
> * If it does not have a layout object (rare, maybe only inside of a
> <canvas>
> subtree), I want to fire these notifications anyway, because it won't be
> rendered on screen but will act to the user kind of like a listbox.

> I added a comment to clarify this.

Thanks!
BTW, don't we need expected result for the added layout test?


https://codereview.chromium.org/1034493005/

[dmaz...@chromium.org]

> BTW, don't we need expected result for the added layout test?

No, if you use testharness.js you don't need an expectation file. Check out
the
comment at the top of LayoutTests/resources/testharness.js for details - it
was
new to me too.




https://codereview.chromium.org/1034493005/

[je_jul...@samsung.com]

On 2015/03/31 15:03:47, dmazzoni wrote:
> > BTW, don't we need expected result for the added layout test?

> No, if you use testharness.js you don't need an expectation file. Check
> out
the
> comment at the top of LayoutTests/resources/testharness.js for details -
> it
was
> new to me too.

Ah, Thanks for letting me know.
LGTM!

https://codereview.chromium.org/1034493005/

[dmaz...@chromium.org]

@mkwst, I fixed all of the errors and the try bots are happy now. Thanks!


https://codereview.chromium.org/1034493005/

[dmaz...@chromium.org]

+tkent - would you mind reviewing the changes to Source/core/?


https://codereview.chromium.org/1034493005/

[tk...@chromium.org]

core lgtm


https://codereview.chromium.org/1034493005/

[commi...@chromium.org]

CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/1034493005/120001


https://codereview.chromium.org/1034493005/

[commi...@chromium.org]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192997

https://codereview.chromium.org/1034493005/