Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2524313002:
Fix AXLayoutObject not to add duplicated children for CSS first-letter (Closed)
|
Created:
4 years ago by kojii Modified:
4 years ago CC:
aboxhall+watch_chromium.org, aboxhall, blink-reviews, chromium-reviews, dmazzoni+watch_chromium.org, dmazzoni, dtseng+watch_chromium.org, haraken, je_julie, nektar+watch_chromium.org, nektarios, yuzo+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix AXLayoutObject not to add duplicated children for CSS first-letter
This patch fixes AXLayoutObject not to add first-letter pseudo elements
twice in the accessibility tree.
Similar fix to the one in WebKit[1], but the condition was changed only
for first-letter pseudo element because of the difference in the layout
tree structure.
[1] https://trac.webkit.org/changeset/203694/
BUG=632453
Committed: https://crrev.com/de49a2b1783ddecd7c1785287f76a4bbe468fdc2
Cr-Commit-Position: refs/heads/master@{#434657}
Patch Set 1 #Patch Set 2 : Rebaseline first-letter-text-transform-causes-crash-expected.txt #
Total comments: 2
Patch Set 3 : eae review #
Total comments: 1
Patch Set 4 : dmazzoni review #
Messages
Total messages: 39 (28 generated)
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== ax-first-letter BUG= ========== to ========== Fix AXLayoutObject not to add duplicated children for CSS first-letter This patch fixes AXLayoutObject not to add first-letter pseudo elements twice in the accessibility tree. Similar fix to the one in WebKit[1], but the condition was changed only for first-letter pseudo element because of the difference in the layout tree structure. [1] https://trac.webkit.org/changeset/203694/ BUG=632453 ==========
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
kojii@chromium.org changed reviewers: + dmazzoni@chromium.org, eae@chromium.org
PTAL. I'm not very familiar with AXObject, comments appreciated if you noticed anything.
Thanks for working on this! LGTM w/caveat https://codereview.chromium.org/2524313002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp (right): https://codereview.chromium.org/2524313002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:128: firstChild = nullptr; Shouldn't this be return nullptr to ensure that it's not overriden by the next statement?
kojii@chromium.org changed reviewers: + aboxhall@chromium.org
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thank you for your prompt review as always. I'll wait a bit if aboxhall/dmazzoni has anything to say, land if not. https://codereview.chromium.org/2524313002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp (right): https://codereview.chromium.org/2524313002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:128: firstChild = nullptr; On 2016/11/24 at 21:50:13, eae wrote: > Shouldn't this be return nullptr to ensure that it's not overriden by the next statement? Done, thank you.
The CQ bit was checked by kojii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org Link to the patchset: https://codereview.chromium.org/2524313002/#ps40001 (title: "eae review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
AX OWNER review is missing. aboxhall@, dmazzoni@, PTAL.
lgtm Thanks! https://codereview.chromium.org/2524313002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/accessibility/css-first-letter-children.html (right): https://codereview.chromium.org/2524313002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/accessibility/css-first-letter-children.html:29: let children = getAccessibilityChildren(element); This test is reasonable but it's a bit more roundabout than what I would have done. A simpler test would be to use an element that gets its own accessible name, like a heading element, as in the example in the linked bug. If you use <h1 id="text">Test text</h1>, then element.name should equal "Test text" and if the bug report is correct that would have failed before. This doesn't work for a paragraph because paragraph elements don't have their own accessible name, so instead you have to walk the children. Go ahead and leave this test, but I'd include a test above this one in the same file with the simpler assertion.
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you dmazzoni for your review and detailed comments, that really helps. Added the simpler test as suggested. It, however, passes with or without the fix. It looks like element.name is correct, only that it has a first letter child in addition to it having full text, that concatenating them would produce duplicated first letter. Still the test looks good to have to ensure it works too, so included. Thank you for your advice.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kojii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dmazzoni@chromium.org, eae@chromium.org Link to the patchset: https://codereview.chromium.org/2524313002/#ps60001 (title: "dmazzoni review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480344790496720,
"parent_rev": "6aefea63d16dff3df4315e73b9382fe42b9740d6", "commit_rev":
"bbcb701e53ab4841a9e09511a0c429cec9234f78"}
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fix AXLayoutObject not to add duplicated children for CSS first-letter This patch fixes AXLayoutObject not to add first-letter pseudo elements twice in the accessibility tree. Similar fix to the one in WebKit[1], but the condition was changed only for first-letter pseudo element because of the difference in the layout tree structure. [1] https://trac.webkit.org/changeset/203694/ BUG=632453 ========== to ========== Fix AXLayoutObject not to add duplicated children for CSS first-letter This patch fixes AXLayoutObject not to add first-letter pseudo elements twice in the accessibility tree. Similar fix to the one in WebKit[1], but the condition was changed only for first-letter pseudo element because of the difference in the layout tree structure. [1] https://trac.webkit.org/changeset/203694/ BUG=632453 Committed: https://crrev.com/de49a2b1783ddecd7c1785287f76a4bbe468fdc2 Cr-Commit-Position: refs/heads/master@{#434657} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/de49a2b1783ddecd7c1785287f76a4bbe468fdc2 Cr-Commit-Position: refs/heads/master@{#434657} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
