Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2033093004:
DevTools: fix invalid outline style across inputs (Closed)
|
Created:
4 years, 6 months ago by luoe Modified:
4 years, 6 months ago Reviewers:
dgozman CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, sergeyv+blink_chromium.org, pfeldman, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: fix invalid outline style across inputs
This makes the custom UA input use the same error highlighting style
as other inputs in DevTools, and fixes the outline offset present when
using OS X.
BUG=616732
Committed: https://crrev.com/de4a6e8d6d598c76b84e6aa85dfb82b55b3d6ae8
Cr-Commit-Position: refs/heads/master@{#400793}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address comments #Patch Set 3 : Address comments; introduce applyOnInvalid #Patch Set 4 : Address comments #
Messages
Total messages: 29 (12 generated)
luoe@chromium.org changed reviewers: + dgozman@chromium.org
Description was changed from ========== DevTools: fix invalid outline style across inputs BUG=616732 ========== to ========== DevTools: fix invalid outline style across inputs This makes the custom UA input use the same error highlighting style as other inputs in DevTools, and fixes the outline offset present when using OS X. BUG=616732 ==========
Description was changed from ========== DevTools: fix invalid outline style across inputs This makes the custom UA input use the same error highlighting style as other inputs in DevTools, and fixes the outline offset present when using OS X. BUG=616732 ========== to ========== DevTools: fix invalid outline style across inputs This makes the custom UA input use the same error highlighting style as other inputs in DevTools, and fixes the outline offset present when using OS X. BUG=616732 ==========
There is also one in ListWidget. Let's update it as well? https://codereview.chromium.org/2033093004/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js (right): https://codereview.chromium.org/2033093004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:115: var otherUserAgentSetter = WebInspector.bindInput(otherUserAgentElement, textChanged, (value) => (value.length > 0), false); value => !!value https://codereview.chromium.org/2033093004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:119: otherUserAgentElement.addEventListener("input", textChanged, false); Remove this line?
About ListWidget: It does use the "error-input" class during validation of multiple inputs. I started adding bindInput() there as well, but it resisted the change. They have a couple differences which aren't compatible with the generic bindInput(). - It wants to know whether all inputs in a group are valid, so it can enable/disable the commit button - It wants to force all inputs to be valid (no red outline), when reset Due to these changes, I think they ought to use a bindInputGroup(), not bindInput(). However, it seems(?) like it's the only component that manages multiple inputs, so we could leave it be. https://codereview.chromium.org/2033093004/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js (right): https://codereview.chromium.org/2033093004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:115: var otherUserAgentSetter = WebInspector.bindInput(otherUserAgentElement, textChanged, (value) => (value.length > 0), false); On 2016/06/02 21:20:51, dgozman wrote: > value => !!value Done. https://codereview.chromium.org/2033093004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:119: otherUserAgentElement.addEventListener("input", textChanged, false); On 2016/06/02 21:20:51, dgozman wrote: > Remove this line? Without this, textChanged only gets called when the user blurs or presses enter after a change, but we really want to call it on every input. I added a new boolean option to bindInput called 'applyOnInput'. What do you think?
Sorry, this slipped through my review list. On 2016/06/02 23:07:03, luoe wrote: > About ListWidget: It does use the "error-input" class during validation of > multiple inputs. I started adding bindInput() there as well, but it resisted > the change. > > They have a couple differences which aren't compatible with the generic > bindInput(). > - It wants to know whether all inputs in a group are valid, so it can > enable/disable the commit button > - It wants to force all inputs to be valid (no red outline), when reset > > Due to these changes, I think they ought to use a bindInputGroup(), not > bindInput(). However, it seems(?) like it's the only component that manages > multiple inputs, so we could leave it be. I agree. > https://codereview.chromium.org/2033093004/diff/1/third_party/WebKit/Source/d... > third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:119: > otherUserAgentElement.addEventListener("input", textChanged, false); > On 2016/06/02 21:20:51, dgozman wrote: > > Remove this line? > > Without this, textChanged only gets called when the user blurs or presses enter > after a change, but we really want to call it on every input. > > I added a new boolean option to bindInput called 'applyOnInput'. What do you > think? Hmm... Do we really need to apply UA override on input? I don't think so. Let's apply it on blur, as we do everywhere. I'd also rename textChanged to applyOtherUserAgent.
textChanged >> applyOtherUserAgent, Done. Yes, we don't really need to apply on every input, just blur is good. However, I think UserAgent is special because we do need to call apply() on blur even for invalid values. If the field is empty, we still need a callback to update the option select menu to 'Custom', which is what the apply() will do. The latest patch includes an 'applyOnInvalid'. This one-off is only applicable for the UserAgent, but is simpler than having to add another listener.
On 2016/06/09 21:15:33, luoe wrote: > textChanged >> applyOtherUserAgent, Done. > > Yes, we don't really need to apply on every input, just blur is good. However, > I think UserAgent is special because we do need to call apply() on blur even for > invalid values. > > If the field is empty, we still need a callback to update the option select menu > to 'Custom', which is what the apply() will do. The latest patch includes an > 'applyOnInvalid'. This one-off is only applicable for the UserAgent, but is > simpler than having to add another listener. Perhaps, we can just not use bindInput then? applyOnInvalid is really strange - why validate if we apply anyway? Can we just fix the styling issue?
Sounds good. Better to update just the styles instead of shoehorning bindInput into the user agent field. I've updated the latest patch.
lgtm!
The CQ bit was checked by luoe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2033093004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by luoe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2033093004/60001
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 luoe@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2033093004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by luoe@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2033093004/60001
Message was sent while issue was closed.
Description was changed from ========== DevTools: fix invalid outline style across inputs This makes the custom UA input use the same error highlighting style as other inputs in DevTools, and fixes the outline offset present when using OS X. BUG=616732 ========== to ========== DevTools: fix invalid outline style across inputs This makes the custom UA input use the same error highlighting style as other inputs in DevTools, and fixes the outline offset present when using OS X. BUG=616732 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== DevTools: fix invalid outline style across inputs This makes the custom UA input use the same error highlighting style as other inputs in DevTools, and fixes the outline offset present when using OS X. BUG=616732 ========== to ========== DevTools: fix invalid outline style across inputs This makes the custom UA input use the same error highlighting style as other inputs in DevTools, and fixes the outline offset present when using OS X. BUG=616732 Committed: https://crrev.com/de4a6e8d6d598c76b84e6aa85dfb82b55b3d6ae8 Cr-Commit-Position: refs/heads/master@{#400793} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/de4a6e8d6d598c76b84e6aa85dfb82b55b3d6ae8 Cr-Commit-Position: refs/heads/master@{#400793} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
