Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(38)

Issue 1525793002: Fall back to title element text for SVG root text alternatives (Closed)

Created:
5 years ago by Elly Fong-Jones
Modified:
4 years, 11 months ago
Reviewers:
dmazzoni, aboxhall
CC:
chromium-reviews, plundblad+watch_chromium.org, aboxhall+watch_chromium.org, nektar+watch_chromium.org, yuzo+watch_chromium.org, nektarios, je_julie, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, blink-reviews, dmazzoni
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Implement the modifications to the accessible name computation algorithm described by the SVG AAM, and change the default role of SVG roots from "unknown" to "group". TEST=layout BUG=566252 Committed: https://crrev.com/2d9269a857221f01a4510ecdb161a8f6fa63f7a2 Cr-Commit-Position: refs/heads/master@{#369410}

Patch Set 1 #

Patch Set 2 : + AXSVGRoot override #

Total comments: 3

Patch Set 3 : implement computeAccessibilityIsIgnored #

Patch Set 4 : Add role=group default for AXSVGRoot #

Total comments: 5

Patch Set 5 : Clone fieldset/legend code a little #

Total comments: 4

Patch Set 6 : Remember relatedObjects #

Total comments: 2

Patch Set 7 : nit for dmazzoni #

Unified diffs Side-by-side diffs Delta from patch set Stats (+60 lines, -8 lines) Patch
A + third_party/WebKit/LayoutTests/accessibility/name-calc-svg.html View 1 2 1 chunk +7 lines, -8 lines 0 comments Download
M third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp View 1 2 3 4 5 6 2 chunks +36 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/accessibility/AXSVGRoot.h View 1 2 3 1 chunk +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/accessibility/AXSVGRoot.cpp View 1 2 3 1 chunk +14 lines, -0 lines 0 comments Download

Messages

Total messages: 32 (11 generated)
aboxhall
https://codereview.chromium.org/1525793002/diff/20001/third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right): https://codereview.chromium.org/1525793002/diff/20001/third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp#newcode2375 third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:2375: Element* title = nullptr; Before this line, add: nameFrom ...
5 years ago (2015-12-15 19:56:03 UTC) #2
dmazzoni
https://codereview.chromium.org/1525793002/diff/20001/third_party/WebKit/Source/modules/accessibility/AXSVGRoot.cpp File third_party/WebKit/Source/modules/accessibility/AXSVGRoot.cpp (right): https://codereview.chromium.org/1525793002/diff/20001/third_party/WebKit/Source/modules/accessibility/AXSVGRoot.cpp#newcode71 third_party/WebKit/Source/modules/accessibility/AXSVGRoot.cpp:71: bool AXSVGRoot::computeAccessibilityIsIgnored(IgnoredReasons* reasons) const On 2015/12/15 19:56:03, aboxhall wrote: ...
5 years ago (2015-12-15 20:00:45 UTC) #4
aboxhall
On 2015/12/15 20:00:45, dmazzoni wrote: > https://codereview.chromium.org/1525793002/diff/20001/third_party/WebKit/Source/modules/accessibility/AXSVGRoot.cpp > File third_party/WebKit/Source/modules/accessibility/AXSVGRoot.cpp (right): > > https://codereview.chromium.org/1525793002/diff/20001/third_party/WebKit/Source/modules/accessibility/AXSVGRoot.cpp#newcode71 > ...
5 years ago (2015-12-15 20:46:51 UTC) #5
dmazzoni
Oh, good point. Yeah, it should extend the existing algorithm rather than always returning false. ...
5 years ago (2015-12-15 20:57:39 UTC) #6
dmazzoni
Oh, good point. Yeah, it should extend the existing algorithm rather than always returning false. ...
5 years ago (2015-12-15 20:57:39 UTC) #7
Elly Fong-Jones
aboxhall, dmazzoni: ptal? the tests all pass now :)
4 years, 11 months ago (2016-01-05 14:34:51 UTC) #9
aboxhall
https://codereview.chromium.org/1525793002/diff/60001/third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right): https://codereview.chromium.org/1525793002/diff/60001/third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp#newcode2390 third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:2390: for (Element& element : ElementTraversal::descendantsOf(*(node()))) { Could this be ...
4 years, 11 months ago (2016-01-06 17:04:07 UTC) #10
Elly Fong-Jones
https://codereview.chromium.org/1525793002/diff/60001/third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right): https://codereview.chromium.org/1525793002/diff/60001/third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp#newcode2390 third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:2390: for (Element& element : ElementTraversal::descendantsOf(*(node()))) { On 2016/01/06 17:04:07, ...
4 years, 11 months ago (2016-01-07 18:46:01 UTC) #11
aboxhall
https://codereview.chromium.org/1525793002/diff/80001/third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right): https://codereview.chromium.org/1525793002/diff/80001/third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp#newcode2390 third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:2390: Element* title = ElementTraversal::firstChild( I just now thought to ...
4 years, 11 months ago (2016-01-07 18:55:26 UTC) #12
Elly Fong-Jones
PTAL? https://codereview.chromium.org/1525793002/diff/80001/third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right): https://codereview.chromium.org/1525793002/diff/80001/third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp#newcode2390 third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:2390: Element* title = ElementTraversal::firstChild( On 2016/01/07 18:55:26, aboxhall ...
4 years, 11 months ago (2016-01-08 14:13:16 UTC) #13
aboxhall
lgtm
4 years, 11 months ago (2016-01-08 15:13:21 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1525793002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1525793002/100001
4 years, 11 months ago (2016-01-08 15:19:56 UTC) #16
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/133686)
4 years, 11 months ago (2016-01-08 15:27:57 UTC) #18
Elly Fong-Jones
dmazzoni: ptal? this needs owner review :)
4 years, 11 months ago (2016-01-11 14:05:20 UTC) #19
dmazzoni
lgtm https://codereview.chromium.org/1525793002/diff/100001/third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right): https://codereview.chromium.org/1525793002/diff/100001/third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp#newcode2389 third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:2389: DCHECK(node()->isContainerNode()); This should be ASSERT
4 years, 11 months ago (2016-01-12 16:23:39 UTC) #20
Elly Fong-Jones
thanks :) https://codereview.chromium.org/1525793002/diff/100001/third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right): https://codereview.chromium.org/1525793002/diff/100001/third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp#newcode2389 third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:2389: DCHECK(node()->isContainerNode()); On 2016/01/12 16:23:38, dmazzoni wrote: > ...
4 years, 11 months ago (2016-01-13 16:51:51 UTC) #21
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1525793002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1525793002/120001
4 years, 11 months ago (2016-01-13 16:53:10 UTC) #23
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 11 months ago (2016-01-13 17:58:12 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1525793002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1525793002/120001
4 years, 11 months ago (2016-01-14 14:22:17 UTC) #28
commit-bot: I haz the power
Committed patchset #7 (id:120001)
4 years, 11 months ago (2016-01-14 14:25:56 UTC) #30
commit-bot: I haz the power
4 years, 11 months ago (2016-01-14 14:27:07 UTC) #32
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/2d9269a857221f01a4510ecdb161a8f6fa63f7a2
Cr-Commit-Position: refs/heads/master@{#369410}

Powered by Google App Engine
This is Rietveld 408576698