HTML: Tests for button layout #14824
Conversation
wpt-pr-bot
requested review from
annevk,
domenic,
foolip,
jdm,
jgraham and
zqzhang
|
Chromium bug for display: grid/inline-grid on buttons: https://bugs.chromium.org/p/chromium/issues/detail?id=758717 |
|
Chromium bug for wrong baseline for inline-block buttons: https://bugs.chromium.org/p/chromium/issues/detail?id=894849 |
|
Chromium bug for not wrapping |
|
Bugs for abspos.html test: |
zcorpan
force-pushed
the
zcorpan/button-layout
branch
from
7fe8829
to
49fd0a6
Compare
zcorpan
changed the title
|
I think this is ready for review now. |
|
A thing I couldn't figure out how to test is the effect of |
|
@MatsPalmgren or @emilio, are you able to review this? |
|
@MatsPalmgren is a better reviewer for this than me I think. @dholbert may be able to review this more easily too? But otherwise sure, I'll try to get to this over the week. |
|
Friendly ping :) |
| <!doctype html> | ||
| <title>default style on buttons</title> | ||
| <script src="/resources/testharness.js"></script> | ||
| <script src="/resources/testharnessreport.js"></script> |
There was a problem hiding this comment.
Could you link to the HTML spec PR from here? Or just adding links when it gets merged into the spec I guess.
There was a problem hiding this comment.
The relevant section of the spec is supposed to be able to get from the last dir name for html, I think. Is that enough?
| <button></button> | ||
| </div> | ||
| <script> | ||
| [].slice.call(document.querySelectorAll('.tests > *')).forEach(el => { |
There was a problem hiding this comment.
nit: querySelectoAll should be iterable, so you can call forEach on it.
There was a problem hiding this comment.
Yeah but it hasn't been for long (not sure it works cross-browser yet?), I didn't want to rely on newish features here.
| }, `default display on ${el.outerHTML}`); | ||
| test(() => { | ||
| assert_equals(getComputedStyle(el).boxSizing, 'border-box'); | ||
| }, `default box-sizing on ${el.outerHTML}`);}) |
There was a problem hiding this comment.
nit: I'd move the }) to the next line.
| <button id=overconstrained>test</button> | ||
| <button id=ltr>test</button> | ||
| <button id=rtl>test</button> | ||
| <script> |
There was a problem hiding this comment.
just double-checking, is there a WebKit / Blink bug on this test failing for rtl?
| const attrs = el.type ? ` type=${el.type}` : ''; | ||
| const tag = `<${el.localName}${attrs}>`; | ||
| test(() => { | ||
| assert_not_equals(el.style.display, '', `display: ${val} is not supported`) |
There was a problem hiding this comment.
Maybe we should skip the test if the display value is not supported?
There was a problem hiding this comment.
No, that makes it hard to reason about test results between browsers and over time.
| <button id=button style="display: block;"><div class=float></div></button><span id=after>after</span> | ||
| <script> | ||
| // These should all behave as flow-root. | ||
| const displayValues = ['run-in', 'flow', 'flow-root', 'table', 'list-item', |
There was a problem hiding this comment.
nobody implements run-in, so maybe remove it from the list? Though not a huge deal otherwise I guess?
There was a problem hiding this comment.
Can we remove it from the spec? Cc @tabatkins
There was a problem hiding this comment.
run-in is still a useful tool (the use-cases from 20 years ago are just as valid today, and just as annoying to work around), and we editors believe it's specified in an implementable, useful way. I'd prefer not to let chicken-and-egg issues ruin this.
| // These should all behave as flow-root. | ||
| const displayValues = ['run-in', 'flow', 'flow-root', 'table', 'list-item', | ||
| 'table-row-group', 'table-header-group', 'table-footer-group', | ||
| 'table-row', 'table-cell', 'table-column-group', 'table-column', |
There was a problem hiding this comment.
Also nobody seems to pass the table parts tests re. offsetLeft / offsetRight, are we sure we really want to treat them as flow-root?
There was a problem hiding this comment.
The table values also don't "work"... Not sure what actually happens but I figured this was close enough for web compat and still simple.
Or do offsetLeft/Top check computed value of display and have different behavior for table values?
|
Is this waiting for anything or should this just land? |
|
I think this and the spec change should be ready to merge. They have both been reviewed. I'll merge both together, but will check with the HTML editors, first. |
This specifies the layout model of buttons (the button element, the input element in the Button, Reset, Submit states, and the button part of input in the File Upload state). Fixes #1024. Fixes #2948. Fixes #4081. Part of #4082. Tests: web-platform-tests/wpt#14824 Bugs: https://bugs.chromium.org/p/chromium/issues/detail?id=962936 https://bugs.webkit.org/show_bug.cgi?id=197879 https://bugzilla.mozilla.org/show_bug.cgi?id=1539469
Follows whatwg/html#4143. (More tests will be added to this PR.) Test grid/inline-grid on button Test flex/inline-flex on button Test inline-level (except ruby) Test other display values on button Add a clarifying comment Test non-auto left/right on abspos button Test that buttons shrink-wrap Test propagation of text-decoration into buttons Test aspects of the anonymous button content box
zcorpan
force-pushed
the
zcorpan/button-layout
branch
from
c70defa
to
bd3b497
Compare
|
Rebased on master. When checks are green, it's OK to squash and merge with this commit message |
Follows whatwg/html#4143.