Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2607103004:
DevTools: Make sure a horizontal scrollbar doesn't appear in SuggestBox (Closed)
|
Created:
3 years, 11 months ago by einbinder Modified:
3 years, 11 months ago Reviewers:
dgozman CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, pfeldman Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: Make sure a horizontal scrollbar doesn't appear in SuggestBox
BUG=none
Review-Url: https://codereview.chromium.org/2607103004
Cr-Commit-Position: refs/heads/master@{#441779}
Committed: https://chromium.googlesource.com/chromium/src/+/f4b2e47af70ac16412fc82c81db5eb16bb34f794
Patch Set 1 #
Total comments: 3
Patch Set 2 : Move overflow hidden to client #
Messages
Total messages: 20 (12 generated)
einbinder@chromium.org changed reviewers: + dgozman@chromium.org
ptal
The CQ bit was checked by einbinder@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...
https://codereview.chromium.org/2607103004/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/ListControl.js (right): https://codereview.chromium.org/2607103004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/ListControl.js:56: this.element.style.overflowX = 'hidden'; I think this line belongs to clients. I can easily imagine viewport with a horizontal scrollbar, e.g. navigator view.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2607103004/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/ListControl.js (right): https://codereview.chromium.org/2607103004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/ListControl.js:56: this.element.style.overflowX = 'hidden'; On 2016/12/30 at 02:30:40, dgozman wrote: > I think this line belongs to clients. I can easily imagine viewport with a horizontal scrollbar, e.g. navigator view. The ListControl doesn't viewport horizontally, so it shouldn't scroll horizontally. It can easily be wrapped in a div that scrolls horizontally.
On 2017/01/03 20:06:54, einbinder wrote: > https://codereview.chromium.org/2607103004/diff/1/third_party/WebKit/Source/d... > File third_party/WebKit/Source/devtools/front_end/ui/ListControl.js (right): > > https://codereview.chromium.org/2607103004/diff/1/third_party/WebKit/Source/d... > third_party/WebKit/Source/devtools/front_end/ui/ListControl.js:56: > this.element.style.overflowX = 'hidden'; > On 2016/12/30 at 02:30:40, dgozman wrote: > > I think this line belongs to clients. I can easily imagine viewport with a > horizontal scrollbar, e.g. navigator view. > > The ListControl doesn't viewport horizontally, so it shouldn't scroll > horizontally. It can easily be wrapped in a div that scrolls horizontally. That would still leave elements clipped by viewport's element. The fact that ListControl doesn't do anything with horizontal scroll/viewport means it should all be controlled by the client.
https://codereview.chromium.org/2607103004/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/ListControl.js (right): https://codereview.chromium.org/2607103004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/ListControl.js:56: this.element.style.overflowX = 'hidden'; On 2017/01/03 at 20:06:53, einbinder wrote: > On 2016/12/30 at 02:30:40, dgozman wrote: > > I think this line belongs to clients. I can easily imagine viewport with a horizontal scrollbar, e.g. navigator view. > > The ListControl doesn't viewport horizontally, so it shouldn't scroll horizontally. It can easily be wrapped in a div that scrolls horizontally. That makes sense. Done.
The CQ bit was checked by einbinder@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...
lgtm
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 einbinder@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": 20001, "attempt_start_ts": 1483656858616830,
"parent_rev": "cad9e8497986d345637e875bcc05952bc34840de", "commit_rev":
"f4b2e47af70ac16412fc82c81db5eb16bb34f794"}
Message was sent while issue was closed.
Description was changed from ========== DevTools: Make sure a horizontal scrollbar doesn't appear in SuggestBox BUG=none ========== to ========== DevTools: Make sure a horizontal scrollbar doesn't appear in SuggestBox BUG=none Review-Url: https://codereview.chromium.org/2607103004 Cr-Commit-Position: refs/heads/master@{#441779} Committed: https://chromium.googlesource.com/chromium/src/+/f4b2e47af70ac16412fc82c81db5... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/f4b2e47af70ac16412fc82c81db5... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
