Fire AX notifications on list box options.

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
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192997

[dmazzoni, 2015-03-30 15:11:42]

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

[Mike West, 2015-03-30 18:44:03]

The bots are on fire. :(

[dmazzoni, 2015-03-30 19:32:45]

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

[je_julie(Not used), 2015-03-31 03:51:04]

Nice change!
There is no expected result.

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

https://codereview.chromium.org/1034493005/diff/40001/Source/core/html/HTMLOp...
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/accessib...
File Source/modules/accessibility/AXListBox.cpp (right):

https://codereview.chromium.org/1034493005/diff/40001/Source/modules/accessib...
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/accessib...
Source/modules/accessibility/AXListBox.cpp:83: if (!node() ||
!isHTMLSelectElement(node()))
Ditto.

[Mike West, 2015-03-31 05:20:52]

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

[dmazzoni, 2015-03-31 06:57:37]

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

https://codereview.chromium.org/1034493005/diff/40001/Source/core/html/HTMLOp...
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/accessib...
File Source/modules/accessibility/AXListBox.cpp (right):

https://codereview.chromium.org/1034493005/diff/40001/Source/modules/accessib...
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/accessib...
Source/modules/accessibility/AXListBox.cpp:83: if (!node() ||
!isHTMLSelectElement(node()))
On 2015/03/31 03:51:04, je_julie wrote:
> Ditto.

Done.

[je_julie(Not used), 2015-03-31 14:54:53]

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?

[dmazzoni, 2015-03-31 15:03:47]

> 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.

[je_julie(Not used), 2015-03-31 16:16:42]

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!

[dmazzoni, 2015-04-01 05:11:18]

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

[dmazzoni, 2015-04-01 19:35:24]

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

[dmazzoni, 2015-04-01 19:35:24]

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

[tkent, 2015-04-01 23:52:29]

core lgtm

[dmazzoni, 2015-04-02 02:23:39]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2015-04-02 02:24:19]

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

[commit-bot: I haz the power, 2015-04-02 05:37:38]

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

[mark a. foltz, 2015-05-06 22:25:12]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1127283002/ by mfoltz@chromium.org.

The reason for reverting is: Suspect as cause of #1 crash in Windows canaries
44.0.2392.0 and 44.0.2393.0

BUG=484642.