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

Issue 2833843005: Handling of different types of empty alt (Closed)

Created:
3 years, 8 months ago by aleventhal
Modified:
3 years, 7 months ago
Reviewers:
dmazzoni
CC:
chromium-reviews, aboxhall+watch_chromium.org, nektar+watch_chromium.org, jam, yuzo+watch_chromium.org, dougt+watch_chromium.org, darin-cc_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, je_julie
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Handle <img alt> and <img alt=""> as purposefully empty alt. BUG=701747, 708192, 710537 Review-Url: https://codereview.chromium.org/2833843005 Cr-Commit-Position: refs/heads/master@{#469050} Committed: https://chromium.googlesource.com/chromium/src/+/7ae4d36b7f3b88d7998cc86c2fc6a7245ccdba96

Patch Set 1 #

Patch Set 2 : Add tests which won't pass yet #

Patch Set 3 : Fix test #

Patch Set 4 : Update windows expected results for empty alt #

Patch Set 5 : Another round on tests #

Patch Set 6 : First attempt #

Patch Set 7 : Fix test #

Patch Set 8 : Try again #

Patch Set 9 : Try again #

Patch Set 10 : git cl format #

Patch Set 11 : Image tests with and without link, add img-expected-blink.txt #

Patch Set 12 : Address android, also update tests, use common method to make code more readable #

Patch Set 13 : Fix compilation #

Patch Set 14 : Fix spelling error in name #

Patch Set 15 : Fix spelling error in name #

Patch Set 16 : Fix tests #

Patch Set 17 : Attempt to fix tests by allowing empty name to be written to dump tree tests #

Patch Set 18 : Fix compiler error #

Patch Set 19 : Fix compiler error #

Patch Set 20 : Try @win-allow-empty #

Patch Set 21 : Fix android test expectations #

Patch Set 22 : Use @WIN-ALLOW_EMPTY:name instead of custom solution #

Patch Set 23 : Try name* with @WIN-ALLOW_EMPTY #

Patch Set 24 : Temporary logging #

Patch Set 25 : Try putting default filters at end so that @WIN-ALLOW-EMPTY:name works #

Patch Set 26 : Always allow empty name #

Patch Set 27 : Add temporary logging #

Patch Set 28 : Add more temporary logging #

Patch Set 29 : Fix compiler error #

Patch Set 30 : Logging again #

Patch Set 31 : Try switching order of allow empty name default rule #

Patch Set 32 : git cl try #

Patch Set 33 : Address Auralinux #

Patch Set 34 : Keep DENY empty string rule but put allow-empty name rules after it #

Patch Set 35 : Logging for auralinux issue #

Patch Set 36 : More logging #

Patch Set 37 : git cl try #

Patch Set 38 : Try again #

Patch Set 39 : More logging #

Patch Set 40 : Log corrupted memory #

Patch Set 41 : Log corrupted memory #

Patch Set 42 : Give up on passing back NULL for auralinux, for now #

Patch Set 43 : Temporary test fix until we figure out why returning null for name does not work #

Total comments: 1

Patch Set 44 : Remove auralinux changes #

Patch Set 45 : Auralinux test failures are now officially a mystery, revert last change in related code to see if … #

Patch Set 46 : One more try to do Linux the right way #

Total comments: 11

Patch Set 47 : Address review nits #

Patch Set 48 : Up-to-date with master #

Patch Set 49 : Fix build error #

Patch Set 50 : Fix whitespace #

Patch Set 51 : Add logging, try different string function #

Patch Set 52 : Fix compiler error #

Patch Set 53 : Fix logging #

Patch Set 54 : git cl try #

Patch Set 55 : More satisfying approach #

Patch Set 56 : Let's see what this does #

Patch Set 57 : Logging -- 99 problems #

Patch Set 58 : New idea #

Patch Set 59 : Should be fine finally #

Patch Set 60 : Ready to land #

Unified diffs Side-by-side diffs Delta from patch set Stats (+203 lines, -43 lines) Patch
M chrome/common/extensions/api/automation.idl View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/accessibility/accessibility_tree_formatter_auralinux.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 44 45 2 chunks +11 lines, -6 lines 0 comments Download
M content/browser/accessibility/accessibility_tree_formatter_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 1 chunk +2 lines, -1 line 0 comments Download
M content/browser/accessibility/browser_accessibility.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +3 lines, -0 lines 0 comments Download
M content/browser/accessibility/browser_accessibility.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 1 chunk +5 lines, -0 lines 0 comments Download
M content/browser/accessibility/browser_accessibility_android.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 1 chunk +2 lines, -1 line 0 comments Download
M content/browser/accessibility/browser_accessibility_auralinux.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 1 chunk +4 lines, -0 lines 0 comments Download
M content/browser/accessibility/browser_accessibility_cocoa.mm View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -1 line 0 comments Download
M content/browser/accessibility/browser_accessibility_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 1 chunk +0 lines, -3 lines 0 comments Download
M content/browser/accessibility/dump_accessibility_tree_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 2 chunks +8 lines, -1 line 0 comments Download
M content/renderer/accessibility/blink_ax_enum_conversion.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 1 chunk +2 lines, -0 lines 0 comments Download
M content/renderer/accessibility/blink_ax_tree_source.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 1 chunk +2 lines, -1 line 0 comments Download
M content/shell/test_runner/web_ax_object_proxy.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 1 chunk +2 lines, -0 lines 0 comments Download
M content/test/data/accessibility/html/img-empty-alt.html View 1 2 3 4 5 6 7 8 9 10 20 21 22 23 24 25 1 chunk +5 lines, -6 lines 0 comments Download
A content/test/data/accessibility/html/img-empty-alt-expected-android.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +6 lines, -0 lines 0 comments Download
M content/test/data/accessibility/html/img-empty-alt-expected-blink.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 1 chunk +4 lines, -7 lines 0 comments Download
M content/test/data/accessibility/html/img-empty-alt-expected-mac.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +4 lines, -5 lines 0 comments Download
A content/test/data/accessibility/html/img-empty-alt-expected-win.txt View 1 2 3 4 5 6 7 8 9 10 1 chunk +6 lines, -0 lines 0 comments Download
M content/test/data/accessibility/html/img-expected-android.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +1 line, -1 line 0 comments Download
A content/test/data/accessibility/html/img-expected-blink.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 1 chunk +5 lines, -0 lines 0 comments Download
M content/test/data/accessibility/html/img-expected-mac.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -1 line 0 comments Download
M content/test/data/accessibility/html/img-expected-win.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 1 chunk +1 line, -1 line 0 comments Download
A + content/test/data/accessibility/html/img-link-empty-alt.html View 1 2 3 4 5 6 7 8 9 10 22 23 24 25 1 chunk +6 lines, -0 lines 0 comments Download
A content/test/data/accessibility/html/img-link-empty-alt-expected-android.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +6 lines, -0 lines 0 comments Download
A content/test/data/accessibility/html/img-link-empty-alt-expected-blink.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 1 chunk +17 lines, -0 lines 0 comments Download
A + content/test/data/accessibility/html/img-link-empty-alt-expected-mac.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +6 lines, -0 lines 0 comments Download
A content/test/data/accessibility/html/img-link-empty-alt-expected-win.txt View 1 2 3 4 5 6 7 8 9 10 1 chunk +13 lines, -0 lines 0 comments Download
A content/test/data/accessibility/html/picture.html View 1 chunk +39 lines, -0 lines 0 comments Download
A content/test/data/accessibility/html/picture-expected-android.txt View 1 chunk +7 lines, -0 lines 0 comments Download
A content/test/data/accessibility/html/picture-expected-mac.txt View 1 chunk +7 lines, -0 lines 0 comments Download
A content/test/data/accessibility/html/picture-expected-win.txt View 1 chunk +7 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 3 chunks +12 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/modules/accessibility/AXObject.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/accessibility/InspectorTypeBuilderHelper.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/web/AssertMatchingEnums.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/public/web/WebAXEnums.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 1 chunk +1 line, -0 lines 0 comments Download
M ui/accessibility/ax_enums.idl View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 1 chunk +1 line, -0 lines 0 comments Download
M ui/accessibility/platform/ax_platform_node_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 2 chunks +1 line, -3 lines 0 comments Download

Messages

Total messages: 242 (226 generated)
aleventhal
Seeking review https://codereview.chromium.org/2833843005/diff/840001/content/browser/accessibility/browser_accessibility_cocoa.mm File content/browser/accessibility/browser_accessibility_cocoa.mm (right): https://codereview.chromium.org/2833843005/diff/840001/content/browser/accessibility/browser_accessibility_cocoa.mm#newcode863 content/browser/accessibility/browser_accessibility_cocoa.mm:863: if (browserAccessibility_->HasExplicitlyEmptyName()) We already checked for empty ...
3 years, 7 months ago (2017-05-01 17:33:17 UTC) #174
dmazzoni
Can you see if bug 710537 will be fixed by this change too?
3 years, 7 months ago (2017-05-01 17:41:18 UTC) #176
dmazzoni
lgtm Looks good, only nits! https://codereview.chromium.org/2833843005/diff/900001/content/browser/accessibility/browser_accessibility_win.cc File content/browser/accessibility/browser_accessibility_win.cc (right): https://codereview.chromium.org/2833843005/diff/900001/content/browser/accessibility/browser_accessibility_win.cc#newcode680 content/browser/accessibility/browser_accessibility_win.cc:680: // Explicitly empty names ...
3 years, 7 months ago (2017-05-01 17:49:23 UTC) #178
aleventhal
On 2017/05/01 17:41:18, dmazzoni wrote: > Can you see if bug 710537 will be fixed ...
3 years, 7 months ago (2017-05-01 18:40:03 UTC) #180
aleventhal
On 2017/05/01 17:41:18, dmazzoni wrote: > Can you see if bug 710537 will be fixed ...
3 years, 7 months ago (2017-05-01 18:40:06 UTC) #181
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2833843005/920001
3 years, 7 months ago (2017-05-01 18:45:36 UTC) #185
commit-bot: I haz the power
Failed to apply patch for content/browser/accessibility/browser_accessibility_win.cc: While running git apply --index -3 -p1; error: patch ...
3 years, 7 months ago (2017-05-01 22:35:36 UTC) #187
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2833843005/950001
3 years, 7 months ago (2017-05-02 03:08:32 UTC) #194
commit-bot: I haz the power
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/434090)
3 years, 7 months ago (2017-05-02 03:43:02 UTC) #196
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2833843005/970001
3 years, 7 months ago (2017-05-02 09:39:50 UTC) #199
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_compile_dbg_ng/builds/287713) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, ...
3 years, 7 months ago (2017-05-02 09:52:18 UTC) #201
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2833843005/970001
3 years, 7 months ago (2017-05-02 10:32:36 UTC) #203
commit-bot: I haz the power
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/416768)
3 years, 7 months ago (2017-05-02 12:40:28 UTC) #205
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2833843005/1170001
3 years, 7 months ago (2017-05-03 16:24:57 UTC) #238
commit-bot: I haz the power
Committed patchset #60 (id:1170001) as https://chromium.googlesource.com/chromium/src/+/7ae4d36b7f3b88d7998cc86c2fc6a7245ccdba96
3 years, 7 months ago (2017-05-03 18:47:03 UTC) #241
aleventhal
3 years, 7 months ago (2017-05-17 17:45:47 UTC) #242
Message was sent while issue was closed.
https://codereview.chromium.org/2833843005/diff/900001/content/browser/access...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://codereview.chromium.org/2833843005/diff/900001/content/browser/access...
content/browser/accessibility/browser_accessibility_win.cc:680: // Explicitly
empty names like <img alt=""> will return return ""/S_OK
On 2017/05/01 17:49:23, dmazzoni wrote:
> delete /S_OK

Done.

https://codereview.chromium.org/2833843005/diff/900001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/2833843005/diff/900001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:2638: const
bool isEmpty = alt.IsEmpty() && !alt.IsNull();
On 2017/05/01 17:49:23, dmazzoni wrote:
> nit: isEmpty -> is_empty (just as you were getting used
> to the style! At least it's consistent everywhere now)

What would I do without you! :)

https://codereview.chromium.org/2833843005/diff/900001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXObject.h (right):

https://codereview.chromium.org/2833843005/diff/900001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXObject.h:365:
kAXNameFromExplicitlyEmptyAttribute,
On 2017/05/01 17:49:23, dmazzoni wrote:
> Sort

Done.

https://codereview.chromium.org/2833843005/diff/900001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXObject.h:365:
kAXNameFromExplicitlyEmptyAttribute,
On 2017/05/01 17:49:23, dmazzoni wrote:
> Sort

I changed enum names to AttributeExplicitly empty, so that it's both sorted and
the related ideas are adjacent.

https://codereview.chromium.org/2833843005/diff/900001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebAXEnums.h (right):

https://codereview.chromium.org/2833843005/diff/900001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebAXEnums.h:326:
kWebAXNameFromExplicitlyEmptyAttribute,
On 2017/05/01 17:49:23, dmazzoni wrote:
> Sort these - put it after Contents

I changed enum names to AttributeExplicitlyEmpty, so that it's both sorted and
the related ideas are adjacent.

https://codereview.chromium.org/2833843005/diff/900001/ui/accessibility/ax_en...
File ui/accessibility/ax_enums.idl (right):

https://codereview.chromium.org/2833843005/diff/900001/ui/accessibility/ax_en...
ui/accessibility/ax_enums.idl:610: explicitly_empty_attribute,
On 2017/05/01 17:49:23, dmazzoni wrote:
> sort these

Changed to attribute_explicitly_empty and kept order so that related ideas could
be adjacent.

Powered by Google App Engine
This is Rietveld 408576698