|
|
Created:
3 years, 8 months ago by scottchen Modified:
3 years, 8 months ago CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: more consistently fix error message space for search engine
I found a really weird behavior -
<paper-input-container> has an "@apply(--paper-input-container)" rule, which generates a "padding: var(--paper-input-container_-_padding, 8px 0)" rule when I set "padding-bottom" in the mixin from my element.
However, about 3 out of 10 times, the renderer doesn't seem to recognize that "padding-bottom" is part of "padding" and behaves like, "well the 'padding' rule isn't specified so I'm gonna render the fallback '8px 0'".
So I explicitly specified a "padding" rule and the intermittent bug went away.
BUG=703528
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2788803003
Cr-Commit-Position: refs/heads/master@{#461502}
Committed: https://chromium.googlesource.com/chromium/src/+/bc5ec856b729b019f0933b04baca929dcca60626
Patch Set 1 #
Total comments: 2
Patch Set 2 : consolidate css rules #Messages
Total messages: 18 (9 generated)
Description was changed from ========== MD Settings: more consistently fix erorr message space for search engine BUG=703528 ========== to ========== MD Settings: more consistently fix erorr message space for search engine BUG=703528 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: more consistently fix erorr message space for search engine BUG=703528 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: more consistently fix erorr message space for search engine I found a really weird behavior - <paper-input-container> has an "@apply(--paper-input-container)" rule, which generates a "padding: var(--paper-input-container_-_padding, 8px 0)" rule when I set "padding-bottom" in the mixin from my element. However, about 3 out of 10 times, the renderer doesn't seem to recognize that "padding-bottom" is part of "padding" and behaves like, "well the 'padding' rule isn't specified so I'm gonna render the fallback '8px 0'". So I explicitly specified a "padding" rule and the intermittent bug went away. BUG=703528 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
scottchen@chromium.org changed reviewers: + dbeam@chromium.org, dpapad@chromium.org
dbeam@ have you seen something like this (what I described in the CL description)?
https://codereview.chromium.org/2788803003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/search_engines_page/search_engine_dialog.html (right): https://codereview.chromium.org/2788803003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.html:17: padding: 8px 0; Given the complication you discovered, would be it more readable to simply merge both padding rules to the following? padding: 8px 0 calc(1rem - 8px) 0;
On 2017/03/31 01:13:02, scottchen wrote: > dbeam@ have you seen something like this (what I described in the CL > description)? no, michaelpg might know though
Changed it to just padding, tested 20 times refreshing the page and spacing seems consistently good now. https://codereview.chromium.org/2788803003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/search_engines_page/search_engine_dialog.html (right): https://codereview.chromium.org/2788803003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.html:17: padding: 8px 0; On 2017/03/31 01:22:18, dpapad wrote: > Given the complication you discovered, would be it more readable to simply merge > both padding rules to the following? > > padding: 8px 0 calc(1rem - 8px) 0; Done.
Typo in CL description, s/erorr/error LGTM
Description was changed from ========== MD Settings: more consistently fix erorr message space for search engine I found a really weird behavior - <paper-input-container> has an "@apply(--paper-input-container)" rule, which generates a "padding: var(--paper-input-container_-_padding, 8px 0)" rule when I set "padding-bottom" in the mixin from my element. However, about 3 out of 10 times, the renderer doesn't seem to recognize that "padding-bottom" is part of "padding" and behaves like, "well the 'padding' rule isn't specified so I'm gonna render the fallback '8px 0'". So I explicitly specified a "padding" rule and the intermittent bug went away. BUG=703528 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: more consistently fix error message space for search engine I found a really weird behavior - <paper-input-container> has an "@apply(--paper-input-container)" rule, which generates a "padding: var(--paper-input-container_-_padding, 8px 0)" rule when I set "padding-bottom" in the mixin from my element. However, about 3 out of 10 times, the renderer doesn't seem to recognize that "padding-bottom" is part of "padding" and behaves like, "well the 'padding' rule isn't specified so I'm gonna render the fallback '8px 0'". So I explicitly specified a "padding" rule and the intermittent bug went away. BUG=703528 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by scottchen@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 unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by scottchen@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": 1491240367742820, "parent_rev": "6576ecf83ab56518daa1274dfa354aee84c8696b", "commit_rev": "bc5ec856b729b019f0933b04baca929dcca60626"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings: more consistently fix error message space for search engine I found a really weird behavior - <paper-input-container> has an "@apply(--paper-input-container)" rule, which generates a "padding: var(--paper-input-container_-_padding, 8px 0)" rule when I set "padding-bottom" in the mixin from my element. However, about 3 out of 10 times, the renderer doesn't seem to recognize that "padding-bottom" is part of "padding" and behaves like, "well the 'padding' rule isn't specified so I'm gonna render the fallback '8px 0'". So I explicitly specified a "padding" rule and the intermittent bug went away. BUG=703528 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: more consistently fix error message space for search engine I found a really weird behavior - <paper-input-container> has an "@apply(--paper-input-container)" rule, which generates a "padding: var(--paper-input-container_-_padding, 8px 0)" rule when I set "padding-bottom" in the mixin from my element. However, about 3 out of 10 times, the renderer doesn't seem to recognize that "padding-bottom" is part of "padding" and behaves like, "well the 'padding' rule isn't specified so I'm gonna render the fallback '8px 0'". So I explicitly specified a "padding" rule and the intermittent bug went away. BUG=703528 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2788803003 Cr-Commit-Position: refs/heads/master@{#461502} Committed: https://chromium.googlesource.com/chromium/src/+/bc5ec856b729b019f0933b04baca... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/bc5ec856b729b019f0933b04baca... |