web_dev_style: discourage CSS *-left/right properties and values
Instead, suggest equivalent *-start/end values that automatically
flip in RTL. For example, if the presubmit finds something along
the lines of:
margin-left: 5px;
It'll flag it and encourage the directionally-aware:
margin-inline-start: 5px;
Unless annotated with:
margin-left: 5px; /* csschecker-disable-line left-right */
for code that's truly direction agnostic.
R=dpapad@chromium.org
BUG=921226
Change-Id: If649c4eed125af4dfd5cb8b3284dc1c900230847
Reviewed-on: https://chromium-review.googlesource.com/c/1407787
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Commit-Queue: Dan Beam <dbeam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#623096}diff --git a/tools/web_dev_style/css_checker.py b/tools/web_dev_style/css_checker.py
index ec5680ad..da17bd8 100644
--- a/tools/web_dev_style/css_checker.py
+++ b/tools/web_dev_style/css_checker.py
@@ -13,6 +13,10 @@
# TODO(dbeam): Real CSS parser? https://github.com/danbeam/css-py/tree/css3
class CSSChecker(object):
+ DISABLE_PREFIX = 'csschecker-disable'
+ DISABLE_FORMAT = DISABLE_PREFIX + '(-[a-z]+)+ [a-z-]+(-[a-z-]+)*'
+ DISABLE_LINE = DISABLE_PREFIX + '-line'
+
def __init__(self, input_api, output_api, file_filter=None):
self.input_api = input_api
self.output_api = output_api
@@ -31,7 +35,7 @@
def _remove_all(s):
s = _remove_grit(s) # Must be done first.
s = _remove_ats(s)
- s = _remove_comments(s)
+ s = _remove_comments_except_for_disables(s)
s = _remove_mixins_and_valid_vars(s)
s = _remove_template_expressions(s)
return s
@@ -47,8 +51,9 @@
.*?} # stuff up to the first end curly }
""", r'\1', s, flags=re.DOTALL | re.VERBOSE)
- def _remove_comments(s):
- return re.sub(r'/\*.*?\*/', '', s, flags=re.DOTALL)
+ def _remove_comments_except_for_disables(s):
+ return re.sub(r'/\*(?! %s \*/$).*?\*/' % self.DISABLE_FORMAT,'', s,
+ flags=re.DOTALL | re.MULTILINE)
def _remove_grit(s):
return re.sub(r"""
@@ -63,6 +68,11 @@
mixin_or_value = r'({.*?}|[^;}]+);?\s*'
return re.sub(valid_vars + mixin_or_value, '', s, flags=re.DOTALL)
+ def _remove_disable(content, lstrip=False):
+ prefix_reg = ('\s*' if lstrip else '')
+ disable_reg = '/\* %s \*/' % self.DISABLE_FORMAT
+ return re.sub(prefix_reg + disable_reg, '', content, re.MULTILINE)
+
def _remove_template_expressions(s):
return re.sub(r'\$i18n(Raw)?{[^}]*}', '', s, flags=re.DOTALL)
@@ -79,8 +89,9 @@
def alphabetize_props(contents):
errors = []
# TODO(dbeam): make this smart enough to detect issues in mixins.
+ strip_rule = lambda t: _remove_disable(t).strip()
for rule in re.finditer(r'{(.*?)}', contents, re.DOTALL):
- semis = map(lambda t: t.strip(), rule.group(1).split(';'))[:-1]
+ semis = map(strip_rule, rule.group(1).split(';'))[:-1]
rules = filter(lambda r: ': ' in r, semis)
props = map(lambda r: r[0:r.find(':')], rules)
if props != sorted(props):
@@ -176,6 +187,7 @@
return re.search('url\s*\(\s*["\']', line, re.IGNORECASE)
def one_rule_per_line(line):
+ line = _remove_disable(line)
one_rule_reg = re.compile(r"""
[\w-](?<!data): # a rule: but no data URIs
(?!//)[^;]+; # value; ignoring colons in protocols:// and };
@@ -285,9 +297,9 @@
""", re.VERBOSE)
def suggest_unprefixed_logical_axis(line):
- preffix, prop = prefixed_logical_axis_reg.search(line).groups()
+ prefix, prop = prefixed_logical_axis_reg.search(line).groups()
block_or_inline = 'block' if prop == 'height' else 'inline'
- return ' (replace with %s)' % (preffix + block_or_inline + '-size')
+ return ' (replace with %s)' % (prefix + block_or_inline + '-size')
def prefixed_logical_axis(line):
return prefixed_logical_axis_reg.search(line)
@@ -313,6 +325,23 @@
def prefixed_logical_side(line):
return prefixed_logical_side_reg.search(line)
+ _LEFT_RIGHT_REG = '(?:(border|margin|padding)-|(text-align): )' \
+ '(left|right)' \
+ '(?:(-[a-z-^:]+):)?(?!.*/\* %s left-right \*/)' % \
+ self.DISABLE_LINE
+
+ def start_end_instead_of_left_right(line):
+ return re.search(_LEFT_RIGHT_REG, line, re.IGNORECASE)
+
+ def suggest_start_end_from_left_right(line):
+ groups = re.search(_LEFT_RIGHT_REG, line, re.IGNORECASE).groups()
+ prop_start, text_align, left_right, prop_end = groups
+ start_end = {'left': 'start', 'right': 'end'}[left_right]
+ if text_align:
+ return ' (replace with text-align: %s)' % start_end
+ prop = '%s-inline-%s%s' % (prop_start, start_end, prop_end or '')
+ return ' (replace with %s)' % prop
+
def zero_width_lengths(contents):
hsl_reg = re.compile(r"""
hsl\([^\)]* # hsl(maybestuff
@@ -401,6 +430,13 @@
'test': prefixed_logical_side,
'after': suggest_unprefixed_logical_side,
},
+ {
+ 'desc': 'Use -start/end instead of -left/right ' \
+ '(https://goo.gl/gQYY7z, add /* %s left-right */ to ' \
+ 'suppress)' % self.DISABLE_LINE,
+ 'test': start_end_instead_of_left_right,
+ 'after': suggest_start_end_from_left_right,
+ },
{ 'desc': 'Use "0" for zero-width lengths (i.e. 0px -> 0)',
'test': zero_width_lengths,
'multiline': True,
@@ -445,7 +481,7 @@
lines = f[1].splitlines()
for lnum, line in enumerate(lines):
if check['test'](line):
- error = ' ' + line.strip()
+ error = ' ' + _remove_disable(line, lstrip=True).strip()
if 'after' in check:
error += check['after'](line)
check_errors.append(error)
diff --git a/tools/web_dev_style/css_checker_test.py b/tools/web_dev_style/css_checker_test.py
index fa066195..a122afd 100755
--- a/tools/web_dev_style/css_checker_test.py
+++ b/tools/web_dev_style/css_checker_test.py
@@ -133,11 +133,11 @@
def testCssAlphaWithLongerDashedProps(self):
self.VerifyContentsProducesOutput("""
div {
- border-left: 5px; /* A hopefully removed comment. */
+ border-inline-start: 5px; /* A hopefully removed comment. */
border: 5px solid red;
}""", """
- Alphabetize properties and list vendor specific (i.e. -webkit) above standard.
- border-left: 5px;
+ border-inline-start: 5px;
border: 5px solid red;""")
def testCssAlphaWithVariables(self):
@@ -466,6 +466,24 @@
-webkit-padding-end: 3px; (replace with padding-inline-end)
-webkit-padding-start: 4px; (replace with padding-inline-start)""")
+ def testStartEndInsteadOfLeftRight(self):
+ self.VerifyContentsProducesOutput("""
+.inline-node {
+ --var-is-ignored-left: 10px;
+ --var-is-ignored-right: 10px;
+ border-left-color: black;
+ border-right: 1px solid blue; /* csschecker-disable-line left-right */
+ margin-right: 5px;
+ padding-left: 10px; /* csschecker-disable-line some-other-thing */
+ text-align: right;
+}""", """
+- Use -start/end instead of -left/right (https://goo.gl/gQYY7z, add /* csschecker-disable-line left-right */ to suppress)
+ border-left-color: black; (replace with border-inline-start-color)
+ margin-right: 5px; (replace with margin-inline-end)
+ padding-left: 10px; (replace with padding-inline-start)
+ text-align: right; (replace with text-align: end)
+""")
+
def testCssZeroWidthLengths(self):
self.VerifyContentsProducesOutput("""
@-webkit-keyframe anim {
