3 years, 5 months ago
(2017-07-06 05:09:05 UTC)
#4
aboxhall
https://codereview.chromium.org/2967193003/diff/1/third_party/WebKit/Source/core/dom/AccessibleNode.cpp File third_party/WebKit/Source/core/dom/AccessibleNode.cpp (right): https://codereview.chromium.org/2967193003/diff/1/third_party/WebKit/Source/core/dom/AccessibleNode.cpp#newcode86 third_party/WebKit/Source/core/dom/AccessibleNode.cpp:86: // Note that there are two valid spellings of ...
3 years, 5 months ago
(2017-07-06 05:37:14 UTC)
#5
https://codereview.chromium.org/2967193003/diff/1/third_party/WebKit/Source/core/dom/AccessibleNode.cpp File third_party/WebKit/Source/core/dom/AccessibleNode.cpp (right): https://codereview.chromium.org/2967193003/diff/1/third_party/WebKit/Source/core/dom/AccessibleNode.cpp#newcode86 third_party/WebKit/Source/core/dom/AccessibleNode.cpp:86: // Note that there are two valid spellings of ...
3 years, 5 months ago
(2017-07-06 06:34:25 UTC)
#6
https://codereview.chromium.org/2967193003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/AccessibleNode.cpp (right):
https://codereview.chromium.org/2967193003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/AccessibleNode.cpp:86: // Note that there are
two valid spellings of this attribute.
On 2017/07/06 05:37:14, aboxhall wrote:
> Perhaps "allowed" instead of "valid"?
Done.
https://codereview.chromium.org/2967193003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/AccessibleNode.cpp:88: return
aria_labeledbyAttr;
On 2017/07/06 05:37:14, aboxhall wrote:
> I think we should prefer the English spelling here, since that's the actual
> valid spelling in the spec.
Done, I flipped a couple other places in this file so that the English
spelling is now the preferred one and the alternate spelling is the
one also checked just in case.
https://codereview.chromium.org/2967193003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/accessibility/AXObject.cpp (right):
https://codereview.chromium.org/2967193003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:1003:
NameSource(*found_text_alternative, aria_labeledbyAttr));
On 2017/07/06 05:37:14, aboxhall wrote:
> Hm, I think we should have a different source for AOM properties (and should
> there be an AOM version for aria_label as well?)
For aria-label and almost every other place in the code where we were
previously getting an ARIA attribute, we're now calling
GetAOMPropertyOrARIAAttribute,
which allows us to not duplicate all of that code.
I'd be totally open to being more explicit about the AOM in developer tools,
but I think it's beyond the scope of this change since we haven't been doing
it for other properties yet.
Considering AOM in general is still experimental and subject to change, we
may also want to consider holding off on adding "extra" dev tools support,
and defer that until we're closer to wanting to ship this.
dmazzoni
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
3 years, 5 months ago
(2017-07-06 06:39:40 UTC)
#7
3 years, 5 months ago
(2017-07-06 06:55:15 UTC)
#9
lgtm
https://codereview.chromium.org/2967193003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/accessibility/AXObject.cpp (right):
https://codereview.chromium.org/2967193003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:1003:
NameSource(*found_text_alternative, aria_labeledbyAttr));
On 2017/07/06 06:34:25, dmazzoni wrote:
> On 2017/07/06 05:37:14, aboxhall wrote:
> > Hm, I think we should have a different source for AOM properties (and should
> > there be an AOM version for aria_label as well?)
>
> For aria-label and almost every other place in the code where we were
> previously getting an ARIA attribute, we're now calling
> GetAOMPropertyOrARIAAttribute,
> which allows us to not duplicate all of that code.
>
> I'd be totally open to being more explicit about the AOM in developer tools,
> but I think it's beyond the scope of this change since we haven't been doing
> it for other properties yet.
>
> Considering AOM in general is still experimental and subject to change, we
> may also want to consider holding off on adding "extra" dev tools support,
> and defer that until we're closer to wanting to ship this.
Makes sense!
dmazzoni
Hmmm, I just realized that now aria-labelledby is showing up twice. I guess that's pretty ...
3 years, 5 months ago
(2017-07-06 07:31:13 UTC)
#10
Hmmm, I just realized that now aria-labelledby is showing up twice.
I guess that's pretty confusing.
I'm assuming a better short-term solution for it to just have a single
entry for aria-labelledby that's taken from the AOM only if the AOM
property is present...
"name": {
"type": "computedString",
"value": "Select All",
"sources": [
{
"type": "relatedElement",
"attribute": "aria-labeledby"
},
{
"type": "relatedElement",
"attribute": "aria-labelledby"
},
{
"type": "attribute",
"attribute": "aria-label"
},
{
"type": "relatedElement",
"nativeSource": "label"
},
{
"type": "contents",
"value": {
"type": "computedString",
"value": "Select All"
}
},
{
"type": "attribute",
"attribute": "title",
"superseded": true
}
]
},
dmazzoni
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
3 years, 5 months ago
(2017-07-07 07:14:20 UTC)
#11
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/465128) win_clang on ...
3 years, 5 months ago
(2017-07-07 08:05:25 UTC)
#16
CQ is committing da patch. Bot data: {"patchset_id": 60001, "attempt_start_ts": 1499441549875520, "parent_rev": "a22efb271e62974dc0f1016fbb2d00730079eff9", "commit_rev": "0ae8387e4e3efa43990288217ed2f042c78a8dd8"}
3 years, 5 months ago
(2017-07-07 16:32:30 UTC)
#21
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1499441549875520,
"parent_rev": "a22efb271e62974dc0f1016fbb2d00730079eff9", "commit_rev":
"0ae8387e4e3efa43990288217ed2f042c78a8dd8"}
commit-bot: I haz the power
Description was changed from ========== Relation list properties for Accessibility Object Model phase 1 This ...
3 years, 5 months ago
(2017-07-07 16:32:47 UTC)
#22
Message was sent while issue was closed.
Description was changed from
==========
Relation list properties for Accessibility Object Model phase 1
This change implements AOM support for controls, describedBy,
flowTo, labeledBy, and owns. The corresponding ARIA attributes
all take lists of IDREFs, and the AOM properties all take references
to AccessibleNodeLists instead.
BUG=680345
==========
to
==========
Relation list properties for Accessibility Object Model phase 1
This change implements AOM support for controls, describedBy,
flowTo, labeledBy, and owns. The corresponding ARIA attributes
all take lists of IDREFs, and the AOM properties all take references
to AccessibleNodeLists instead.
BUG=680345
Review-Url: https://codereview.chromium.org/2967193003
Cr-Commit-Position: refs/heads/master@{#484950}
Committed:
https://chromium.googlesource.com/chromium/src/+/0ae8387e4e3efa43990288217ed2...
==========
commit-bot: I haz the power
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/0ae8387e4e3efa43990288217ed2f042c78a8dd8
3 years, 5 months ago
(2017-07-07 16:32:49 UTC)
#23
Hi, I am looking at your code: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/AccessibleNode.cpp?rcl=5def8d45e821877c0bf84fd7dbef9a154f96f72c&l=420 value.SimplifyWhiteSpace(); Vector<String> ids; value.Split(' ', ids); SimplyfyWhiteSpace() ...
3 years, 4 months ago
(2017-08-16 21:20:21 UTC)
#25
Message was sent while issue was closed.
Hi, I am looking at your code:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Acces...
value.SimplifyWhiteSpace();
Vector<String> ids;
value.Split(' ', ids);
SimplyfyWhiteSpace() returns a String, but it doesn't modify the feeding String
in place.
Either "value.SimplifyWhiteSpace();" was unnecessary or it should be "value_new
= value.SimplifyWhiteSpace(); ... ; value_new.Split(' ', ids)"
Please take another look at this.
Thanks
Issue 2967193003: Relation list properties for Accessibility Object Model phase 1
(Closed)
Created 3 years, 5 months ago by dmazzoni
Modified 3 years, 4 months ago
Reviewers: Mike West, aboxhall, loonybear
Base URL:
Comments: 7