Only call TextChanged on existing AXObjects. This avoids triggering the code which calls UpdateDistribution as part of creating a new AXObject, which was causing issues.
This change should cause no behaviour changes, as in cases where an AXObject needs to be created for a text node, its parent will also have a ChildrenChanged notification fired on it, which will cause the appropriate updates to fire.
Reverts https://codereview.chromium.org/2926713002/ as the code which causes UpdateDistribution to be called should never run with this change, so we don't need to check whether distribution is dirty any more.
BUG=729229, 732200, 734348
Review-Url: https://codereview.chromium.org/2939933002
Cr-Commit-Position: refs/heads/master@{#480792}
Committed: https://chromium.googlesource.com/chromium/src/+/0011b9db970f1203f59c4e1b1b52fcdd2035101e
Description was changed from ========== Only call TextChanged on existing AXObjects BUG=729229,732200 ========== to ========== ...
3 years, 6 months ago
(2017-06-14 16:39:05 UTC)
#1
Description was changed from
==========
Only call TextChanged on existing AXObjects
BUG=729229,732200
==========
to
==========
Only call TextChanged on existing AXObjects. This avoids triggering the code
which calls UpdateDistribution as part of creating a new AXObject, which was
causing issues.
This change should cause no behaviour changes, as in cases where an AXObject
needs to be created for a text node, its parent will also have a ChildrenChanged
notification fired on it, which will cause the appropriate updates to fire.
Reverts https://codereview.chromium.org/2926713002/ as the code which causes
UpdateDistribution to be called should never run with this change, so we don't
need to check whether distribution is dirty any more.
BUG=729229,732200
==========
3 years, 6 months ago
(2017-06-14 16:39:14 UTC)
#3
kojii
Superb! Thank you! LGTM!
3 years, 6 months ago
(2017-06-14 16:49:01 UTC)
#4
Superb! Thank you! LGTM!
dmazzoni
lgtm with nit https://codereview.chromium.org/2939933002/diff/1/third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp File third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp (right): https://codereview.chromium.org/2939933002/diff/1/third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp#newcode589 third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp:589: if (!axObject) I think you could ...
3 years, 6 months ago
(2017-06-14 18:38:15 UTC)
#5
https://codereview.chromium.org/2939933002/diff/1/third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp File third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp (right): https://codereview.chromium.org/2939933002/diff/1/third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp#newcode589 third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp:589: if (!axObject) On 2017/06/14 18:38:14, dmazzoni wrote: > I ...
3 years, 6 months ago
(2017-06-14 18:46:05 UTC)
#6
https://codereview.chromium.org/2939933002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp
(right):
https://codereview.chromium.org/2939933002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp:589: if
(!axObject)
On 2017/06/14 18:38:14, dmazzoni wrote:
> I think you could remove this null-check since TextChanged(AXObjectImpl*)
> already does that. Throughout the whole file pretty much all methods allow
null
> as an argument to keep the code simple.
>
> I'd just write TextChanged(Get(node))
>
> and add a comment saying we don't want to do anything if there's no existing
> object because layout may not be clean and/or updatedistribution may be needed
Great, done.
https://codereview.chromium.org/2939933002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp:596: if
(!axObject)
On 2017/06/14 18:38:14, dmazzoni wrote:
> Same
Done.
aboxhall
The CQ bit was checked by aboxhall@chromium.org
3 years, 6 months ago
(2017-06-14 18:46:08 UTC)
#7
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/449738)
3 years, 6 months ago
(2017-06-14 19:54:06 UTC)
#11
Keep in mind that LayoutTests differ from when accessibility is enabled in Chrome, in that ...
3 years, 6 months ago
(2017-06-15 06:14:38 UTC)
#12
Keep in mind that LayoutTests differ from when accessibility
is enabled in Chrome, in that there's no code to explore the
whole accessibility tree.
Previously the TextChanged event would trigger creating an
AXObject, but now it won't. The tests were essentially
depending on it creating the AXObject, but they shouldn't.
Exploring the whole tree towards the beginning of the
test may fix it. A call to accessibleElementById("dummy")
is one way to do that.
aboxhall
On 2017/06/15 06:14:38, dmazzoni wrote: > Keep in mind that LayoutTests differ from when accessibility ...
3 years, 6 months ago
(2017-06-15 18:27:14 UTC)
#13
On 2017/06/15 06:14:38, dmazzoni wrote:
> Keep in mind that LayoutTests differ from when accessibility
> is enabled in Chrome, in that there's no code to explore the
> whole accessibility tree.
>
> Previously the TextChanged event would trigger creating an
> AXObject, but now it won't. The tests were essentially
> depending on it creating the AXObject, but they shouldn't.
>
> Exploring the whole tree towards the beginning of the
> test may fix it. A call to accessibleElementById("dummy")
> is one way to do that.
Hm - given the content_browsertests failure this looks like a more complex issue
around the expected notifications not firing. I had a play with explicitly
firing notifications on the parent, but it doesn't fix the issue. Any ideas?
aboxhall
The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run
3 years, 6 months ago
(2017-06-16 21:28:02 UTC)
#14
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/451678)
3 years, 6 months ago
(2017-06-16 23:19:08 UTC)
#17
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/479766)
3 years, 6 months ago
(2017-06-18 23:22:05 UTC)
#21
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/470458)
3 years, 6 months ago
(2017-06-19 01:57:40 UTC)
#25
The only failure is AccessibilityEventsMenuListCollapse on Windows, but it's firing an extra (harmless) event, so ...
3 years, 6 months ago
(2017-06-20 05:00:12 UTC)
#26
The only failure is AccessibilityEventsMenuListCollapse on Windows,
but it's firing an extra (harmless) event, so go ahead and update the
expectation.
lgtm!
aboxhall
The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run
3 years, 6 months ago
(2017-06-20 05:43:35 UTC)
#27
Description was changed from ========== Only call TextChanged on existing AXObjects. This avoids triggering the ...
3 years, 6 months ago
(2017-06-20 11:43:08 UTC)
#38
Description was changed from
==========
Only call TextChanged on existing AXObjects. This avoids triggering the code
which calls UpdateDistribution as part of creating a new AXObject, which was
causing issues.
This change should cause no behaviour changes, as in cases where an AXObject
needs to be created for a text node, its parent will also have a ChildrenChanged
notification fired on it, which will cause the appropriate updates to fire.
Reverts https://codereview.chromium.org/2926713002/ as the code which causes
UpdateDistribution to be called should never run with this change, so we don't
need to check whether distribution is dirty any more.
BUG=729229,732200
==========
to
==========
Only call TextChanged on existing AXObjects. This avoids triggering the code
which calls UpdateDistribution as part of creating a new AXObject, which was
causing issues.
This change should cause no behaviour changes, as in cases where an AXObject
needs to be created for a text node, its parent will also have a ChildrenChanged
notification fired on it, which will cause the appropriate updates to fire.
Reverts https://codereview.chromium.org/2926713002/ as the code which causes
UpdateDistribution to be called should never run with this change, so we don't
need to check whether distribution is dirty any more.
BUG=729229,732200,734348
==========
aboxhall
The CQ bit was checked by aboxhall@chromium.org
3 years, 6 months ago
(2017-06-20 11:43:14 UTC)
#39
On 2017/06/20 08:00:01, Takayoshi Kochi (Google) wrote: > On 2017/06/20 07:58:57, Takayoshi Kochi (Google) wrote: ...
3 years, 6 months ago
(2017-06-20 11:43:27 UTC)
#41
On 2017/06/20 08:00:01, Takayoshi Kochi (Google) wrote:
> On 2017/06/20 07:58:57, Takayoshi Kochi (Google) wrote:
> > This seems also fixing crbug.com/734348 :)
>
> Alice, feel free to add 734348 to BUG= line.
Done!
commit-bot: I haz the power
CQ is committing da patch. Bot data: {"patchset_id": 120001, "attempt_start_ts": 1497958994907390, "parent_rev": "8f3cf8617dc8c27d6b0155dd775d4beebd58d62c", "commit_rev": "0011b9db970f1203f59c4e1b1b52fcdd2035101e"}
3 years, 6 months ago
(2017-06-20 11:49:33 UTC)
#42
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1497958994907390,
"parent_rev": "8f3cf8617dc8c27d6b0155dd775d4beebd58d62c", "commit_rev":
"0011b9db970f1203f59c4e1b1b52fcdd2035101e"}
commit-bot: I haz the power
Description was changed from ========== Only call TextChanged on existing AXObjects. This avoids triggering the ...
3 years, 6 months ago
(2017-06-20 11:49:48 UTC)
#43
Message was sent while issue was closed.
Description was changed from
==========
Only call TextChanged on existing AXObjects. This avoids triggering the code
which calls UpdateDistribution as part of creating a new AXObject, which was
causing issues.
This change should cause no behaviour changes, as in cases where an AXObject
needs to be created for a text node, its parent will also have a ChildrenChanged
notification fired on it, which will cause the appropriate updates to fire.
Reverts https://codereview.chromium.org/2926713002/ as the code which causes
UpdateDistribution to be called should never run with this change, so we don't
need to check whether distribution is dirty any more.
BUG=729229,732200,734348
==========
to
==========
Only call TextChanged on existing AXObjects. This avoids triggering the code
which calls UpdateDistribution as part of creating a new AXObject, which was
causing issues.
This change should cause no behaviour changes, as in cases where an AXObject
needs to be created for a text node, its parent will also have a ChildrenChanged
notification fired on it, which will cause the appropriate updates to fire.
Reverts https://codereview.chromium.org/2926713002/ as the code which causes
UpdateDistribution to be called should never run with this change, so we don't
need to check whether distribution is dirty any more.
BUG=729229,732200,734348
Review-Url: https://codereview.chromium.org/2939933002
Cr-Commit-Position: refs/heads/master@{#480792}
Committed:
https://chromium.googlesource.com/chromium/src/+/0011b9db970f1203f59c4e1b1b52...
==========
commit-bot: I haz the power
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/0011b9db970f1203f59c4e1b1b52fcdd2035101e
3 years, 6 months ago
(2017-06-20 11:49:50 UTC)
#44
Issue 2939933002: Only call TextChanged on existing AXObjects
(Closed)
Created 3 years, 6 months ago by aboxhall
Modified 3 years, 6 months ago
Reviewers: dmazzoni, kojii, Takayoshi Kochi (Google)
Base URL:
Comments: 4