Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2478083002:
Add more topics to accessiiblity documentation. (Closed)
|
Created:
4 years, 1 month ago by dmazzoni Modified:
4 years, 1 month ago CC:
chromium-reviews, aboxhall+watch_chromium.org, nektar+watch_chromium.org, yuzo+watch_chromium.org, je_julie, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd more topics to accessiiblity documentation.
Thanks to Elly for writing a great first draft of
technical documentation. I took a stab at introducing
a lot of the key higher-level concepts that provide
important context for understanding the rest of the
code.
BUG=none
NOTRY=true
Committed: https://crrev.com/c6ff030caf54656a3e6fd3e56de167d17c1b945c
Cr-Commit-Position: refs/heads/master@{#430033}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Address feedback #Messages
Total messages: 13 (6 generated)
dmazzoni@chromium.org changed reviewers: + aboxhall@chromium.org, ellyjones@chromium.org
lgtm Thanks so much for writing this! I learned a great deal from reading it :) https://codereview.chromium.org/2478083002/diff/1/docs/accessibility.md File docs/accessibility.md (right): https://codereview.chromium.org/2478083002/diff/1/docs/accessibility.md#newco... docs/accessibility.md:106: if you call accessibilityRole(), it returns @"AXWebArea", and if you objc methods are generally referenced like -[NSAccessibility accessibilityRole], mimicking the call syntax https://codereview.chromium.org/2478083002/diff/1/docs/accessibility.md#newco... docs/accessibility.md:108: what (if anything) happens on Linux? https://codereview.chromium.org/2478083002/diff/1/docs/accessibility.md#newco... docs/accessibility.md:146: a live region. The point is just to illustrate that the concepts are similar, is this last sentence a meta-comment on the rest of the paragraph?
lgtm This is amazing. Some suggestions but generally lgtm. https://codereview.chromium.org/2478083002/diff/1/docs/accessibility.md File docs/accessibility.md (right): https://codereview.chromium.org/2478083002/diff/1/docs/accessibility.md#newcode8 docs/accessibility.md:8: However, the majority of accessibility code in Chromium is concerned with Rather than "the majority of accessibility code in Chromium", can we say "the code in Chromium which is explicitly referred to as accessibility code, usually in part of the file name or path"? Reasoning being that e.g. all the code dealing with browser focus is arguably "accessibility" code, as is your contrast code, but that code isn't named as such, and the code which is named as such has a narrower focus and that focus is what is being discussed here. https://codereview.chromium.org/2478083002/diff/1/docs/accessibility.md#newco... docs/accessibility.md:12: Assistive technology includes: How about "'Assistive technology' here refers to software or hardware which makes use of these APIs to create an alternative interface for the user which accommodates some specific needs, for example:" or similar? https://codereview.chromium.org/2478083002/diff/1/docs/accessibility.md#newco... docs/accessibility.md:180: ## Chromium's multi-process architecture A diagram somewhere in here would be very helpful, I think. https://codereview.chromium.org/2478083002/diff/1/docs/accessibility.md#newco... docs/accessibility.md:280: The tree serializer doesn't know anything about accessibility attributes. Would it be dangerous to point to the specific locations in code for the concepts discussed in these sections?
https://codereview.chromium.org/2478083002/diff/1/docs/accessibility.md File docs/accessibility.md (right): https://codereview.chromium.org/2478083002/diff/1/docs/accessibility.md#newcode8 docs/accessibility.md:8: However, the majority of accessibility code in Chromium is concerned with On 2016/11/04 17:02:07, aboxhall wrote: > Rather than "the majority of accessibility code in Chromium", can we say "the > code in Chromium which is explicitly referred to as accessibility code, usually > in part of the file name or path"? > > Reasoning being that e.g. all the code dealing with browser focus is arguably > "accessibility" code, as is your contrast code, but that code isn't named as > such, and the code which is named as such has a narrower focus and that focus is > what is being discussed here. Done. https://codereview.chromium.org/2478083002/diff/1/docs/accessibility.md#newco... docs/accessibility.md:12: Assistive technology includes: On 2016/11/04 17:02:07, aboxhall wrote: > How about "'Assistive technology' here refers to software or hardware which > makes use of these APIs to create an alternative interface for the user which > accommodates some specific needs, for example:" or similar? Done. https://codereview.chromium.org/2478083002/diff/1/docs/accessibility.md#newco... docs/accessibility.md:106: if you call accessibilityRole(), it returns @"AXWebArea", and if you On 2016/11/04 15:59:06, Elly Fong-Jones wrote: > objc methods are generally referenced like -[NSAccessibility accessibilityRole], > mimicking the call syntax Good idea https://codereview.chromium.org/2478083002/diff/1/docs/accessibility.md#newco... docs/accessibility.md:108: On 2016/11/04 15:59:06, Elly Fong-Jones wrote: > what (if anything) happens on Linux? Done. https://codereview.chromium.org/2478083002/diff/1/docs/accessibility.md#newco... docs/accessibility.md:146: a live region. The point is just to illustrate that the concepts are similar, On 2016/11/04 15:59:06, Elly Fong-Jones wrote: > is this last sentence a meta-comment on the rest of the paragraph? Yes, I tried to clarify it a bit. I just didn't want someone to think that was supposed to be a complete explanation of live region events. https://codereview.chromium.org/2478083002/diff/1/docs/accessibility.md#newco... docs/accessibility.md:180: ## Chromium's multi-process architecture On 2016/11/04 17:02:07, aboxhall wrote: > A diagram somewhere in here would be very helpful, I think. Sounds great, but I'll save that for a future revision. In the meantime, I added a link to the design doc on chromium.org, which does have a diagram. https://codereview.chromium.org/2478083002/diff/1/docs/accessibility.md#newco... docs/accessibility.md:280: The tree serializer doesn't know anything about accessibility attributes. On 2016/11/04 17:02:07, aboxhall wrote: > Would it be dangerous to point to the specific locations in code for the > concepts discussed in these sections? My plan was to cover all of the concepts at the top *without* technical details like source directory locations and class names, since those tend to change much more frequently than the key underlying concepts do.
Description was changed from ========== Add more topics to accessiiblity documentation. Thanks to Elly for writing a great first draft of technical documentation. I took a stab at introducing a lot of the key higher-level concepts that provide important context for understanding the rest of the code. BUG=none ========== to ========== Add more topics to accessiiblity documentation. Thanks to Elly for writing a great first draft of technical documentation. I took a stab at introducing a lot of the key higher-level concepts that provide important context for understanding the rest of the code. BUG=none NOTRY=true ==========
The CQ bit was checked by dmazzoni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ellyjones@chromium.org, aboxhall@chromium.org Link to the patchset: https://codereview.chromium.org/2478083002/#ps20001 (title: "Address feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add more topics to accessiiblity documentation. Thanks to Elly for writing a great first draft of technical documentation. I took a stab at introducing a lot of the key higher-level concepts that provide important context for understanding the rest of the code. BUG=none NOTRY=true ========== to ========== Add more topics to accessiiblity documentation. Thanks to Elly for writing a great first draft of technical documentation. I took a stab at introducing a lot of the key higher-level concepts that provide important context for understanding the rest of the code. BUG=none NOTRY=true ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Add more topics to accessiiblity documentation. Thanks to Elly for writing a great first draft of technical documentation. I took a stab at introducing a lot of the key higher-level concepts that provide important context for understanding the rest of the code. BUG=none NOTRY=true ========== to ========== Add more topics to accessiiblity documentation. Thanks to Elly for writing a great first draft of technical documentation. I took a stab at introducing a lot of the key higher-level concepts that provide important context for understanding the rest of the code. BUG=none NOTRY=true Committed: https://crrev.com/c6ff030caf54656a3e6fd3e56de167d17c1b945c Cr-Commit-Position: refs/heads/master@{#430033} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/c6ff030caf54656a3e6fd3e56de167d17c1b945c Cr-Commit-Position: refs/heads/master@{#430033} |
