-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Define button layout #4143
Define button layout #4143
Conversation
Also note that in w3c/csswg-drafts#3226 there is a discussion about ways to "unstyle" buttons. |
@MatsPalmgren can you please review this and comment if anything is different from Gecko but shouldn't be? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes looks fine to with the nits below addressed as you see fit.
BTW, it's a bit hard to review a patch file on HTML markup like this, and I don't really have much time to review individual changes like this, so it's probably better if I give feedback on the final document instead (or at least somewhat complete snapshots thereof).
<p>The <code>button</code> element expected to render as an <span>'inline-block'</span> box | ||
depicting a button whose contents are the contents of the element.</p> | ||
<p>The <code>button</code> element, when it generates a box (i.e., is not 'display: none' or | ||
'display: contents') and the conditions below do not both apply is expected to depict a button |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "expected to depict a button" is too general. You can make it depict various things using -webkit-appearance
in current crop of UAs... although perhaps we should restrict some of that. Depends on a spec for "which elements should each of the -webkit-appearance values apply to".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I would need to revisit this when the appearance spec can be referenced. For now "depict a button" is no change from the previous text.
source
Outdated
palette) from which the color can be changed. The element is expected to use <span>button | ||
layout</span>, that has no child boxes of the <span>anonymous button content box</span>. The | ||
<span>anonymous button content box</span> is expected to have a <span data-x="presentational | ||
hints">presentational hint</span> setting the <span>'background-color'</span> property to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also depends on -webkit-appearance
.
source
Outdated
@@ -113251,11 +113323,14 @@ details[open] > summary { | |||
<h4>The <code>input</code> element as a file upload control</h4> | |||
|
|||
<p>An <code>input</code> element whose <code data-x="attr-input-type">type</code> attribute is in | |||
the <span data-x="attr-input-type-file">File Upload</span> state is expected to render as an | |||
the <span data-x="attr-input-type-file">File Upload</span> state, when it generates a box (i.e., | |||
is not 'display: none' or 'display: contents'), is expected to render as an | |||
<span>'inline-block'</span> box containing a span of text giving the file name(s) of the <span | |||
data-x="concept-input-type-file-selected">selected files</span>, if any, followed by a button |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"file name(s) ... followed by a button" -- the button is typically before the file name(s), but the spec should probably be worded to not make any assumptions about placement. Also, the rendering is affected by -webkit-appearance
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed #4276
This matches WebKit/Chromium and is consistent with other buttons. Fixes #4281.
PRs in whatwg/html (and several other repos) have generated HTML diffs, linked from the description. Does that help? |
Follows whatwg/html#4143. (More tests will be added to this PR.)
Per w3c/csswg-drafts#3226 (comment) I will remove the "display: inline and appearance: none disables button layout" for now, so we can discuss other ways to disable button layout (a new property or new appearance value or something). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a big improvement from the status quo. I have a number of questions, most of the form "is this formally defined anywhere we could xref", that are mostly reflective of my unfamiliarity with this space. But hopefully they're helpful.
Given the improvement, I'm happy to get this merged and consider none of my comments blocking. Please address as many or as few as makes sense to you.
source
Outdated
<li><p>If the computed value of <span>'display'</span> is 'inline-grid', 'grid', | ||
'inline-flex', or 'flex', then behave as the computed value.</p></li> | ||
|
||
<li><p>Otherwise, if the computed value of <span>'display'</span> is inline-level, then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason inline-level is not quoted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inline-level is not a value, but that means this sentence is skipping a level of indirection.
It should probably say something like "if the computed value of 'display' is a value such that the outer display type is 'inline'"
</ul> | ||
</li> | ||
|
||
<li><p>The element is expected to establish a new formatting context for its contents. The type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we xref "formatting context" to something?
<li><p>The element is expected to establish a new formatting context for its contents. The type | ||
of this formatting context is determined by its <span>'display'</span> value, as usual.</p></li> | ||
|
||
<li><p>If the element is absolutely positioned, then for the purpose of the CSS visual |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is "absolutely positioned" formally defined anywhere we could link to? (If not, oh well.)
of this formatting context is determined by its <span>'display'</span> value, as usual.</p></li> | ||
|
||
<li><p>If the element is absolutely positioned, then for the purpose of the CSS visual | ||
formatting model, act as if the element is a replaced element. <ref spec=CSS></p></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
xref replaced element
<li><p>If the element is absolutely positioned, then for the purpose of the CSS visual | ||
formatting model, act as if the element is a replaced element. <ref spec=CSS></p></li> | ||
|
||
<li><p>The element's box shrink-wraps.</p></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, maybe something like
"If 'inline-size' is 'auto', then the used value is the fit-content inline size"
<li><p>The element's box shrink-wraps.</p></li> | ||
|
||
<li><p>For the purpose of the 'normal' keyword of the <span>'align-self'</span> property, act | ||
as if the element is a replaced element.</p></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
xref replaced element
following behaviors:</p> | ||
|
||
<ul> | ||
<li><p>The <span>'display'</span> property is set to 'flow-root'.</p></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is "set to" sufficiently precise? (It may be, I'm not a CSS expert.) It seems different than other parts of the spec which talk about computed values or presentation hints or similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really know if it's precise, and I don't know of anything to link to. It's different, but this is an anonymous box -- we get to say what it is more directly, and the property is not directly observable. But I've changed this to say what the effect is on the box, instead.
source
Outdated
layout</span>, that has no child boxes of the <span>anonymous button content box</span>. The | ||
<span>anonymous button content box</span> is expected to have a <span data-x="presentational | ||
hints">presentational hint</span> setting the <span>'background-color'</span> property to the | ||
element's <span data-x="concept-fe-value">value</span>.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Color well does not have the "when it generates a box (i.e., is not display: none or display: contents)" that the others have. Is that intentional? Is that sentence necessary in general---perhaps it could be factored out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's necessary in that we don't want to require things like "behave as display: inline-block" on elements that are display: none. The omission here was not intentional. I'll file an issue on factoring it out.
Thanks, @domenic. Referencing terms is indeed useful to do! I'm insure about how to properly define "shrink-wrap" (in CSS 2.2 it's referred to as "shrink-to-fit"). @tabatkins is there a term we can use? See https://github.com/whatwg/html/pull/4143/files#diff-36cd38f49b9afa08222c0dc9ebfe35ebR113123 for how I've specified it in this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the additional info! LGTM now.
OK, I have now filed bugs (see OP). Let's merge. |
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
<p>The following features are defined in the <cite>CSS Flexible Box Layout</cite> | ||
specification: <ref spec=CSSFLEXBOX></p> | ||
|
||
<ul class="brief"> | ||
<li>The <dfn data-x-href="https://drafts.csswg.org/css-flexbox/#propdef-align-self">'align-self'</dfn> property</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the definition from the CSS Box Alignment be referenced instead? The hystorical Flexbox definition doesn't seem to have the normal
value at all...
</ul> | ||
|
||
<p>The term <dfn | ||
data-x-href="https://drafts.csswg.org/css-display/#block-level">block-level</dfn> is defined in | ||
<p>The following terms and features are defined in theis defined in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant, ungrammatical fragment: "theis defined in"
These were introduced in #4143.
These were introduced in #4143.
Automatic update from web-platform-tests HTML: Tests for button layout Follows whatwg/html#4143. -- wpt-commits: 849c1240850a4749e58bd87fe9f7233047cfbad4 wpt-pr: 14824
Automatic update from web-platform-tests HTML: Tests for button layout Follows whatwg/html#4143. -- wpt-commits: 849c1240850a4749e58bd87fe9f7233047cfbad4 wpt-pr: 14824
Automatic update from web-platform-tests HTML: Tests for button layout Follows whatwg/html#4143. -- wpt-commits: 849c1240850a4749e58bd87fe9f7233047cfbad4 wpt-pr: 14824 UltraBlame original commit: ea2740416f462d0fc0903057878480e8cfb94f30
Automatic update from web-platform-tests HTML: Tests for button layout Follows whatwg/html#4143. -- wpt-commits: 849c1240850a4749e58bd87fe9f7233047cfbad4 wpt-pr: 14824 UltraBlame original commit: ea2740416f462d0fc0903057878480e8cfb94f30
Automatic update from web-platform-tests HTML: Tests for button layout Follows whatwg/html#4143. -- wpt-commits: 849c1240850a4749e58bd87fe9f7233047cfbad4 wpt-pr: 14824 UltraBlame original commit: ea2740416f462d0fc0903057878480e8cfb94f30
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
cc @MatsPalmgren @tkent-google @whsieh @bzbarsky
/index.html ( diff )
/infrastructure.html ( diff )
/references.html ( diff )
/rendering.html ( diff )