Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2611993003:
[DevTools] Accessibility pane: Sync highlight between a11y pane and DOM pane (Closed)
|
Created:
3 years, 11 months ago by aboxhall Modified:
3 years, 11 months ago Reviewers:
dgozman CC:
chromium-reviews, caseq+blink_chromium.org, haraken, aboxhall+watch_chromium.org, nektar+watch_chromium.org, lushnikov+blink_chromium.org, yuzo+watch_chromium.org, pfeldman+blink_chromium.org, nektarios, je_julie, apavlov+blink_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, devtools-reviews_chromium.org, blink-reviews, pfeldman, dmazzoni, kozyatinskiy+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[DevTools] Accessibility pane: Sync highlight between a11y pane and DOM pane
BUG=678480
Review-Url: https://codereview.chromium.org/2611993003
Cr-Commit-Position: refs/heads/master@{#445600}
Committed: https://chromium.googlesource.com/chromium/src/+/b11b7e7c8ade6d7ac945e772dc5776b2fe663bf8
Patch Set 1 : rebase #
Total comments: 4
Patch Set 2 : rebase #Patch Set 3 : rebase #Patch Set 4 : revert ElementsTreeElement #Patch Set 5 : rebase #
Messages
Total messages: 29 (17 generated)
Patchset #1 (id:1) has been deleted
aboxhall@chromium.org changed reviewers: + dgozman@chromium.org
See bug for screenshot. Would love any ideas you have on how to implement this nicely.
Description was changed from ========== First pass syncing highlight between a11y pane and DOM pane BUG=678480 ========== to ========== [DevTools] Accessibility pane: First pass syncing highlight between a11y pane and DOM pane BUG=678480 ==========
On 2017/01/05 05:10:57, aboxhall wrote: > See bug for screenshot. Would love any ideas you have on how to implement this > nicely. dgozman@: are you back in the office at this point? Just bumping this up in case you have a chance to look at it.
https://codereview.chromium.org/2611993003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js (right): https://codereview.chromium.org/2611993003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:1018: * @override Methods starting with underscore are private and should not be overriden. https://codereview.chromium.org/2611993003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right): https://codereview.chromium.org/2611993003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:777: this._selectionElement.style.setProperty('margin-left', (-this._computeLeftIndent()) + 'px'); Why do you need this? Can we just copy this code to specific tree outlines instead? It's not very generic. If you are fixing something general, make a separate patch please.
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/2611993003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js (right): https://codereview.chromium.org/2611993003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:1018: * @override On 2017/01/09 23:21:22, dgozman wrote: > Methods starting with underscore are private and should not be overriden. Done. https://codereview.chromium.org/2611993003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right): https://codereview.chromium.org/2611993003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:777: this._selectionElement.style.setProperty('margin-left', (-this._computeLeftIndent()) + 'px'); On 2017/01/09 23:21:22, dgozman wrote: > Why do you need this? Can we just copy this code to specific tree outlines > instead? It's not very generic. > > If you are fixing something general, make a separate patch please. The issue is that the "selection" element keeps the initial "margin-left: -10000px from treeoutline.css, except in the Elements tree outline which fixes it. It sort of works because the -10000px margin is clipped by the container, but when you add rounded corners to the selection style the rounded corners are 10000px left of where they should be. I'm not sure how I could pull this down into a specific tree outline without duplicating all the logic in this method. I made a separate patch: https://codereview.chromium.org/2623053002
This is ready for another look, if you have a chance.
On 2017/01/13 02:40:23, aboxhall wrote: > This is ready for another look, if you have a chance. Ping?
lgtm
The CQ bit was checked by aboxhall@chromium.org
The CQ bit was unchecked by aboxhall@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2623053002 Patch 60001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by aboxhall@chromium.org
The CQ bit was unchecked by aboxhall@chromium.org
The CQ bit was checked by aboxhall@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by aboxhall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2611993003/#ps120001 (title: "rebase")
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 aboxhall@chromium.org
Description was changed from ========== [DevTools] Accessibility pane: First pass syncing highlight between a11y pane and DOM pane BUG=678480 ========== to ========== [DevTools] Accessibility pane: Sync highlight between a11y pane and DOM pane BUG=678480 ==========
The CQ bit was checked by aboxhall@chromium.org
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": 120001, "attempt_start_ts": 1485212327727350,
"parent_rev": "824295e06184bab209d43d663b49fdf39cb887d5", "commit_rev":
"b11b7e7c8ade6d7ac945e772dc5776b2fe663bf8"}
Message was sent while issue was closed.
Description was changed from ========== [DevTools] Accessibility pane: Sync highlight between a11y pane and DOM pane BUG=678480 ========== to ========== [DevTools] Accessibility pane: Sync highlight between a11y pane and DOM pane BUG=678480 Review-Url: https://codereview.chromium.org/2611993003 Cr-Commit-Position: refs/heads/master@{#445600} Committed: https://chromium.googlesource.com/chromium/src/+/b11b7e7c8ade6d7ac945e772dc57... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001) as https://chromium.googlesource.com/chromium/src/+/b11b7e7c8ade6d7ac945e772dc57... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
