Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2372823002:
Fix a case where the accessible bounding box of an inline text box was wrong. (Closed)
|
Created:
4 years, 2 months ago by dmazzoni Modified:
4 years, 2 months ago CC:
aboxhall+watch_chromium.org, aboxhall, blink-reviews, chromium-reviews, dmazzoni+watch_chromium.org, 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 a case where the accessible bounding box of an inline text box was wrong.
The local bounds of an inline text box is relative to the same box as its
LayoutText, not relative to the LayoutText itself. Subtract the LayoutText
origin from the InlineTextBox coordinates and add some new tests to cover
this case.
BUG=650440
Committed: https://crrev.com/dbdcf389f7e2055c8c986c5ccaec6c46180ee19b
Cr-Commit-Position: refs/heads/master@{#421568}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Remove console.log #
Total comments: 4
Patch Set 3 : Address feedback #Patch Set 4 : Fix bug in bounds calc uncovered by windows test #
Messages
Total messages: 30 (17 generated)
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
dmazzoni@chromium.org changed reviewers: + aboxhall@chromium.org, chrishtr@chromium.org
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_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2372823002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/accessibility/bounds-calc.html (right): https://codereview.chromium.org/2372823002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/accessibility/bounds-calc.html:81: console.log(text.data); Remove this. https://codereview.chromium.org/2372823002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/accessibility/AXInlineTextBox.cpp (right): https://codereview.chromium.org/2372823002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/accessibility/AXInlineTextBox.cpp:69: if (!m_inlineTextBox || !parentObject() || !parentObject()->getLayoutObject()) Why are these conditionals needed?
https://codereview.chromium.org/2372823002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/accessibility/bounds-calc.html (right): https://codereview.chromium.org/2372823002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/accessibility/bounds-calc.html:81: console.log(text.data); On 2016/09/26 at 23:42:51, chrishtr wrote: > Remove this. Done https://codereview.chromium.org/2372823002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/accessibility/AXInlineTextBox.cpp (right): https://codereview.chromium.org/2372823002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/accessibility/AXInlineTextBox.cpp:69: if (!m_inlineTextBox || !parentObject() || !parentObject()->getLayoutObject()) On 2016/09/26 at 23:42:51, chrishtr wrote: > Why are these conditionals needed? In general, accessibility objects outlive layout objects and this code could get triggered during teardown, so I'd rather not assume parentObject() or parentObject->getLayoutObject() must be valid if we have m_inlineTextBox. Does it seem redundant?
The CQ bit was checked by dmazzoni@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...
On 2016/09/27 at 16:07:47, dmazzoni wrote: > https://codereview.chromium.org/2372823002/diff/1/third_party/WebKit/LayoutTe... > File third_party/WebKit/LayoutTests/accessibility/bounds-calc.html (right): > > https://codereview.chromium.org/2372823002/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/accessibility/bounds-calc.html:81: console.log(text.data); > On 2016/09/26 at 23:42:51, chrishtr wrote: > > Remove this. > > Done > > https://codereview.chromium.org/2372823002/diff/1/third_party/WebKit/Source/m... > File third_party/WebKit/Source/modules/accessibility/AXInlineTextBox.cpp (right): > > https://codereview.chromium.org/2372823002/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/accessibility/AXInlineTextBox.cpp:69: if (!m_inlineTextBox || !parentObject() || !parentObject()->getLayoutObject()) > On 2016/09/26 at 23:42:51, chrishtr wrote: > > Why are these conditionals needed? > > In general, accessibility objects outlive layout objects and > this code could get triggered during teardown, so I'd rather > not assume parentObject() or parentObject->getLayoutObject() > must be valid if we have m_inlineTextBox. > > Does it seem redundant? Well I'm just wondering why you added it in this patch, when it apparently was ok before. I don't have strong feelings about it.
lgtm
On 2016/09/27 16:49:30, chrishtr wrote: > On 2016/09/27 at 16:07:47, dmazzoni wrote: > > > https://codereview.chromium.org/2372823002/diff/1/third_party/WebKit/LayoutTe... > > File third_party/WebKit/LayoutTests/accessibility/bounds-calc.html (right): > > > > > https://codereview.chromium.org/2372823002/diff/1/third_party/WebKit/LayoutTe... > > third_party/WebKit/LayoutTests/accessibility/bounds-calc.html:81: > console.log(text.data); > > On 2016/09/26 at 23:42:51, chrishtr wrote: > > > Remove this. > > > > Done > > > > > https://codereview.chromium.org/2372823002/diff/1/third_party/WebKit/Source/m... > > File third_party/WebKit/Source/modules/accessibility/AXInlineTextBox.cpp > (right): > > > > > https://codereview.chromium.org/2372823002/diff/1/third_party/WebKit/Source/m... > > third_party/WebKit/Source/modules/accessibility/AXInlineTextBox.cpp:69: if > (!m_inlineTextBox || !parentObject() || !parentObject()->getLayoutObject()) > > On 2016/09/26 at 23:42:51, chrishtr wrote: > > > Why are these conditionals needed? > > > > In general, accessibility objects outlive layout objects and > > this code could get triggered during teardown, so I'd rather > > not assume parentObject() or parentObject->getLayoutObject() > > must be valid if we have m_inlineTextBox. > > > > Does it seem redundant? > > Well I'm just wondering why you added it in this patch, when it apparently was > ok before. I don't have strong feelings about it. Ah - I added it just to guard access to those pointers below, but what might not be clear is why I added the checks above rather than immediately before using the pointers - that's just because I want to either set all output variables, or none of them.
lgtm https://codereview.chromium.org/2372823002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/accessibility/bounds-calc.html (right): https://codereview.chromium.org/2372823002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/accessibility/bounds-calc.html:137: assertStaticTextChildDOMRectSameAsAXRect("twolines", 2); What's the second child, out of curiosity? https://codereview.chromium.org/2372823002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/accessibility/AXInlineTextBox.cpp (right): https://codereview.chromium.org/2372823002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/accessibility/AXInlineTextBox.cpp:75: outBoundsInContainer.moveBy(-parentLayoutObject->localBoundingBoxRectForAccessibility().location()); This is breaking my brain a bit. Would it be ok to move parentLayoutObject->localBoundingBoxRectForAccessibility().location() out into a local var just for clarity as to what the unary - is applying to?
https://codereview.chromium.org/2372823002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/accessibility/bounds-calc.html (right): https://codereview.chromium.org/2372823002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/accessibility/bounds-calc.html:137: assertStaticTextChildDOMRectSameAsAXRect("twolines", 2); On 2016/09/27 17:02:48, aboxhall wrote: > What's the second child, out of curiosity? The <br> element. I'll add a comment. https://codereview.chromium.org/2372823002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/accessibility/AXInlineTextBox.cpp (right): https://codereview.chromium.org/2372823002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/accessibility/AXInlineTextBox.cpp:75: outBoundsInContainer.moveBy(-parentLayoutObject->localBoundingBoxRectForAccessibility().location()); On 2016/09/27 17:02:48, aboxhall wrote: > This is breaking my brain a bit. Would it be ok to move > parentLayoutObject->localBoundingBoxRectForAccessibility().location() out into a > local var just for clarity as to what the unary - is applying to? Sure, done.
The CQ bit was checked by dmazzoni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrishtr@chromium.org, aboxhall@chromium.org Link to the patchset: https://codereview.chromium.org/2372823002/#ps40001 (title: "Address feedback")
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by dmazzoni@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: This issue passed the CQ dry run.
The CQ bit was checked by dmazzoni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrishtr@chromium.org, aboxhall@chromium.org Link to the patchset: https://codereview.chromium.org/2372823002/#ps60001 (title: "Fix bug in bounds calc uncovered by windows test")
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.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fix a case where the accessible bounding box of an inline text box was wrong. The local bounds of an inline text box is relative to the same box as its LayoutText, not relative to the LayoutText itself. Subtract the LayoutText origin from the InlineTextBox coordinates and add some new tests to cover this case. BUG=650440 ========== to ========== Fix a case where the accessible bounding box of an inline text box was wrong. The local bounds of an inline text box is relative to the same box as its LayoutText, not relative to the LayoutText itself. Subtract the LayoutText origin from the InlineTextBox coordinates and add some new tests to cover this case. BUG=650440 Committed: https://crrev.com/dbdcf389f7e2055c8c986c5ccaec6c46180ee19b Cr-Commit-Position: refs/heads/master@{#421568} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/dbdcf389f7e2055c8c986c5ccaec6c46180ee19b Cr-Commit-Position: refs/heads/master@{#421568} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
